Re: [RFC PATCH 12/13] locks: break delegations on link

["J. Bruce Fields" <bfields@fieldses.org>]

On Thu, Sep 06, 2012 at 07:01:22AM -0400, Jeff Layton wrote:
> On Wed, 5 Sep 2012 17:02:06 -0400
> "J. Bruce Fields" <bfields@fieldses.org> 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.

OK, thanks for the explanation, I'll keep an eye on the audit_names
overhaul.

--b.

> 
> > > 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@vger.kernel.org
> > > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 
> -- 
> Jeff Layton <jlayton@redhat.com>