| Message ID | 20200623223927.31795-2-fllinden@amazon.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3,01/10] xattr: break delegations in {set,remove}xattr | expand |
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@amazon.com/ 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@vger.kernel.org > Cc: linux-fsdevel@vger.kernel.org > Cc: Al Viro <viro@zeniv.linux.org.uk> > Signed-off-by: Frank van der Linden <fllinden@amazon.com> > --- > 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 >
Hi
[This is an automated email]
This commit has been processed because it contains a -stable tag.
The stable tag indicates that it's relevant for the following trees: all
The bot has tested the following trees: v5.7.6, v5.4.49, v4.19.130, v4.14.186, v4.9.228, v4.4.228.
v5.7.6: Build OK!
v5.4.49: Build OK!
v4.19.130: Build OK!
v4.14.186: Build OK!
v4.9.228: Build OK!
v4.4.228: Failed to apply! Possible dependencies:
5d6c31910bc07 ("xattr: Add __vfs_{get,set,remove}xattr helpers")
6b2553918d8b4 ("replace ->follow_link() with new method that could stay in RCU mode")
aa80deab33a8f ("namei: page_getlink() and page_follow_link_light() are the same thing")
cd3417c8fc950 ("kill free_page_put_link()")
ce23e64013348 ("->getxattr(): pass dentry and inode as separate arguments")
fceef393a5381 ("switch ->get_link() to delayed_call, kill ->put_link()")
NOTE: The patch will not be queued to stable trees until it is upstream.
How should we proceed with this patch?
Hi
[This is an automated email]
This commit has been processed because it contains a -stable tag.
The stable tag indicates that it's relevant for the following trees: all
The bot has tested the following trees: v5.7.6, v5.4.49, v4.19.130, v4.14.186, v4.9.228, v4.4.228.
v5.7.6: Build OK!
v5.4.49: Build OK!
v4.19.130: Build OK!
v4.14.186: Build OK!
v4.9.228: Build OK!
v4.4.228: Failed to apply! Possible dependencies:
5d6c31910bc07 ("xattr: Add __vfs_{get,set,remove}xattr helpers")
6b2553918d8b4 ("replace ->follow_link() with new method that could stay in RCU mode")
aa80deab33a8f ("namei: page_getlink() and page_follow_link_light() are the same thing")
cd3417c8fc950 ("kill free_page_put_link()")
ce23e64013348 ("->getxattr(): pass dentry and inode as separate arguments")
fceef393a5381 ("switch ->get_link() to delayed_call, kill ->put_link()")
NOTE: The patch will not be queued to stable trees until it is upstream.
How should we proceed with this patch?
Hi Sasha- > On Jul 10, 2020, at 10:03 AM, Sasha Levin <sashal@kernel.org> wrote: > > Hi > > [This is an automated email] > > This commit has been processed because it contains a -stable tag. > The stable tag indicates that it's relevant for the following trees: all > > The bot has tested the following trees: v5.7.6, v5.4.49, v4.19.130, v4.14.186, v4.9.228, v4.4.228. > > v5.7.6: Build OK! > v5.4.49: Build OK! > v4.19.130: Build OK! > v4.14.186: Build OK! > v4.9.228: Build OK! > v4.4.228: Failed to apply! Possible dependencies: > 5d6c31910bc07 ("xattr: Add __vfs_{get,set,remove}xattr helpers") > 6b2553918d8b4 ("replace ->follow_link() with new method that could stay in RCU mode") > aa80deab33a8f ("namei: page_getlink() and page_follow_link_light() are the same thing") > cd3417c8fc950 ("kill free_page_put_link()") > ce23e64013348 ("->getxattr(): pass dentry and inode as separate arguments") > fceef393a5381 ("switch ->get_link() to delayed_call, kill ->put_link()") > > > NOTE: The patch will not be queued to stable trees until it is upstream. > > How should we proceed with this patch? I've updated the "cc: stable" tag in my testing branch to include "# v4.9+". -- Chuck Lever
On Thu, Jun 25, 2020 at 08:39:54PM +0000, Frank van der Linden wrote: > 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@amazon.com/ > > 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 Hi Al, Any comments on this one? It's a simple change to break (NFSv4) delegations on set/removexattr, so it's pretty nfsd specific. Linus - since this is nfsd specific, could this go in through the nfsd maintainer tree directly? Chuck has included it in a tree that is being readied for 5.9. 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@vger.kernel.org > > Cc: linux-fsdevel@vger.kernel.org > > Cc: Al Viro <viro@zeniv.linux.org.uk> > > Signed-off-by: Frank van der Linden <fllinden@amazon.com> > > --- > > 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 > >
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);
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@vger.kernel.org Cc: linux-fsdevel@vger.kernel.org Cc: Al Viro <viro@zeniv.linux.org.uk> Signed-off-by: Frank van der Linden <fllinden@amazon.com> --- fs/xattr.c | 84 +++++++++++++++++++++++++++++++++++++++---- include/linux/xattr.h | 2 ++ 2 files changed, 79 insertions(+), 7 deletions(-)