[PATCH] nfs: handle servers that support either ALLOW or DENY ACE types.

[PATCH] nfs: handle servers that support either ALLOW or DENY ACE types.

[Malahal Naineni]

Currently we support ACLs if the NFS server file system supports
ALLOW and DENY ACE types. This patch makes the Linux client work with
ACLs if the server supports either ALLOW or DENY ACE types.

Signed-off-by: Malahal Naineni <malahal@us.ibm.com>
---
 fs/nfs/nfs4proc.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 15052b8..4504685 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -4321,9 +4321,9 @@ static int nfs4_proc_renew(struct nfs_client *clp, struct rpc_cred *cred)
 
 static inline int nfs4_server_supports_acls(struct nfs_server *server)
 {
-	return (server->caps & NFS_CAP_ACLS)
-		&& (server->acl_bitmask & ACL4_SUPPORT_ALLOW_ACL)
-		&& (server->acl_bitmask & ACL4_SUPPORT_DENY_ACL);
+	return server->caps & NFS_CAP_ACLS &&
+			(server->acl_bitmask & ACL4_SUPPORT_ALLOW_ACL ||
+			 server->acl_bitmask & ACL4_SUPPORT_DENY_ACL);
 }
 
 /* Assuming that XATTR_SIZE_MAX is a multiple of PAGE_SIZE, and that
-- 
1.8.3.1

Re: [PATCH] nfs: handle servers that support either ALLOW or DENY ACE types.

[Trond Myklebust]

On Jan 23, 2014, at 20:50, Malahal Naineni <malahal@us.ibm.com> wrote:

> Currently we support ACLs if the NFS server file system supports
> ALLOW and DENY ACE types. This patch makes the Linux client work with
> ACLs if the server supports either ALLOW or DENY ACE types.

According to RFC5661, the behaviour if you don’t have ALLOW aces is to deny all access. How does it make sense to accept that?

--
Trond Myklebust
Linux NFS client maintainer

Re: [PATCH] nfs: handle servers that support either ALLOW or DENY ACE types.

[Malahal Naineni]

Trond Myklebust [trond.myklebust@primarydata.com] wrote:
> 
> On Jan 23, 2014, at 20:50, Malahal Naineni <malahal@us.ibm.com> wrote:
> 
> > Currently we support ACLs if the NFS server file system supports
> > ALLOW and DENY ACE types. This patch makes the Linux client work with
> > ACLs if the server supports either ALLOW or DENY ACE types.
> 
> According to RFC5661, the behaviour if you don’t have ALLOW aces is to deny all access. How does it make sense to accept that?

I have a server that only returned 'ALLOW' type support probably due to
a bug!  There is nothing in the spec that said a server 'MUST' support
'ALLOW' and 'DENY' ACE types (RFC5661 does say 'SHOULD' though!). That
was my reasoning to fix the client to be more liberal/lenient.

Can a server implicitly construct 'ALLOW' ACEs based on mode and not
support explicitly setting such ACEs by a client? I am not too familiar
with ACLs, if you think we should only check for 'ALLOW' support flag, I
can re-spin the patch but I think it is better to be more lenient
specially if it is not incorrect by being more lenient!

Please let me know either way.

Regards, Malahal.

Re: [PATCH] nfs: handle servers that support either ALLOW or DENY ACE types.

[Trond Myklebust]

On Jan 24, 2014, at 7:28, Malahal Naineni <malahal@us.ibm.com> wrote:

> Trond Myklebust [trond.myklebust@primarydata.com] wrote:
>> 
>> On Jan 23, 2014, at 20:50, Malahal Naineni <malahal@us.ibm.com> wrote:
>> 
>>> Currently we support ACLs if the NFS server file system supports
>>> ALLOW and DENY ACE types. This patch makes the Linux client work with
>>> ACLs if the server supports either ALLOW or DENY ACE types.
>> 
>> According to RFC5661, the behaviour if you don’t have ALLOW aces is to deny all access. How does it make sense to accept that?
> 
> I have a server that only returned 'ALLOW' type support probably due to
> a bug!  There is nothing in the spec that said a server 'MUST' support
> 'ALLOW' and 'DENY' ACE types (RFC5661 does say 'SHOULD' though!). That
> was my reasoning to fix the client to be more liberal/lenient.
> 
> Can a server implicitly construct 'ALLOW' ACEs based on mode and not
> support explicitly setting such ACEs by a client? I am not too familiar
> with ACLs, if you think we should only check for 'ALLOW' support flag, I
> can re-spin the patch but I think it is better to be more lenient
> specially if it is not incorrect by being more lenient!

The way I read the spec, the default behaviour is to disallow access unless you have sufficient ALLOW aces for the behaviour that you want. The DENY aces are optional, and are there in order to explicitly deny a user or group a particular behaviour that they would otherwise be allowed due to some subsequent combination of ALLOW aces.

So, I accept that a server could function while only supporting ALLOW aces, but I don’t see how it could work at all with only DENY ace support.

Cheers
  Trond
--
Trond Myklebust
Linux NFS client maintainer

Re: [PATCH] nfs: handle servers that support either ALLOW or DENY ACE types.

[Malahal Naineni]

Trond Myklebust [trond.myklebust@primarydata.com] wrote:
> 
> On Jan 24, 2014, at 7:28, Malahal Naineni <malahal@us.ibm.com> wrote:
> 
> > Trond Myklebust [trond.myklebust@primarydata.com] wrote:
> >> 
> >> On Jan 23, 2014, at 20:50, Malahal Naineni <malahal@us.ibm.com> wrote:
> >> 
> >>> Currently we support ACLs if the NFS server file system supports
> >>> ALLOW and DENY ACE types. This patch makes the Linux client work with
> >>> ACLs if the server supports either ALLOW or DENY ACE types.
> >> 
> >> According to RFC5661, the behaviour if you don’t have ALLOW aces is to deny all access. How does it make sense to accept that?
> > 
> > I have a server that only returned 'ALLOW' type support probably due to
> > a bug!  There is nothing in the spec that said a server 'MUST' support
> > 'ALLOW' and 'DENY' ACE types (RFC5661 does say 'SHOULD' though!). That
> > was my reasoning to fix the client to be more liberal/lenient.
> > 
> > Can a server implicitly construct 'ALLOW' ACEs based on mode and not
> > support explicitly setting such ACEs by a client? I am not too familiar
> > with ACLs, if you think we should only check for 'ALLOW' support flag, I
> > can re-spin the patch but I think it is better to be more lenient
> > specially if it is not incorrect by being more lenient!
> 
> The way I read the spec, the default behaviour is to disallow access unless you have sufficient ALLOW aces for the behaviour that you want. The DENY aces are optional, and are there in order to explicitly deny a user or group a particular behaviour that they would otherwise be allowed due to some subsequent combination of ALLOW aces.
> 
> So, I accept that a server could function while only supporting ALLOW aces, but I don’t see how it could work at all with only DENY ace support.

Fair enough!

[PATCH] nfs: handle servers that support only ALLOW ACE type.

[Malahal Naineni]

Currently we support ACLs if the NFS server file system supports both
ALLOW and DENY ACE types. This patch makes the Linux client work with
ACLs even if the server supports only 'ALLOW' ACE type.

Signed-off-by: Malahal Naineni <malahal@us.ibm.com>
---
 fs/nfs/nfs4proc.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 15052b8..e3b8fa6 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -4321,9 +4321,8 @@ static int nfs4_proc_renew(struct nfs_client *clp, struct rpc_cred *cred)
 
 static inline int nfs4_server_supports_acls(struct nfs_server *server)
 {
-	return (server->caps & NFS_CAP_ACLS)
-		&& (server->acl_bitmask & ACL4_SUPPORT_ALLOW_ACL)
-		&& (server->acl_bitmask & ACL4_SUPPORT_DENY_ACL);
+	return server->caps & NFS_CAP_ACLS &&
+		server->acl_bitmask & ACL4_SUPPORT_ALLOW_ACL;
 }
 
 /* Assuming that XATTR_SIZE_MAX is a multiple of PAGE_SIZE, and that
-- 
1.8.3.1

Re: [PATCH] nfs: handle servers that support only ALLOW ACE type.

[Trond Myklebust]

On Fri, 2014-01-24 at 11:19 -0600, Malahal Naineni wrote:
> Currently we support ACLs if the NFS server file system supports both
> ALLOW and DENY ACE types. This patch makes the Linux client work with
> ACLs even if the server supports only 'ALLOW' ACE type.
> 
> Signed-off-by: Malahal Naineni <malahal@us.ibm.com>
> ---
>  fs/nfs/nfs4proc.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> index 15052b8..e3b8fa6 100644
> --- a/fs/nfs/nfs4proc.c
> +++ b/fs/nfs/nfs4proc.c
> @@ -4321,9 +4321,8 @@ static int nfs4_proc_renew(struct nfs_client *clp, struct rpc_cred *cred)
>  
>  static inline int nfs4_server_supports_acls(struct nfs_server *server)
>  {
> -	return (server->caps & NFS_CAP_ACLS)
> -		&& (server->acl_bitmask & ACL4_SUPPORT_ALLOW_ACL)
> -		&& (server->acl_bitmask & ACL4_SUPPORT_DENY_ACL);
> +	return server->caps & NFS_CAP_ACLS &&
> +		server->acl_bitmask & ACL4_SUPPORT_ALLOW_ACL;
>  }
>  
>  /* Assuming that XATTR_SIZE_MAX is a multiple of PAGE_SIZE, and that

Wait... Having looked at the code a bit more carefully. Is there any
reason to set NFS_CAP_ACLS at all if we don't see server->acl_bitmask &
ACL4_SUPPORT_ALLOW_ACL?

IOW: Is there any reason why we shouldn't just move the test for
server->acl_bitmask & ACL4_SUPPORT_ALLOW_ACL into
_nfs4_server_capabilities()?
I agree that will change the output of /proc/self/mountstats for broken
servers that advertise acls, but don't set ACL4_SUPPORT_ALLOW_ACL,
however since we will never serve up getacl, setacl or listacl requests
in that case, why would we advertise the server as supporting it?

Re: [PATCH] nfs: handle servers that support only ALLOW ACE type.

[Malahal Naineni]

[-- Attachment #1: Type: text/plain, Size: 1404 bytes --]

Trond Myklebust [trondmy@gmail.com] wrote:
> On Fri, 2014-01-24 at 11:19 -0600, Malahal Naineni wrote:
> > Currently we support ACLs if the NFS server file system supports both
> > ALLOW and DENY ACE types. This patch makes the Linux client work with
> > ACLs even if the server supports only 'ALLOW' ACE type.
> > 
> > Signed-off-by: Malahal Naineni <malahal@us.ibm.com>
> > ---
> >  fs/nfs/nfs4proc.c | 5 ++---
> >  1 file changed, 2 insertions(+), 3 deletions(-)
> > 
> > diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> > index 15052b8..e3b8fa6 100644
> > --- a/fs/nfs/nfs4proc.c
> > +++ b/fs/nfs/nfs4proc.c
> > @@ -4321,9 +4321,8 @@ static int nfs4_proc_renew(struct nfs_client *clp, struct rpc_cred *cred)
> >  
> >  static inline int nfs4_server_supports_acls(struct nfs_server *server)
> >  {
> > -	return (server->caps & NFS_CAP_ACLS)
> > -		&& (server->acl_bitmask & ACL4_SUPPORT_ALLOW_ACL)
> > -		&& (server->acl_bitmask & ACL4_SUPPORT_DENY_ACL);
> > +	return server->caps & NFS_CAP_ACLS &&
> > +		server->acl_bitmask & ACL4_SUPPORT_ALLOW_ACL;
> >  }
> >  
> >  /* Assuming that XATTR_SIZE_MAX is a multiple of PAGE_SIZE, and that
> 
> Wait... Having looked at the code a bit more carefully. Is there any
> reason to set NFS_CAP_ACLS at all if we don't see server->acl_bitmask &
> ACL4_SUPPORT_ALLOW_ACL?

I don't see any. Something like the attached patch should work!

Regards, Malahal.

[-- Attachment #2: 0001-nfs-handle-servers-that-support-only-ALLOW-ACE-type.patch --]
[-- Type: text/x-diff, Size: 1582 bytes --]

>From 7b949e7906d40704c4ae28a06f4c501161cb5a99 Mon Sep 17 00:00:00 2001
From: Malahal Naineni <malahal@us.ibm.com>
Date: Thu, 23 Jan 2014 08:54:55 -0600
Subject: [PATCH] nfs: handle servers that support only ALLOW ACE type.

Currently we support ACLs if the NFS server file system supports both
ALLOW and DENY ACE types. This patch makes the Linux client work with
ACLs even if the server supports only 'ALLOW' ACE type.

Signed-off-by: Malahal Naineni <malahal@us.ibm.com>
---
 fs/nfs/nfs4proc.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 15052b8..b007559 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -2744,7 +2744,8 @@ static int _nfs4_server_capabilities(struct nfs_server *server, struct nfs_fh *f
 				NFS_CAP_OWNER_GROUP|NFS_CAP_ATIME|
 				NFS_CAP_CTIME|NFS_CAP_MTIME|
 				NFS_CAP_SECURITY_LABEL);
-		if (res.attr_bitmask[0] & FATTR4_WORD0_ACL)
+		if (res.attr_bitmask[0] & FATTR4_WORD0_ACL &&
+				res.acl_bitmask & ACL4_SUPPORT_ALLOW_ACL)
 			server->caps |= NFS_CAP_ACLS;
 		if (res.has_links != 0)
 			server->caps |= NFS_CAP_HARDLINKS;
@@ -4321,9 +4322,7 @@ static int nfs4_proc_renew(struct nfs_client *clp, struct rpc_cred *cred)
 
 static inline int nfs4_server_supports_acls(struct nfs_server *server)
 {
-	return (server->caps & NFS_CAP_ACLS)
-		&& (server->acl_bitmask & ACL4_SUPPORT_ALLOW_ACL)
-		&& (server->acl_bitmask & ACL4_SUPPORT_DENY_ACL);
+	return server->caps & NFS_CAP_ACLS;
 }
 
 /* Assuming that XATTR_SIZE_MAX is a multiple of PAGE_SIZE, and that
-- 
1.8.3.1