| Message ID | 20200311195954.27117-3-fllinden@amazon.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | server side user xattr support (RFC 8276) | expand |
> On Mar 11, 2020, at 3:59 PM, Frank van der Linden <fllinden@amazon.com> wrote: > > To be called from the upcoming NFS server xattr code, the vfs_removexattr > and vfs_setxattr need some modifications. > > First, they need to grow a _locked variant, since the NFS server code > will call this with i_rwsem held. It needs to do that in fh_lock to be > able to atomically provide the before and after change attributes. > > Second, RFC 8276 (NFSv4 extended attribute support) specifies that > delegations should be recalled (8.4.2.4, 8.4.4.4) when a SETXATTR > or REMOVEXATTR operation is performed. So, like with other fs > operations, try to break the delegation. The _locked version of > these operations will not wait for the delegation to be successfully > broken, instead returning an error if it wasn't, so that the NFS > server code can return NFS4ERR_DELAY to the client (similar to > what e.g. vfs_link does). > > Signed-off-by: Frank van der Linden <fllinden@amazon.com> Frank, I appreciate the verbosity of the patch descriptions, and thanks very much for splitting the client and server work into separate series. [cel@klimt linux]$ scripts/get_maintainer.pl fs/xattr.c Alexander Viro <viro@zeniv.linux.org.uk> (maintainer:FILESYSTEMS (VFS and infrastructure)) linux-fsdevel@vger.kernel.org (open list:FILESYSTEMS (VFS and infrastructure)) linux-kernel@vger.kernel.org (open list) [cel@klimt linux]$ So patches like this one and 13/14 (or perhaps the whole series) needs to be cc: linux-fsdevel@vger.kernel.org. At least those two patches in particular will need an Acked-by: from viro. > --- > fs/xattr.c | 63 +++++++++++++++++++++++++++++++++++++++++++++------ > include/linux/xattr.h | 2 ++ > 2 files changed, 58 insertions(+), 7 deletions(-) > > diff --git a/fs/xattr.c b/fs/xattr.c > index 90dd78f0eb27..58013bcbc333 100644 > --- a/fs/xattr.c > +++ b/fs/xattr.c > @@ -204,10 +204,10 @@ int __vfs_setxattr_noperm(struct dentry *dentry, const char *name, > return error; > } > > - > 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) Since you will need to repost, please consider adding a Doxygen comment in front of newly introduced global APIs. Such a comment would be an appropriate place to add non-NFS-related explanatory text you have provided in the patch description. Goes for global APIs introduced in other patches too. > { > struct inode *inode = dentry->d_inode; > int error; > @@ -216,15 +216,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); > @@ -379,7 +404,8 @@ __vfs_removexattr(struct dentry *dentry, const char *name) > EXPORT_SYMBOL(__vfs_removexattr); > > 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 +414,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 +430,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 6dad031be3c2..3a71ad716da5 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.16.6 > -- Chuck Lever
On Wed, Mar 11, 2020 at 07:59:42PM +0000, Frank van der Linden wrote: > To be called from the upcoming NFS server xattr code, the vfs_removexattr > and vfs_setxattr need some modifications. > > First, they need to grow a _locked variant, since the NFS server code > will call this with i_rwsem held. It needs to do that in fh_lock to be > able to atomically provide the before and after change attributes. Unlike NFSv3, NFSv4+ doesn't support atomic before- and after- change attributes for setattr. Trying to take advantage of that assumption might result in worse code, though. > Second, RFC 8276 (NFSv4 extended attribute support) specifies that > delegations should be recalled (8.4.2.4, 8.4.4.4) when a SETXATTR > or REMOVEXATTR operation is performed. So, like with other fs > operations, try to break the delegation. The _locked version of > these operations will not wait for the delegation to be successfully > broken, instead returning an error if it wasn't, so that the NFS > server code can return NFS4ERR_DELAY to the client (similar to > what e.g. vfs_link does). Is there a preexisting bug here? Even without NFS support for xattrs, a local setxattr on the filesystem should still revoke any delegations held by remote NFS clients. I couldn't tell whether we're getting that right from a quick look at the current code. --b. > > Signed-off-by: Frank van der Linden <fllinden@amazon.com> > --- > fs/xattr.c | 63 +++++++++++++++++++++++++++++++++++++++++++++------ > include/linux/xattr.h | 2 ++ > 2 files changed, 58 insertions(+), 7 deletions(-) > > diff --git a/fs/xattr.c b/fs/xattr.c > index 90dd78f0eb27..58013bcbc333 100644 > --- a/fs/xattr.c > +++ b/fs/xattr.c > @@ -204,10 +204,10 @@ int __vfs_setxattr_noperm(struct dentry *dentry, const char *name, > return error; > } > > - > 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 +216,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); > @@ -379,7 +404,8 @@ __vfs_removexattr(struct dentry *dentry, const char *name) > EXPORT_SYMBOL(__vfs_removexattr); > > 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 +414,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 +430,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 6dad031be3c2..3a71ad716da5 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.16.6
On Fri, Mar 13, 2020 at 11:35:49AM -0400, J. Bruce Fields wrote: > > > On Wed, Mar 11, 2020 at 07:59:42PM +0000, Frank van der Linden wrote: > > Second, RFC 8276 (NFSv4 extended attribute support) specifies that > > delegations should be recalled (8.4.2.4, 8.4.4.4) when a SETXATTR > > or REMOVEXATTR operation is performed. So, like with other fs > > operations, try to break the delegation. The _locked version of > > these operations will not wait for the delegation to be successfully > > broken, instead returning an error if it wasn't, so that the NFS > > server code can return NFS4ERR_DELAY to the client (similar to > > what e.g. vfs_link does). > > Is there a preexisting bug here? Even without NFS support for xattrs, a > local setxattr on the filesystem should still revoke any delegations > held by remote NFS clients. I couldn't tell whether we're getting that > right from a quick look at the current code. > > --b. I think there's currently a bug if that's the expected behavior, yes. Attribute changes will call notify_change(), and other methods (unlink, link, rename) call try_break_deleg(). But the xattr entry points don't do that, which is why I added it. - Frank
On Fri, Mar 13, 2020 at 04:07:02PM +0000, Frank van der Linden wrote: > On Fri, Mar 13, 2020 at 11:35:49AM -0400, J. Bruce Fields wrote: > > > > > > On Wed, Mar 11, 2020 at 07:59:42PM +0000, Frank van der Linden wrote: > > > Second, RFC 8276 (NFSv4 extended attribute support) specifies that > > > delegations should be recalled (8.4.2.4, 8.4.4.4) when a SETXATTR > > > or REMOVEXATTR operation is performed. So, like with other fs > > > operations, try to break the delegation. The _locked version of > > > these operations will not wait for the delegation to be successfully > > > broken, instead returning an error if it wasn't, so that the NFS > > > server code can return NFS4ERR_DELAY to the client (similar to > > > what e.g. vfs_link does). > > > > Is there a preexisting bug here? Even without NFS support for xattrs, a > > local setxattr on the filesystem should still revoke any delegations > > held by remote NFS clients. I couldn't tell whether we're getting that > > right from a quick look at the current code. > > > > --b. > > I think there's currently a bug if that's the expected behavior, yes. > Attribute changes will call notify_change(), and other methods (unlink, > link, rename) call try_break_deleg(). But the xattr entry points > don't do that, which is why I added it. Got it, thanks. In that case I'd move this patch (or the part of it required to fix that bug) to the front of the series and add a Cc: stable@vger.kernel.org --b.
diff --git a/fs/xattr.c b/fs/xattr.c index 90dd78f0eb27..58013bcbc333 100644 --- a/fs/xattr.c +++ b/fs/xattr.c @@ -204,10 +204,10 @@ int __vfs_setxattr_noperm(struct dentry *dentry, const char *name, return error; } - 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 +216,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); @@ -379,7 +404,8 @@ __vfs_removexattr(struct dentry *dentry, const char *name) EXPORT_SYMBOL(__vfs_removexattr); 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 +414,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 +430,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 6dad031be3c2..3a71ad716da5 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);
To be called from the upcoming NFS server xattr code, the vfs_removexattr and vfs_setxattr need some modifications. First, they need to grow a _locked variant, since the NFS server code will call this with i_rwsem held. It needs to do that in fh_lock to be able to atomically provide the before and after change attributes. Second, RFC 8276 (NFSv4 extended attribute support) specifies that delegations should be recalled (8.4.2.4, 8.4.4.4) when a SETXATTR or REMOVEXATTR operation is performed. So, like with other fs operations, try to break the delegation. The _locked version of these operations will not wait for the delegation to be successfully broken, instead returning an error if it wasn't, so that the NFS server code can return NFS4ERR_DELAY to the client (similar to what e.g. vfs_link does). Signed-off-by: Frank van der Linden <fllinden@amazon.com> --- fs/xattr.c | 63 +++++++++++++++++++++++++++++++++++++++++++++------ include/linux/xattr.h | 2 ++ 2 files changed, 58 insertions(+), 7 deletions(-)