Messages in this thread | | | Date | Tue, 18 Jul 2000 19:51:24 +0200 | From | Manfred Spraul <> | Subject | Re: [patch-2.4.0-test5-pre1] nullfs and forced umount |
| |
Tigran Aivazian wrote: > > +/* moves the inode into nullfs and bads it, called on every open file. > + * The 'root' argument is passed so that we don't down root->i_sem twice > + */ > +static void disable_fd(struct task_struct *tsk, int fd, struct dentry *root) > +{ > + struct files_struct *files; > + struct file *file; > + struct inode *inode; > + mode_t saved_mode; > + struct file_operations *fop; > + > + files = tsk->files; > + file = files->fd[fd]; > + inode = file->f_dentry->d_inode; > + fop = file->f_op; > + > + /* serialize with operations on this inode */ > + if (inode != root->d_inode) /* root is down'd by the caller */ > + down(&inode->i_sem); > +
Is it possible that disable_fd is called for a directory? Then this could dead lock with a concurrent rename. (double_down, triple_down,...)
> + lock_kernel(); > + locks_remove_posix(file, files); > + locks_remove_flock(file); > + unlock_kernel();
Now superflous, locks_remove_xy calls lock_kernel internally.
> + > + /* flush and release fs-specific resources */ > + if (fop->flush) > + fop->flush(file); > + if (fop->release) > + fop->release(inode, file);
This one could cause crashes if another thread is within kernel space. E.g. coda frees allocated memory in coda_release.
> + > +void disable_filesystem(struct vfsmount *mnt) > +{
Please use get_task_struct(), task_lock() and atomic_inc(ref_count). Your code contains the same bug as the old access_process_vm code that caused crashes in 2.3.51(+-2)
> diff -urN -X dontdiff linux/fs/super.c nullfs/fs/super.c > --- linux/fs/super.c Tue Jul 11 19:26:50 2000 > +++ nullfs/fs/super.c Tue Jul 18 17:29:34 2000 > @@ -979,6 +979,9 @@ > return retval; > } > > + if (flags&MNT_FORCE) > + disable_filesystem(mnt); > + Hmmm. What about module unload races?
thread 1: calls sys_read for an nfs file. nfs enters uninterruptible sleep.
Thread 2: calls do_umount(MNT_FORCE) for that filesystem. Succeeds. filp got killed. umount decrements the ref count for nfs. do_umount returns. user calls rmmod nfs. nfs module removed.
Thread 1: the nfs server replies, and nfs leaves the uninterruptible sleep. crash, the module is gone.
As I wrote before, IMHO the design is flawed: you can't kill an inode while another thread is working with that inode. I would prefer break_uninterruptible_sleep + SIGSTOP_INTERNAL [as SIGSTOP, but without notifying childs/parents]
-- Manfred
- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.rutgers.edu Please read the FAQ at http://www.tux.org/lkml/
| |