Re: extended attributes limitation of xattr_size_max

[Olga Kornievskaia <aglo@umich.edu>]

On Thu, Jan 2, 2020 at 6:31 PM Frank van der Linden <fllinden@amazon.com> wrote:
>
> 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.

Hi Frank,

Thank you for your feedback. You are right, unchecked memory
allocation in the kernel would not be a good idea. Your suggested
approach of page-based xattr seems like a good idea but not something
I feel like I can take on right now. I wonder if we can lobby for the
xattr limit to be increased to 1M. That would serve NFS needs as right
now rpc calls (and thus getattr) are no larger than 1M. Thoughts on
that? I'm not familiar with generic xattr usage and I wonder if even
local usage could benefit from having a larger limit.

>
> - Frank