Here's a pair of fixes for AFS.
(1) Fix an oops in AFS that can be triggered by accessing one of the
afs.yfs.* xattrs against a yfs server[1][2] - for instance by "cp -a"
or "rsync -X". These try and copy all of the xattrs.
They should pay attention to the list in /etc/xattr.conf, but cp
doesn't on Ubuntu and rsync doesn't seem to on Ubuntu or Fedora.
xattr.conf has been modified upstream[3], but a new version hasn't
been cut yet. I've logged a bug against rsync for the problem
there[4].
(2) Hide ACL-related AFS xattrs[6]. This removes them from the list
returned by listxattr(), but they're still available to get/set.
With further regard to the second patch, I tried just hiding the
appropriate ACL-related xattrs[5] first, but it didn't work well,
especially when a volume is replicated across servers of different types.
I wonder if it's better to just hide all the afs.* xattrs from listxattr().
It would probably be even better to not use xattrs for this, but I'm not
sure what I would use instead.
The patches can be found here:
https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git/log/?h=afs-fixes
David
Link: http://lists.infradead.org/pipermail/linux-afs/2021-March/003498.html [1]
Link: http://lists.infradead.org/pipermail/linux-afs/2021-March/003501.html [2]
Link: https://git.savannah.nongnu.org/cgit/attr.git/commit/?id=74da517cc655a82ded715dea7245ce88ebc91b98 [3]
Link: https://github.com/WayneD/rsync/issues/163 [4]
Link: http://lists.infradead.org/pipermail/linux-afs/2021-March/003516.html [5]
Link: http://lists.infradead.org/pipermail/linux-afs/2021-March/003524.html [6]
---
David Howells (2):
afs: Fix accessing YFS xattrs on a non-YFS server
afs: Fix afs_listxattr() to not list afs ACL special xattrs
fs/afs/xattr.c | 7 +------
1 file changed, 1 insertion(+), 6 deletions(-)
If someone attempts to access YFS-related xattrs (e.g. afs.yfs.acl) on a
file on a non-YFS AFS server (such as OpenAFS), then the kernel will jump
to a NULL function pointer because the afs_fetch_acl_operation descriptor
doesn't point to a function for issuing an operation on a non-YFS
server[1].
Fix this by making afs_wait_for_operation() check that the issue_afs_rpc
method is set before jumping to it and setting -ENOTSUPP if not. This fix
also covers other potential operations that also only exist on YFS servers.
afs_xattr_get/set_yfs() then need to translate -ENOTSUPP to -ENODATA as the
former error is internal to the kernel.
The bug shows up as an oops like the following:
BUG: kernel NULL pointer dereference, address: 0000000000000000
[...]
Code: Unable to access opcode bytes at RIP 0xffffffffffffffd6.
[...]
Call Trace:
afs_wait_for_operation+0x83/0x1b0 [kafs]
afs_xattr_get_yfs+0xe6/0x270 [kafs]
__vfs_getxattr+0x59/0x80
vfs_getxattr+0x11c/0x140
getxattr+0x181/0x250
? __check_object_size+0x13f/0x150
? __fput+0x16d/0x250
__x64_sys_fgetxattr+0x64/0xb0
do_syscall_64+0x49/0xc0
entry_SYSCALL_64_after_hwframe+0x44/0xa9
RIP: 0033:0x7fb120a9defe
This was triggered with "cp -a" which attempts to copy xattrs, including
afs ones, but is easier to reproduce with getfattr, e.g.:
getfattr -d -m ".*" /afs/openafs.org/
Fixes: e49c7b2f6de7 ("afs: Build an abstraction around an "operation" concept")
Reported-by: Gaja Sophie Peters <[email protected]>
Signed-off-by: David Howells <[email protected]>
Tested-by: Gaja Sophie Peters <[email protected]>
cc: [email protected]
Link: http://lists.infradead.org/pipermail/linux-afs/2021-March/003498.html [1]
---
fs/afs/fs_operation.c | 7 +++++--
fs/afs/xattr.c | 8 +++++++-
2 files changed, 12 insertions(+), 3 deletions(-)
diff --git a/fs/afs/fs_operation.c b/fs/afs/fs_operation.c
index 97cab12b0a6c..71c58723763d 100644
--- a/fs/afs/fs_operation.c
+++ b/fs/afs/fs_operation.c
@@ -181,10 +181,13 @@ void afs_wait_for_operation(struct afs_operation *op)
if (test_bit(AFS_SERVER_FL_IS_YFS, &op->server->flags) &&
op->ops->issue_yfs_rpc)
op->ops->issue_yfs_rpc(op);
- else
+ else if (op->ops->issue_afs_rpc)
op->ops->issue_afs_rpc(op);
+ else
+ op->ac.error = -ENOTSUPP;
- op->error = afs_wait_for_call_to_complete(op->call, &op->ac);
+ if (op->call)
+ op->error = afs_wait_for_call_to_complete(op->call, &op->ac);
}
switch (op->error) {
diff --git a/fs/afs/xattr.c b/fs/afs/xattr.c
index c629caae5002..4934e325a14a 100644
--- a/fs/afs/xattr.c
+++ b/fs/afs/xattr.c
@@ -231,6 +231,8 @@ static int afs_xattr_get_yfs(const struct xattr_handler *handler,
else
ret = -ERANGE;
}
+ } else if (ret == -ENOTSUPP) {
+ ret = -ENODATA;
}
error_yacl:
@@ -256,6 +258,7 @@ static int afs_xattr_set_yfs(const struct xattr_handler *handler,
{
struct afs_operation *op;
struct afs_vnode *vnode = AFS_FS_I(inode);
+ int ret;
if (flags == XATTR_CREATE ||
strcmp(name, "acl") != 0)
@@ -270,7 +273,10 @@ static int afs_xattr_set_yfs(const struct xattr_handler *handler,
return afs_put_operation(op);
op->ops = &yfs_store_opaque_acl2_operation;
- return afs_do_sync_operation(op);
+ ret = afs_do_sync_operation(op);
+ if (ret == -ENOTSUPP)
+ ret = -ENODATA;
+ return ret;
}
static const struct xattr_handler afs_xattr_yfs_handler = {
afs_listxattr() lists all the available special afs xattrs (i.e. those in
the "afs.*" space), no matter what type of server we're dealing with. But
OpenAFS servers, for example, cannot deal with some of the extra-capable
attributes that AuriStor (YFS) servers provide. Unfortunately, the
presence of the afs.yfs.* attributes causes errors[1] for anything that
tries to read them if the server is of the wrong type.
Fix afs_listxattr() so that it doesn't list any of the AFS ACL xattrs. It
does mean, however, that getfattr won't list them, though they can still be
accessed with getxattr() and setxattr().
This can be tested with something like:
getfattr -d -m ".*" /afs/example.com/path/to/file
With this change, none of the afs.* ACL attributes should be visible.
Fixes: ae46578b963f ("afs: Get YFS ACLs and information through xattrs")
Reported-by: Gaja Sophie Peters <[email protected]>
Signed-off-by: David Howells <[email protected]>
Tested-by: Gaja Sophie Peters <[email protected]>
cc: [email protected]
Link: http://lists.infradead.org/pipermail/linux-afs/2021-March/003502.html [1]
---
fs/afs/xattr.c | 7 +------
1 file changed, 1 insertion(+), 6 deletions(-)
diff --git a/fs/afs/xattr.c b/fs/afs/xattr.c
index 4934e325a14a..81a6aec764cc 100644
--- a/fs/afs/xattr.c
+++ b/fs/afs/xattr.c
@@ -12,14 +12,9 @@
#include "internal.h"
static const char afs_xattr_list[] =
- "afs.acl\0"
"afs.cell\0"
"afs.fid\0"
- "afs.volume\0"
- "afs.yfs.acl\0"
- "afs.yfs.acl_inherited\0"
- "afs.yfs.acl_num_cleaned\0"
- "afs.yfs.vol_acl";
+ "afs.volume";
/*
* Retrieve a list of the supported xattrs.
On 3/11/2021 9:10 AM, David Howells ([email protected]) wrote:
> I wonder if it's better to just hide all the afs.* xattrs from listxattr().
> It would probably be even better to not use xattrs for this, but I'm not
> sure what I would use instead.
I believe that all of the "afs.*" xattrs should be hidden from
listxattr(). Any "afs.*" xattr that is read from an afs inode can be
copied to another filesystem and stored. Attempts to set these values
in an afs volume will fail.
The use of xattrs is intended to be an alternative to the IBM/OpenAFS
pioctls which are accessed by processes such as "fs", "aklog", "tokens",
etc which would know the names of the xattrs and how to use them. Such
tools would not require listxattr() to find them.
Thanks for the consideration.
Jeffrey Altman
On Thu, Mar 11, 2021 at 10:10 AM David Howells <[email protected]> wrote:
>
> If someone attempts to access YFS-related xattrs (e.g. afs.yfs.acl) on a
> file on a non-YFS AFS server (such as OpenAFS), then the kernel will jump
> to a NULL function pointer because the afs_fetch_acl_operation descriptor
> doesn't point to a function for issuing an operation on a non-YFS
> server[1].
>
> Fix this by making afs_wait_for_operation() check that the issue_afs_rpc
> method is set before jumping to it and setting -ENOTSUPP if not. This fix
> also covers other potential operations that also only exist on YFS servers.
>
> afs_xattr_get/set_yfs() then need to translate -ENOTSUPP to -ENODATA as the
> former error is internal to the kernel.
>
> The bug shows up as an oops like the following:
>
> BUG: kernel NULL pointer dereference, address: 0000000000000000
> [...]
> Code: Unable to access opcode bytes at RIP 0xffffffffffffffd6.
> [...]
> Call Trace:
> afs_wait_for_operation+0x83/0x1b0 [kafs]
> afs_xattr_get_yfs+0xe6/0x270 [kafs]
> __vfs_getxattr+0x59/0x80
> vfs_getxattr+0x11c/0x140
> getxattr+0x181/0x250
> ? __check_object_size+0x13f/0x150
> ? __fput+0x16d/0x250
> __x64_sys_fgetxattr+0x64/0xb0
> do_syscall_64+0x49/0xc0
> entry_SYSCALL_64_after_hwframe+0x44/0xa9
> RIP: 0033:0x7fb120a9defe
>
> This was triggered with "cp -a" which attempts to copy xattrs, including
> afs ones, but is easier to reproduce with getfattr, e.g.:
>
> getfattr -d -m ".*" /afs/openafs.org/
>
> Fixes: e49c7b2f6de7 ("afs: Build an abstraction around an "operation" concept")
> Reported-by: Gaja Sophie Peters <[email protected]>
> Signed-off-by: David Howells <[email protected]>
> Tested-by: Gaja Sophie Peters <[email protected]>
> cc: [email protected]
> Link: http://lists.infradead.org/pipermail/linux-afs/2021-March/003498.html [1]
> ---
>
> fs/afs/fs_operation.c | 7 +++++--
> fs/afs/xattr.c | 8 +++++++-
> 2 files changed, 12 insertions(+), 3 deletions(-)
>
> diff --git a/fs/afs/fs_operation.c b/fs/afs/fs_operation.c
> index 97cab12b0a6c..71c58723763d 100644
> --- a/fs/afs/fs_operation.c
> +++ b/fs/afs/fs_operation.c
> @@ -181,10 +181,13 @@ void afs_wait_for_operation(struct afs_operation *op)
> if (test_bit(AFS_SERVER_FL_IS_YFS, &op->server->flags) &&
> op->ops->issue_yfs_rpc)
> op->ops->issue_yfs_rpc(op);
> - else
> + else if (op->ops->issue_afs_rpc)
> op->ops->issue_afs_rpc(op);
> + else
> + op->ac.error = -ENOTSUPP;
>
> - op->error = afs_wait_for_call_to_complete(op->call, &op->ac);
> + if (op->call)
> + op->error = afs_wait_for_call_to_complete(op->call, &op->ac);
> }
>
> switch (op->error) {
> diff --git a/fs/afs/xattr.c b/fs/afs/xattr.c
> index c629caae5002..4934e325a14a 100644
> --- a/fs/afs/xattr.c
> +++ b/fs/afs/xattr.c
> @@ -231,6 +231,8 @@ static int afs_xattr_get_yfs(const struct xattr_handler *handler,
> else
> ret = -ERANGE;
> }
> + } else if (ret == -ENOTSUPP) {
> + ret = -ENODATA;
> }
>
> error_yacl:
> @@ -256,6 +258,7 @@ static int afs_xattr_set_yfs(const struct xattr_handler *handler,
> {
> struct afs_operation *op;
> struct afs_vnode *vnode = AFS_FS_I(inode);
> + int ret;
>
> if (flags == XATTR_CREATE ||
> strcmp(name, "acl") != 0)
> @@ -270,7 +273,10 @@ static int afs_xattr_set_yfs(const struct xattr_handler *handler,
> return afs_put_operation(op);
>
> op->ops = &yfs_store_opaque_acl2_operation;
> - return afs_do_sync_operation(op);
> + ret = afs_do_sync_operation(op);
> + if (ret == -ENOTSUPP)
> + ret = -ENODATA;
> + return ret;
> }
>
> static const struct xattr_handler afs_xattr_yfs_handler = {
Reviewed-by: Marc Dionne <[email protected]>
Marc