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;
--
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 <[email protected]> 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 <[email protected]> wrote:
>
> On Thu, Jan 2, 2020 at 6:31 PM Frank van der Linden <[email protected]> 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