[PATCH] nfs: only show Posix ACLs in listxattr if actually present

[PATCH] nfs: only show Posix ACLs in listxattr if actually present

[Christoph Hellwig]

The big ACL switched nfs to use generic_listxattr, which calls all existing
->list handlers.  Add a custom .listxattr implementation that only lists
the ACLs if they actually are present on the given inode.

Signed-off-by: Christoph Hellwig <hch@lst.de>
Reported-by: Philippe Troin <phil@fifi.org>
Tested-by: Philippe Troin <phil@fifi.org>
---
 fs/nfs/nfs3acl.c  |   43 +++++++++++++++++++++++++++++++++++++++++++
 fs/nfs/nfs3proc.c |    4 ++--
 2 files changed, 45 insertions(+), 2 deletions(-)

diff --git a/fs/nfs/nfs3acl.c b/fs/nfs/nfs3acl.c
index 871d6ed..8f854dd 100644
--- a/fs/nfs/nfs3acl.c
+++ b/fs/nfs/nfs3acl.c
@@ -247,3 +247,46 @@ const struct xattr_handler *nfs3_xattr_handlers[] = {
 	&posix_acl_default_xattr_handler,
 	NULL,
 };
+
+static int
+nfs3_list_one_acl(struct inode *inode, int type, const char *name, void *data,
+		size_t size, ssize_t *result)
+{
+	struct posix_acl *acl;
+	char *p = data + *result;
+
+	acl = get_acl(inode, type);
+	if (!acl)
+		return 0;
+
+	posix_acl_release(acl);
+
+	*result += strlen(name);
+	*result += 1;
+	if (!size)
+		return 0;
+	if (*result > size)
+		return -ERANGE;
+
+	strcpy(p, name);
+	return 0;
+}
+
+ssize_t
+nfs3_listxattr(struct dentry *dentry, char *data, size_t size)
+{
+	struct inode *inode = dentry->d_inode;
+	ssize_t result = 0;
+	int error;
+
+	error = nfs3_list_one_acl(inode, ACL_TYPE_ACCESS,
+			POSIX_ACL_XATTR_ACCESS, data, size, &result);
+	if (error)
+		return error;
+
+	error = nfs3_list_one_acl(inode, ACL_TYPE_DEFAULT,
+			POSIX_ACL_XATTR_DEFAULT, data, size, &result);
+	if (error)
+		return error;
+	return result;
+}
diff --git a/fs/nfs/nfs3proc.c b/fs/nfs/nfs3proc.c
index e7daa42..f0afa29 100644
--- a/fs/nfs/nfs3proc.c
+++ b/fs/nfs/nfs3proc.c
@@ -885,7 +885,7 @@ static const struct inode_operations nfs3_dir_inode_operations = {
 	.getattr	= nfs_getattr,
 	.setattr	= nfs_setattr,
 #ifdef CONFIG_NFS_V3_ACL
-	.listxattr	= generic_listxattr,
+	.listxattr	= nfs3_listxattr,
 	.getxattr	= generic_getxattr,
 	.setxattr	= generic_setxattr,
 	.removexattr	= generic_removexattr,
@@ -899,7 +899,7 @@ static const struct inode_operations nfs3_file_inode_operations = {
 	.getattr	= nfs_getattr,
 	.setattr	= nfs_setattr,
 #ifdef CONFIG_NFS_V3_ACL
-	.listxattr	= generic_listxattr,
+	.listxattr	= nfs3_listxattr,
 	.getxattr	= generic_getxattr,
 	.setxattr	= generic_setxattr,
 	.removexattr	= generic_removexattr,
-- 
1.7.10.4

Re: [PATCH] nfs: only show Posix ACLs in listxattr if actually present

[Trond Myklebust]

Hi Christoph,

On Wed, Jun 18, 2014 at 5:07 AM, Christoph Hellwig <hch@lst.de> wrote:
> The big ACL switched nfs to use generic_listxattr, which calls all existing
> ->list handlers.  Add a custom .listxattr implementation that only lists
> the ACLs if they actually are present on the given inode.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> Reported-by: Philippe Troin <phil@fifi.org>
> Tested-by: Philippe Troin <phil@fifi.org>
> ---
>  fs/nfs/nfs3acl.c  |   43 +++++++++++++++++++++++++++++++++++++++++++
>  fs/nfs/nfs3proc.c |    4 ++--
>  2 files changed, 45 insertions(+), 2 deletions(-)
>
> diff --git a/fs/nfs/nfs3acl.c b/fs/nfs/nfs3acl.c
> index 871d6ed..8f854dd 100644
> --- a/fs/nfs/nfs3acl.c
> +++ b/fs/nfs/nfs3acl.c
> @@ -247,3 +247,46 @@ const struct xattr_handler *nfs3_xattr_handlers[] = {
>         &posix_acl_default_xattr_handler,
>         NULL,
>  };
> +
> +static int
> +nfs3_list_one_acl(struct inode *inode, int type, const char *name, void *data,
> +               size_t size, ssize_t *result)

Why do you make 'result' a pointer to ssize_t rather than a size_t here?

> +{
> +       struct posix_acl *acl;
> +       char *p = data + *result;
> +
> +       acl = get_acl(inode, type);
> +       if (!acl)
> +               return 0;
> +
> +       posix_acl_release(acl);
> +
> +       *result += strlen(name);
> +       *result += 1;
> +       if (!size)
> +               return 0;
> +       if (*result > size)
> +               return -ERANGE;
> +
> +       strcpy(p, name);
> +       return 0;
> +}
> +
> +ssize_t
> +nfs3_listxattr(struct dentry *dentry, char *data, size_t size)
> +{
> +       struct inode *inode = dentry->d_inode;
> +       ssize_t result = 0;
> +       int error;
> +
> +       error = nfs3_list_one_acl(inode, ACL_TYPE_ACCESS,
> +                       POSIX_ACL_XATTR_ACCESS, data, size, &result);
> +       if (error)
> +               return error;
> +
> +       error = nfs3_list_one_acl(inode, ACL_TYPE_DEFAULT,
> +                       POSIX_ACL_XATTR_DEFAULT, data, size, &result);
> +       if (error)
> +               return error;
> +       return result;
> +}
> diff --git a/fs/nfs/nfs3proc.c b/fs/nfs/nfs3proc.c
> index e7daa42..f0afa29 100644
> --- a/fs/nfs/nfs3proc.c
> +++ b/fs/nfs/nfs3proc.c
> @@ -885,7 +885,7 @@ static const struct inode_operations nfs3_dir_inode_operations = {
>         .getattr        = nfs_getattr,
>         .setattr        = nfs_setattr,
>  #ifdef CONFIG_NFS_V3_ACL
> -       .listxattr      = generic_listxattr,
> +       .listxattr      = nfs3_listxattr,
>         .getxattr       = generic_getxattr,
>         .setxattr       = generic_setxattr,
>         .removexattr    = generic_removexattr,
> @@ -899,7 +899,7 @@ static const struct inode_operations nfs3_file_inode_operations = {
>         .getattr        = nfs_getattr,
>         .setattr        = nfs_setattr,
>  #ifdef CONFIG_NFS_V3_ACL
> -       .listxattr      = generic_listxattr,
> +       .listxattr      = nfs3_listxattr,
>         .getxattr       = generic_getxattr,
>         .setxattr       = generic_setxattr,
>         .removexattr    = generic_removexattr,
> --
> 1.7.10.4
>



-- 
Trond Myklebust

Linux NFS client maintainer, PrimaryData

trond.myklebust@primarydata.com

Re: [PATCH] nfs: only show Posix ACLs in listxattr if actually present

[Christoph Hellwig]

On Wed, Jun 18, 2014 at 07:35:27AM -0400, Trond Myklebust wrote:
> Hi Christoph,
> 
> On Wed, Jun 18, 2014 at 5:07 AM, Christoph Hellwig <hch@lst.de> wrote:
> > The big ACL switched nfs to use generic_listxattr, which calls all existing
> > ->list handlers.  Add a custom .listxattr implementation that only lists
> > the ACLs if they actually are present on the given inode.
> >
> > Signed-off-by: Christoph Hellwig <hch@lst.de>
> > Reported-by: Philippe Troin <phil@fifi.org>
> > Tested-by: Philippe Troin <phil@fifi.org>
> > ---
> >  fs/nfs/nfs3acl.c  |   43 +++++++++++++++++++++++++++++++++++++++++++
> >  fs/nfs/nfs3proc.c |    4 ++--
> >  2 files changed, 45 insertions(+), 2 deletions(-)
> >
> > diff --git a/fs/nfs/nfs3acl.c b/fs/nfs/nfs3acl.c
> > index 871d6ed..8f854dd 100644
> > --- a/fs/nfs/nfs3acl.c
> > +++ b/fs/nfs/nfs3acl.c
> > @@ -247,3 +247,46 @@ const struct xattr_handler *nfs3_xattr_handlers[] = {
> >         &posix_acl_default_xattr_handler,
> >         NULL,
> >  };
> > +
> > +static int
> > +nfs3_list_one_acl(struct inode *inode, int type, const char *name, void *data,
> > +               size_t size, ssize_t *result)
> 
> Why do you make 'result' a pointer to ssize_t rather than a size_t here?

Because ->listxattr returns a ssize_t, and it points to the variable used
as return value of nfs3_listxattr.

Re: [PATCH] nfs: only show Posix ACLs in listxattr if actually present

[Philippe Troin]

Hi Chris & Trond,

On Wed, 2014-06-18 at 15:00 +0200, Christoph Hellwig wrote:
> On Wed, Jun 18, 2014 at 07:35:27AM -0400, Trond Myklebust wrote:
> > Hi Christoph,
> > 
> > On Wed, Jun 18, 2014 at 5:07 AM, Christoph Hellwig <hch@lst.de> wrote:
> > > The big ACL switched nfs to use generic_listxattr, which calls all existing
> > > ->list handlers.  Add a custom .listxattr implementation that only lists
> > > the ACLs if they actually are present on the given inode.
> > >
> > > Signed-off-by: Christoph Hellwig <hch@lst.de>
> > > Reported-by: Philippe Troin <phil@fifi.org>
> > > Tested-by: Philippe Troin <phil@fifi.org>
> > > ---
> > >  fs/nfs/nfs3acl.c  |   43 +++++++++++++++++++++++++++++++++++++++++++
> > >  fs/nfs/nfs3proc.c |    4 ++--
> > >  2 files changed, 45 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/fs/nfs/nfs3acl.c b/fs/nfs/nfs3acl.c
> > > index 871d6ed..8f854dd 100644
> > > --- a/fs/nfs/nfs3acl.c
> > > +++ b/fs/nfs/nfs3acl.c
> > > @@ -247,3 +247,46 @@ const struct xattr_handler *nfs3_xattr_handlers[] = {
> > >         &posix_acl_default_xattr_handler,
> > >         NULL,
> > >  };
> > > +
> > > +static int
> > > +nfs3_list_one_acl(struct inode *inode, int type, const char *name, void *data,
> > > +               size_t size, ssize_t *result)
> > 
> > Why do you make 'result' a pointer to ssize_t rather than a size_t here?
> 
> Because ->listxattr returns a ssize_t, and it points to the variable used
> as return value of nfs3_listxattr.

Have these two patches been merged or at least been queued for inclusion
into mainline?
I have just checked 3.15.3, and the patches do not seem to be included
there.  I haven't tested that specific kernel revision yet though.

Phil.

Re: [PATCH] nfs: only show Posix ACLs in listxattr if actually present

[Trond Myklebust]

Another gmail vs. vger.kernel.org mailing list battle lost. Resending...

On Tue, Jul 8, 2014 at 2:59 PM, Trond Myklebust
<trond.myklebust@primarydata.com> wrote:
>
>
>
> On Tue, Jul 8, 2014 at 11:27 AM, Philippe Troin <phil@fifi.org> wrote:
>>
>> Hi Chris & Trond,
>>
>> On Wed, 2014-06-18 at 15:00 +0200, Christoph Hellwig wrote:
>> > On Wed, Jun 18, 2014 at 07:35:27AM -0400, Trond Myklebust wrote:
>> > > Hi Christoph,
>> > >
>> > > On Wed, Jun 18, 2014 at 5:07 AM, Christoph Hellwig <hch@lst.de> wrote:
>> > > > The big ACL switched nfs to use generic_listxattr, which calls all
>> > > > existing
>> > > > ->list handlers.  Add a custom .listxattr implementation that only
>> > > > lists
>> > > > the ACLs if they actually are present on the given inode.
>> > > >
>> > > > Signed-off-by: Christoph Hellwig <hch@lst.de>
>> > > > Reported-by: Philippe Troin <phil@fifi.org>
>> > > > Tested-by: Philippe Troin <phil@fifi.org>
>> > > > ---
>> > > >  fs/nfs/nfs3acl.c  |   43
>> > > > +++++++++++++++++++++++++++++++++++++++++++
>> > > >  fs/nfs/nfs3proc.c |    4 ++--
>> > > >  2 files changed, 45 insertions(+), 2 deletions(-)
>> > > >
>> > > > diff --git a/fs/nfs/nfs3acl.c b/fs/nfs/nfs3acl.c
>> > > > index 871d6ed..8f854dd 100644
>> > > > --- a/fs/nfs/nfs3acl.c
>> > > > +++ b/fs/nfs/nfs3acl.c
>> > > > @@ -247,3 +247,46 @@ const struct xattr_handler
>> > > > *nfs3_xattr_handlers[] = {
>> > > >         &posix_acl_default_xattr_handler,
>> > > >         NULL,
>> > > >  };
>> > > > +
>> > > > +static int
>> > > > +nfs3_list_one_acl(struct inode *inode, int type, const char *name,
>> > > > void *data,
>> > > > +               size_t size, ssize_t *result)
>> > >
>> > > Why do you make 'result' a pointer to ssize_t rather than a size_t
>> > > here?
>> >
>> > Because ->listxattr returns a ssize_t, and it points to the variable
>> > used
>> > as return value of nfs3_listxattr.
>>
>> Have these two patches been merged or at least been queued for inclusion
>> into mainline?
>
>
> I believe that the final version of Christoph's patch contained both of his
> former patches, so there should be just one to merge, right?
>
>>
>> I have just checked 3.15.3, and the patches do not seem to be included
>> there.  I haven't tested that specific kernel revision yet though.
>
>
> I've applied the patch to my linux-next branch with a Cc: stable >= 3.14.
> I'll push it to Linus once it has soaked a few days.
>
> Cheers
>   Trond
> --
>
> Trond Myklebust
>
> Linux NFS client maintainer, PrimaryData
>
> trond.myklebust@primarydata.com



-- 
Trond Myklebust

Linux NFS client maintainer, PrimaryData

trond.myklebust@primarydata.com