On Wed, 5 Sep 2012 17:02:06 -0400
"J. Bruce Fields" <bfields@xxxxxxxxxxxx> wrote:
> Oops, I meant to cc this to Jeff:
>
> On Wed, Sep 05, 2012 at 04:55:22PM -0400, J. Bruce Fields wrote:
> > @@ -3556,6 +3560,7 @@ SYSCALL_DEFINE5(linkat, int, olddfd, const char __user *, oldname,
> > if (error)
> > return error;
> >
> > +retry_deleg:
> > new_dentry = user_path_create(newdfd, newname, &new_path, 0);
> > error = PTR_ERR(new_dentry);
> > if (IS_ERR(new_dentry))
> > @@ -3570,9 +3575,14 @@ SYSCALL_DEFINE5(linkat, int, olddfd, const char __user *, oldname,
> > error = security_path_link(old_path.dentry, &new_path, new_dentry);
> > if (error)
> > goto out_dput;
> > - error = vfs_link(old_path.dentry, new_path.dentry->d_inode, new_dentry);
> > + error = vfs_link(old_path.dentry, new_path.dentry->d_inode, new_dentry, &delegated_inode);
> > out_dput:
> > done_path_create(&new_path, new_dentry);
> > + if (delegated_inode) {
> > + error = break_deleg_wait(&delegated_inode);
> > + if (!error)
> > + goto retry_deleg;
> > + }
> > out:
> > path_put(&old_path);
> >
>
> I think this will get me into trouble with the audit code, right?
>
> (On a quick skim I think I'm retrying from a point after the getname()
> in rename and unlink, so I think those are OK, but I may be missing
> something....)
>
> --b.
>
tl;dr: this patch is OK, but the unlink and rename patches will be
problematic.
Longer explanation:
The big problem is multiple calls into audit_inode_child(), which will
give you duplicate audit records when you retry the call. That function
mostly gets called from the fsnotify_* calls, but is also called from
may_delete().
In the create case (like this one), you're safe since we only call
fsnotify_* after the create succeeds. In the may_delete() case, we have
to call that function before the actual operation. A quick look at your
patches looks like you'll end up with multiple calls into may_delete on
a retry. That'll give you multiple audit records for each.
The good news is that if and when my audit_names overhaul patches go
in, that problem should go away. At that point audit_inode_child() will
know how to update existing records instead of creating new ones in
this case. I'm hoping those will go in for 3.7, so you may just want to
try to ensure that these patches go in after those.
> > diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> > index c8f5e74..2a77b28 100644
> > --- a/fs/nfsd/vfs.c
> > +++ b/fs/nfsd/vfs.c
> > @@ -1717,7 +1717,7 @@ nfsd_link(struct svc_rqst *rqstp, struct svc_fh *ffhp,
> > err = nfserrno(host_err);
> > goto out_dput;
> > }
> > - host_err = vfs_link(dold, dirp, dnew);
> > + host_err = vfs_link(dold, dirp, dnew, NULL);
> > if (!host_err) {
> > err = nfserrno(commit_metadata(ffhp));
> > if (!err)
> > diff --git a/include/linux/fs.h b/include/linux/fs.h
> > index ad9df65e..52b8c67 100644
> > --- a/include/linux/fs.h
> > +++ b/include/linux/fs.h
> > @@ -1721,7 +1721,7 @@ extern int vfs_create(struct inode *, struct dentry *, umode_t, bool);
> > extern int vfs_mkdir(struct inode *, struct dentry *, umode_t);
> > extern int vfs_mknod(struct inode *, struct dentry *, umode_t, dev_t);
> > extern int vfs_symlink(struct inode *, struct dentry *, const char *);
> > -extern int vfs_link(struct dentry *, struct inode *, struct dentry *);
> > +extern int vfs_link(struct dentry *, struct inode *, struct dentry *, struct inode **);
> > extern int vfs_rmdir(struct inode *, struct dentry *);
> > extern int vfs_unlink(struct inode *, struct dentry *, struct inode **);
> > extern int vfs_rename(struct inode *, struct dentry *, struct inode *, struct dentry *, struct inode **);
> > --
> > 1.7.9.5
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> > the body of a message to majordomo@xxxxxxxxxxxxxxx
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Jeff Layton <jlayton@xxxxxxxxxx>
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
