Received-SPF: pass (google.com: best guess record for domain of linux-nfs-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
IronPort-SDR: 7hLUdf6+W/hbVue1v7zts+KaeEl3RciCKVH7wHJXe2d9LhLBjMfm3s1rvgQaCfU13qdg8k3grt
 xMHcqQWATTPg==
Date:   Thu, 2 Jan 2020 23:31:10 +0000
From:   Frank van der Linden <fllinden@amazon.com>
To:     Olga Kornievskaia <aglo@umich.edu>
CC:     Andreas Gruenbacher <agruen@kernel.org>,
        "J. Bruce Fields" <bfields@redhat.com>,
        linux-nfs <linux-nfs@vger.kernel.org>
Subject: Re: extended attributes limitation of xattr_size_max
Message-ID: <20200102233109.GA8735@dev-dsk-fllinden-2c-c1893d73.us-west-2.amazon.com>
References: <CAN-5tyEg2b1CnbJrc-Hf2406cPWCAOjYcpPq0FremYjFXsytDQ@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <CAN-5tyEg2b1CnbJrc-Hf2406cPWCAOjYcpPq0FremYjFXsytDQ@mail.gmail.com>
User-Agent: Mutt/1.11.4 (2019-03-13)
Sender: linux-nfs-owner@vger.kernel.org
Precedence: bulk

On Thu, Jan 02, 2020 at 05:28:44PM -0500, Olga Kornievskaia wrote:
> Hi Andreas and Bruce,
> 
> I thought of you folks as somebody who might have a strong opinion on
> the topic. Right now an NFS client is limited to setting and getting
> ACLs that can't be larger than 64K (XATTR_SIZE_MAX) (where some NFS
> server don't have such limit on the ACL size). There are limits in
> fs/xattr.c during getting and setting xattrs. I believe that's because
> linux local xattr system is limited to that particular constraint.
> However, an NFS acl that uses the xattr interface can be larger than
> that. Is it at all possible to suggest to the larger FS community to
> remove those limits or would that be a non-starter?
> 
> diff --git a/fs/xattr.c b/fs/xattr.c
> index 90dd78f..52a3f91 100644
> --- a/fs/xattr.c
> +++ b/fs/xattr.c
> @@ -428,8 +428,6 @@ int __vfs_setxattr_noperm(struct dentry *dentry,
> const char *name,
>   return error;
> 
>   if (size) {
> - if (size > XATTR_SIZE_MAX)
> - return -E2BIG;
>   kvalue = kvmalloc(size, GFP_KERNEL);
>   if (!kvalue)
>   return -ENOMEM;
> @@ -528,8 +526,6 @@ static int path_setxattr(const char __user *pathname,
>   return error;
> 
>   if (size) {
> - if (size > XATTR_SIZE_MAX)
> - size = XATTR_SIZE_MAX;
>   kvalue = kvzalloc(size, GFP_KERNEL);
>   if (!kvalue)
>   return -ENOMEM;

Aside from not wanting to allocate a raw amount of kernel memory based
on a system call parameter without any checks, I support the idea :-)

The internal xattr interface can be a little awkward to deal with,
the static upper limit being one of the issues that caused me some
problems when implementing user xattrs for NFS.

In general, I would love to see paged-based xattr kernel interfaces,
treating extended attributes as a secondary data stream, which would
make caching fit in a lot more naturally, and means all FS-specific
caching implementations could be removed in favor of a generic one.

One issue right now is that, an xattr not being a 'stream', a lot
of FS code caches the entire value in kmalloc-ed space, which becomes
unwieldy if the XATTR_SIZE_MAX limit is removed.

In other words, I think removing the limit won't work that well with
the current implementation, but I hope that the implementation can
be changed so that the limit can be removed.

- Frank