Re: [patch-2.4.0-test5-pre1] nullfs and forced umount

Manfred Spraul (manfred@colorfullife.com)
Tue, 18 Jul 2000 19:51:24 +0200

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/