lkml.org 
[lkml]   [2000]   [Jul]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [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/

    \
     
     \ /
      Last update: 2005-03-22 13:57    [W:0.026 / U:0.200 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site