Re: extended attributes limitation of xattr_size_max

[Olga Kornievskaia <aglo@umich.edu>]

On Fri, Jan 3, 2020 at 11:13 AM Olga Kornievskaia <aglo@umich.edu> wrote:
>
> 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.

I guess I found an answer to my own question in
https://www.spinics.net/lists/linux-fsdevel/msg85580.html

"> Can we get rid of the 64k size limit for EAs? The API on AIX is the
same as on
> Linux.  But there is a huge size limit - which would help us already a lot.

No - the maximum xattr size of 64k is encoded into the on-disk
format of many filesystems and that's not a simple thing to change."


>
> >
> > - Frank