| Message ID | CAN-5tyEg2b1CnbJrc-Hf2406cPWCAOjYcpPq0FremYjFXsytDQ@mail.gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | extended attributes limitation of xattr_size_max | expand |
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
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
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
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)
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? 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; --