[PATCH 0/2] AFS metadata xattr fixes

[PATCH 0/2] AFS metadata xattr fixes

[David Howells]

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(-)

[PATCH 1/2] afs: Fix accessing YFS xattrs on a non-YFS server

[David Howells]

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 <gaja.peters@math.uni-hamburg.de>
Signed-off-by: David Howells <dhowells@redhat.com>
Tested-by: Gaja Sophie Peters <gaja.peters@math.uni-hamburg.de>
cc: linux-afs@lists.infradead.org
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 = {

[PATCH 2/2] afs: Fix afs_listxattr() to not list afs ACL special xattrs

[David Howells]

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 <gaja.peters@math.uni-hamburg.de>
Signed-off-by: David Howells <dhowells@redhat.com>
Tested-by: Gaja Sophie Peters <gaja.peters@math.uni-hamburg.de>
cc: linux-afs@lists.infradead.org
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.

Re: [PATCH 0/2] AFS metadata xattr fixes

[Jeffrey E Altman]

[-- Attachment #1.1: Type: text/plain, Size: 826 bytes --]

On 3/11/2021 9:10 AM, David Howells (dhowells@redhat.com) 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



[-- Attachment #1.2: jaltman.vcf --]
[-- Type: text/x-vcard, Size: 283 bytes --]

begin:vcard
fn:Jeffrey Altman
n:Altman;Jeffrey
org:AuriStor, Inc.
adr:;;255 W 94TH ST STE 6B;New York;NY;10025-6985;United States
email;internet:jaltman@auristor.com
title:CEO
tel;work:+1-212-769-9018
url:https://www.linkedin.com/in/jeffreyaltman/
version:2.1
end:vcard


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4033 bytes --]

Re: [PATCH 1/2] afs: Fix accessing YFS xattrs on a non-YFS server

[Marc Dionne]

On Thu, Mar 11, 2021 at 10:10 AM David Howells <dhowells@redhat.com> 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 <gaja.peters@math.uni-hamburg.de>
> Signed-off-by: David Howells <dhowells@redhat.com>
> Tested-by: Gaja Sophie Peters <gaja.peters@math.uni-hamburg.de>
> cc: linux-afs@lists.infradead.org
> 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 <marc.dionne@auristor.com>

Marc

Re: [PATCH 2/2] afs: Fix afs_listxattr() to not list afs ACL special xattrs

[Marc Dionne]

On Thu, Mar 11, 2021 at 10:10 AM David Howells <dhowells@redhat.com> wrote:
>
> 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 <gaja.peters@math.uni-hamburg.de>
> Signed-off-by: David Howells <dhowells@redhat.com>
> Tested-by: Gaja Sophie Peters <gaja.peters@math.uni-hamburg.de>
> cc: linux-afs@lists.infradead.org
> 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.

As Jeffrey suggested, you may want to consider not exposing any of
them, but this seems fine to me.

Besides the reasons you mention, acls getting copied blindly is not a
good idea.  The source and target may be in different cells where
users and groups mean different things.  For files, it may switch them
from inheriting permissions from their parent to having their own ACL,
etc.

Reviewed-by: Marc Dionne <marc.dionne@auristor.com>

Marc