Hi Al, Do you have any comments / concerns about this patch? It's part of nfs server side user xattr support, full series here: https://lore.kernel.org/linux-nfs/20200623223927.31795-1-fllinden@xxxxxxxxxx/ I copied this one to linux-fsdevel and you, just giving you an extra ping. Bruce/Chuck are OK with the rest of the series, so I just need your ACK on this one, and the next one (will send the ping separately). Thanks, - Frank On Tue, Jun 23, 2020 at 10:39:18PM +0000, Frank van der Linden wrote: > set/removexattr on an exported filesystem should break NFS delegations. > This is true in general, but also for the upcoming support for > RFC 8726 (NFSv4 extended attribute support). Make sure that they do. > > Additonally, they need to grow a _locked variant, since callers might > call this with i_rwsem held (like the NFS server code). > > Cc: stable@xxxxxxxxxxxxxxx > Cc: linux-fsdevel@xxxxxxxxxxxxxxx > Cc: Al Viro <viro@xxxxxxxxxxxxxxxxxx> > Signed-off-by: Frank van der Linden <fllinden@xxxxxxxxxx> > --- > fs/xattr.c | 84 +++++++++++++++++++++++++++++++++++++++---- > include/linux/xattr.h | 2 ++ > 2 files changed, 79 insertions(+), 7 deletions(-) > > diff --git a/fs/xattr.c b/fs/xattr.c > index 91608d9bfc6a..95f38f57347f 100644 > --- a/fs/xattr.c > +++ b/fs/xattr.c > @@ -204,10 +204,22 @@ int __vfs_setxattr_noperm(struct dentry *dentry, const char *name, > return error; > } > > - > +/** > + * __vfs_setxattr_locked: set an extended attribute while holding the inode > + * lock > + * > + * @dentry - object to perform setxattr on > + * @name - xattr name to set > + * @value - value to set @name to > + * @size - size of @value > + * @flags - flags to pass into filesystem operations > + * @delegated_inode - on return, will contain an inode pointer that > + * a delegation was broken on, NULL if none. > + */ > int > -vfs_setxattr(struct dentry *dentry, const char *name, const void *value, > - size_t size, int flags) > +__vfs_setxattr_locked(struct dentry *dentry, const char *name, > + const void *value, size_t size, int flags, > + struct inode **delegated_inode) > { > struct inode *inode = dentry->d_inode; > int error; > @@ -216,15 +228,40 @@ vfs_setxattr(struct dentry *dentry, const char *name, const void *value, > if (error) > return error; > > - inode_lock(inode); > error = security_inode_setxattr(dentry, name, value, size, flags); > if (error) > goto out; > > + error = try_break_deleg(inode, delegated_inode); > + if (error) > + goto out; > + > error = __vfs_setxattr_noperm(dentry, name, value, size, flags); > > out: > + return error; > +} > +EXPORT_SYMBOL_GPL(__vfs_setxattr_locked); > + > +int > +vfs_setxattr(struct dentry *dentry, const char *name, const void *value, > + size_t size, int flags) > +{ > + struct inode *inode = dentry->d_inode; > + struct inode *delegated_inode = NULL; > + int error; > + > +retry_deleg: > + inode_lock(inode); > + error = __vfs_setxattr_locked(dentry, name, value, size, flags, > + &delegated_inode); > inode_unlock(inode); > + > + if (delegated_inode) { > + error = break_deleg_wait(&delegated_inode); > + if (!error) > + goto retry_deleg; > + } > return error; > } > EXPORT_SYMBOL_GPL(vfs_setxattr); > @@ -378,8 +415,18 @@ __vfs_removexattr(struct dentry *dentry, const char *name) > } > EXPORT_SYMBOL(__vfs_removexattr); > > +/** > + * __vfs_removexattr_locked: set an extended attribute while holding the inode > + * lock > + * > + * @dentry - object to perform setxattr on > + * @name - name of xattr to remove > + * @delegated_inode - on return, will contain an inode pointer that > + * a delegation was broken on, NULL if none. > + */ > int > -vfs_removexattr(struct dentry *dentry, const char *name) > +__vfs_removexattr_locked(struct dentry *dentry, const char *name, > + struct inode **delegated_inode) > { > struct inode *inode = dentry->d_inode; > int error; > @@ -388,11 +435,14 @@ vfs_removexattr(struct dentry *dentry, const char *name) > if (error) > return error; > > - inode_lock(inode); > error = security_inode_removexattr(dentry, name); > if (error) > goto out; > > + error = try_break_deleg(inode, delegated_inode); > + if (error) > + goto out; > + > error = __vfs_removexattr(dentry, name); > > if (!error) { > @@ -401,12 +451,32 @@ vfs_removexattr(struct dentry *dentry, const char *name) > } > > out: > + return error; > +} > +EXPORT_SYMBOL_GPL(__vfs_removexattr_locked); > + > +int > +vfs_removexattr(struct dentry *dentry, const char *name) > +{ > + struct inode *inode = dentry->d_inode; > + struct inode *delegated_inode = NULL; > + int error; > + > +retry_deleg: > + inode_lock(inode); > + error = __vfs_removexattr_locked(dentry, name, &delegated_inode); > inode_unlock(inode); > + > + if (delegated_inode) { > + error = break_deleg_wait(&delegated_inode); > + if (!error) > + goto retry_deleg; > + } > + > return error; > } > EXPORT_SYMBOL_GPL(vfs_removexattr); > > - > /* > * Extended attribute SET operations > */ > diff --git a/include/linux/xattr.h b/include/linux/xattr.h > index 47eaa34f8761..a2f3cd02653c 100644 > --- a/include/linux/xattr.h > +++ b/include/linux/xattr.h > @@ -51,8 +51,10 @@ ssize_t vfs_getxattr(struct dentry *, const char *, void *, size_t); > ssize_t vfs_listxattr(struct dentry *d, char *list, size_t size); > int __vfs_setxattr(struct dentry *, struct inode *, const char *, const void *, size_t, int); > int __vfs_setxattr_noperm(struct dentry *, const char *, const void *, size_t, int); > +int __vfs_setxattr_locked(struct dentry *, const char *, const void *, size_t, int, struct inode **); > int vfs_setxattr(struct dentry *, const char *, const void *, size_t, int); > int __vfs_removexattr(struct dentry *, const char *); > +int __vfs_removexattr_locked(struct dentry *, const char *, struct inode **); > int vfs_removexattr(struct dentry *, const char *); > > ssize_t generic_listxattr(struct dentry *dentry, char *buffer, size_t buffer_size); > -- > 2.17.2 >
