lkml.org 
[lkml]   [2008]   [Jun]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: Linux 2.6.26-rc4
    On Thu, 05 Jun 2008 15:31:37 +0800
    Ian Kent <raven@themaw.net> wrote:

    >
    > On Tue, 2008-06-03 at 08:01 -0700, Linus Torvalds wrote:
    > >
    > > On Tue, 3 Jun 2008, Ian Kent wrote:
    > > > >
    > > > > > I think it must be autofs4 doing something weird. Like this in
    > > > > > autofs4_lookup_unhashed():
    > > > > >
    > > > > > /*
    > > > > > * Make the rehashed dentry negative so the VFS
    > > > > > * behaves as it should.
    > > > > > */
    > > > > > if (inode) {
    > > > > > dentry->d_inode = NULL;
    > >
    > > Uhhuh. Yeah, that's not allowed.
    > >
    > > A dentry inode can start _out_ as NULL, but it can never later become NULL
    > > again until it is totally unused.
    >
    > Here is a patch for autofs4 to, hopefully, resolve this.
    >
    > Keep in mind this doesn't address any other autofs4 issues but I it
    > should allow us to identify if this was in fact the root cause of the
    > problem Jesper reported.
    >
    > autofs4 - leave rehashed dentry positive
    >
    > From: Ian Kent <raven@themaw.net>
    >
    > Correct the error of making a positive dentry negative after it has been
    > instantiated.
    >
    > This involves removing the code in autofs4_lookup_unhashed() that
    > makes the dentry negative and updating autofs4_dir_symlink() and
    > autofs4_dir_mkdir() to recognise they have been given a postive
    > dentry (previously the dentry was always negative) and deal with
    > it. In addition the dentry info struct initialization, autofs4_init_ino(),
    > and the symlink free function, ino_lnkfree(), have been made aware
    > of this possible re-use. This is needed because the current case
    > re-uses a dentry in order to preserve it's flags as described in
    > commit f50b6f8691cae2e0064c499dd3ef3f31142987f0.
    >
    > ...
    >
    > ...
    >
    > diff --git a/fs/autofs4/root.c b/fs/autofs4/root.c
    > index edf5b6b..6ce603b 100644
    > --- a/fs/autofs4/root.c
    > +++ b/fs/autofs4/root.c
    > @@ -555,24 +555,8 @@ static struct dentry *autofs4_lookup_unhashed(struct autofs_sb_info *sbi, struct
    > goto next;
    >
    > if (d_unhashed(dentry)) {
    > - struct inode *inode = dentry->d_inode;
    > -
    > - ino = autofs4_dentry_ino(dentry);
    > list_del_init(&ino->rehash);
    > dget(dentry);
    > - /*
    > - * Make the rehashed dentry negative so the VFS
    > - * behaves as it should.
    > - */
    > - if (inode) {
    > - dentry->d_inode = NULL;
    > - list_del_init(&dentry->d_alias);
    > - spin_unlock(&dentry->d_lock);
    > - spin_unlock(&sbi->rehash_lock);
    > - spin_unlock(&dcache_lock);
    > - iput(inode);
    > - return dentry;
    > - }
    > spin_unlock(&dentry->d_lock);
    > spin_unlock(&sbi->rehash_lock);
    > spin_unlock(&dcache_lock);
    > @@ -728,35 +712,50 @@ static int autofs4_dir_symlink(struct inode *dir,
    > return -EACCES;
    >
    > ino = autofs4_init_ino(ino, sbi, S_IFLNK | 0555);
    > - if (ino == NULL)
    > + if (!ino)
    > return -ENOSPC;

    Should have been ENOMEM, I guess.

    > - ino->size = strlen(symname);
    > - ino->u.symlink = cp = kmalloc(ino->size + 1, GFP_KERNEL);
    > -
    > - if (cp == NULL) {
    > - kfree(ino);
    > + cp = kmalloc(ino->size + 1, GFP_KERNEL);
    > + if (!cp) {
    > + if (!dentry->d_fsdata)
    > + kfree(ino);

    OK, so here we work out that autofs4_init_ino() had to allocate a new
    autofs_info and if so, free it here. It took me a moment..

    > return -ENOSPC;

    ENOMEM?

    > }
    >
    > strcpy(cp, symname);
    >
    > - inode = autofs4_get_inode(dir->i_sb, ino);
    > - d_add(dentry, inode);
    > + inode = dentry->d_inode;
    > + if (inode)
    > + d_rehash(dentry);
    > + else {
    > + inode = autofs4_get_inode(dir->i_sb, ino);
    > + if (!inode) {
    > + kfree(cp);
    > + if (!dentry->d_fsdata)
    > + kfree(ino);
    > + return -ENOSPC;
    > + }
    > +
    > + d_add(dentry, inode);
    >
    > - if (dir == dir->i_sb->s_root->d_inode)
    > - dentry->d_op = &autofs4_root_dentry_operations;
    > - else
    > - dentry->d_op = &autofs4_dentry_operations;
    > + if (dir == dir->i_sb->s_root->d_inode)
    > + dentry->d_op = &autofs4_root_dentry_operations;
    > + else
    > + dentry->d_op = &autofs4_dentry_operations;
    > +
    > + dentry->d_fsdata = ino;
    > + ino->dentry = dentry;
    > + ino->inode = inode;
    > + }
    > + dget(dentry);
    >
    > - dentry->d_fsdata = ino;
    > - ino->dentry = dget(dentry);
    > atomic_inc(&ino->count);
    > p_ino = autofs4_dentry_ino(dentry->d_parent);
    > if (p_ino && dentry->d_parent != dentry)
    > atomic_inc(&p_ino->count);
    > - ino->inode = inode;
    >
    > + ino->u.symlink = cp;
    > + ino->size = strlen(symname);
    > dir->i_mtime = CURRENT_TIME;

    This all seems a bit ungainly. I assume that on entry to
    autofs4_dir_symlink(), ino->size is equal to strlen(symname)? If it's
    not, that strcpy() will overrun.

    But if ino->size _is_ equal to strlen(symname) then why did we just
    recalculate the same thing?

    I'm suspecting we can zap a lump of code and just do

    cp = kstrdup(symname, GFP_KERNEL);

    Anyway, please check that.

    > return 0;
    > @@ -866,24 +865,38 @@ static int autofs4_dir_mkdir(struct inode *dir, struct dentry *dentry, int mode)
    > dentry, dentry->d_name.len, dentry->d_name.name);
    >
    > ino = autofs4_init_ino(ino, sbi, S_IFDIR | 0555);
    > - if (ino == NULL)
    > + if (!ino)
    > return -ENOSPC;

    ENOMEM?

    > - inode = autofs4_get_inode(dir->i_sb, ino);
    > - d_add(dentry, inode);
    > + inode = dentry->d_inode;
    > + if (inode)
    > + d_rehash(dentry);
    > + else {
    > + inode = autofs4_get_inode(dir->i_sb, ino);
    > + if (!inode) {
    > + if (!dentry->d_fsdata)
    > + kfree(ino);
    > + return -ENOSPC;
    > + }
    >
    > - if (dir == dir->i_sb->s_root->d_inode)
    > - dentry->d_op = &autofs4_root_dentry_operations;
    > - else
    > - dentry->d_op = &autofs4_dentry_operations;
    > + d_add(dentry, inode);
    > +
    > + if (dir == dir->i_sb->s_root->d_inode)
    > + dentry->d_op = &autofs4_root_dentry_operations;
    > + else
    > + dentry->d_op = &autofs4_dentry_operations;
    > +
    > + dentry->d_fsdata = ino;
    > + ino->dentry = dentry;
    > + ino->inode = inode;
    > + }
    > + dget(dentry);

    This all looks very similar to the code in autofs4_dir_symlink(). Some
    refactoring might be needed at some stage?

    > - dentry->d_fsdata = ino;
    > - ino->dentry = dget(dentry);
    > atomic_inc(&ino->count);
    > p_ino = autofs4_dentry_ino(dentry->d_parent);
    > if (p_ino && dentry->d_parent != dentry)
    > atomic_inc(&p_ino->count);
    > - ino->inode = inode;
    > +
    > inc_nlink(dir);
    > dir->i_mtime = CURRENT_TIME;
    >
    >


    \
     
     \ /
      Last update: 2008-06-06 00:35    [W:5.262 / U:0.844 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site