Re: [PATCH v2 2/2] afs: Stop listxattr() from listing "afs.*" attributes

From: Marc Dionne <marc.c.dionne@gmail.com>
To: David Howells <dhowells@redhat.com>
Cc: linux-afs@lists.infradead.org,
	Gaja Sophie Peters <gaja.peters@math.uni-hamburg.de>,
	linux-fsdevel@vger.kernel.org,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v2 2/2] afs: Stop listxattr() from listing "afs.*" attributes
Date: Mon, 15 Mar 2021 14:02:22 -0300	[thread overview]
Message-ID: <CAB9dFdsTxs4NxprGG2vaj1pGE26qtZO7s2v+D5Gf=27op_94oQ@mail.gmail.com> (raw)
In-Reply-To: <161550399833.1983424.16644306048746346626.stgit@warthog.procyon.org.uk>

On Thu, Mar 11, 2021 at 7:07 PM 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 the problem by removing afs_listxattr() so that none of the special
> xattrs are listed (AFS doesn't support 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.* attributes should be visible.
>
> Changes:
> ver #2:
>  - Hide all of the afs.* xattrs, not just the ACL ones.
>
> 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>
> cc: linux-afs@lists.infradead.org
> Link: http://lists.infradead.org/pipermail/linux-afs/2021-March/003502.html [1]
> Link: http://lists.infradead.org/pipermail/linux-afs/2021-March/003567.html # v1
> ---
>
>  fs/afs/dir.c      |    1 -
>  fs/afs/file.c     |    1 -
>  fs/afs/inode.c    |    1 -
>  fs/afs/internal.h |    1 -
>  fs/afs/mntpt.c    |    1 -
>  fs/afs/xattr.c    |   23 -----------------------
>  6 files changed, 28 deletions(-)
>
> diff --git a/fs/afs/dir.c b/fs/afs/dir.c
> index 714fcca9af99..17548c1faf02 100644
> --- a/fs/afs/dir.c
> +++ b/fs/afs/dir.c
> @@ -70,7 +70,6 @@ const struct inode_operations afs_dir_inode_operations = {
>         .permission     = afs_permission,
>         .getattr        = afs_getattr,
>         .setattr        = afs_setattr,
> -       .listxattr      = afs_listxattr,
>  };
>
>  const struct address_space_operations afs_dir_aops = {
> diff --git a/fs/afs/file.c b/fs/afs/file.c
> index 85f5adf21aa0..960b64268623 100644
> --- a/fs/afs/file.c
> +++ b/fs/afs/file.c
> @@ -43,7 +43,6 @@ const struct inode_operations afs_file_inode_operations = {
>         .getattr        = afs_getattr,
>         .setattr        = afs_setattr,
>         .permission     = afs_permission,
> -       .listxattr      = afs_listxattr,
>  };
>
>  const struct address_space_operations afs_fs_aops = {
> diff --git a/fs/afs/inode.c b/fs/afs/inode.c
> index 1156b2df28d3..12be88716e4c 100644
> --- a/fs/afs/inode.c
> +++ b/fs/afs/inode.c
> @@ -27,7 +27,6 @@
>
>  static const struct inode_operations afs_symlink_inode_operations = {
>         .get_link       = page_get_link,
> -       .listxattr      = afs_listxattr,
>  };
>
>  static noinline void dump_vnode(struct afs_vnode *vnode, struct afs_vnode *parent_vnode)
> diff --git a/fs/afs/internal.h b/fs/afs/internal.h
> index b626e38e9ab5..1627b1872812 100644
> --- a/fs/afs/internal.h
> +++ b/fs/afs/internal.h
> @@ -1509,7 +1509,6 @@ extern int afs_launder_page(struct page *);
>   * xattr.c
>   */
>  extern const struct xattr_handler *afs_xattr_handlers[];
> -extern ssize_t afs_listxattr(struct dentry *, char *, size_t);
>
>  /*
>   * yfsclient.c
> diff --git a/fs/afs/mntpt.c b/fs/afs/mntpt.c
> index 052dab2f5c03..bbb2c210d139 100644
> --- a/fs/afs/mntpt.c
> +++ b/fs/afs/mntpt.c
> @@ -32,7 +32,6 @@ const struct inode_operations afs_mntpt_inode_operations = {
>         .lookup         = afs_mntpt_lookup,
>         .readlink       = page_readlink,
>         .getattr        = afs_getattr,
> -       .listxattr      = afs_listxattr,
>  };
>
>  const struct inode_operations afs_autocell_inode_operations = {
> diff --git a/fs/afs/xattr.c b/fs/afs/xattr.c
> index 4934e325a14a..7751b0b3f81d 100644
> --- a/fs/afs/xattr.c
> +++ b/fs/afs/xattr.c
> @@ -11,29 +11,6 @@
>  #include <linux/xattr.h>
>  #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";
> -
> -/*
> - * Retrieve a list of the supported xattrs.
> - */
> -ssize_t afs_listxattr(struct dentry *dentry, char *buffer, size_t size)
> -{
> -       if (size == 0)
> -               return sizeof(afs_xattr_list);
> -       if (size < sizeof(afs_xattr_list))
> -               return -ERANGE;
> -       memcpy(buffer, afs_xattr_list, sizeof(afs_xattr_list));
> -       return sizeof(afs_xattr_list);
> -}
> -
>  /*
>   * Deal with the result of a successful fetch ACL operation.
>   */

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

Marc