diff mbox series

fs: fix regression querying for ACL on fs's that don't support them

Message ID 20230908-acl-fix-v1-1-1e6b76c8dcc8@kernel.org (mailing list archive)
State New, archived
Headers show
Series fs: fix regression querying for ACL on fs's that don't support them | expand

Commit Message

Jeff Layton Sept. 8, 2023, 9:05 p.m. UTC 
In the not too distant past, the VFS ACL infrastructure would return
-EOPNOTSUPP on filesystems (like NFS) that set SB_POSIXACL but that
don't supply a get_acl or get_inode_acl method. On more recent kernels
this returns -ENODATA, which breaks one method of detecting when ACLs
are supported.

Fix __get_acl to also check whether the inode has a "get_(inode_)?acl"
method and to just return -EOPNOTSUPP if not.

Reported-by: Ondrej Valousek <ondrej.valousek.xm@renesas.com>
Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
This patch is another approach to fixing this issue. I don't care too
much either way which approach we take, but this may fix the problem
for other filesystems too. Should we take a belt and suspenders
approach here and fix it in both places?
---
 fs/posix_acl.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)


---
base-commit: a48fa7efaf1161c1c898931fe4c7f0070964233a
change-id: 20230908-acl-fix-6f8f86930f32

Best regards,

Comments

Christian Brauner Sept. 10, 2023, 10:14 a.m. UTC | #1 
On Fri, Sep 08, 2023 at 05:05:27PM -0400, Jeff Layton wrote:
> In the not too distant past, the VFS ACL infrastructure would return
> -EOPNOTSUPP on filesystems (like NFS) that set SB_POSIXACL but that
> don't supply a get_acl or get_inode_acl method. On more recent kernels
> this returns -ENODATA, which breaks one method of detecting when ACLs
> are supported.
> 
> Fix __get_acl to also check whether the inode has a "get_(inode_)?acl"
> method and to just return -EOPNOTSUPP if not.
> 
> Reported-by: Ondrej Valousek <ondrej.valousek.xm@renesas.com>
> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
> This patch is another approach to fixing this issue. I don't care too
> much either way which approach we take, but this may fix the problem
> for other filesystems too. Should we take a belt and suspenders
> approach here and fix it in both places?
> ---
>  fs/posix_acl.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/posix_acl.c b/fs/posix_acl.c
> index a05fe94970ce..4c7c62040c43 100644
> --- a/fs/posix_acl.c
> +++ b/fs/posix_acl.c
> @@ -130,8 +130,12 @@ static struct posix_acl *__get_acl(struct mnt_idmap *idmap,
>  	if (!is_uncached_acl(acl))
>  		return acl;
>  
> -	if (!IS_POSIXACL(inode))
> -		return NULL;
> +	/*
> +	 * NB: checking this after checking for a cached ACL allows tmpfs
> +	 * (which doesn't specify a get_acl operation) to work properly.
> +	 */
> +	if (!IS_POSIXACL(inode) || (!inode->i_op->get_acl && !inode->i_op->get_inode_acl))
> +		return ERR_PTR(-EOPNOTSUPP);

Hmmm, I think that'll cause issues for permission checking during
lookup:

generic_permission()
-> acl_permission_check()
   -> check_acl()
      -> get_inode_acl()
         -> __get_acl()
            // return ERR_PTR(-EOPNOTSUPP) instead of NULL

Before this change this would've returned NULL and thus check_acl()
would've returned EAGAIN which would've informed acl_permission_check()
to continue with non-ACL based permission checking.

Now you're going to error out with EOPNOTSUPP and cause permission
checking to fallback to CAP_DAC_READ_SEARCH/CAP_DAC_OVERRIDE.

So if you want this change you'll either need to change check_acl() as well.
Unless I'm misreading.
Jeff Layton Sept. 10, 2023, 12:03 p.m. UTC | #2 
On Sun, 2023-09-10 at 12:14 +0200, Christian Brauner wrote:
> On Fri, Sep 08, 2023 at 05:05:27PM -0400, Jeff Layton wrote:
> > In the not too distant past, the VFS ACL infrastructure would return
> > -EOPNOTSUPP on filesystems (like NFS) that set SB_POSIXACL but that
> > don't supply a get_acl or get_inode_acl method. On more recent kernels
> > this returns -ENODATA, which breaks one method of detecting when ACLs
> > are supported.
> > 
> > Fix __get_acl to also check whether the inode has a "get_(inode_)?acl"
> > method and to just return -EOPNOTSUPP if not.
> > 
> > Reported-by: Ondrej Valousek <ondrej.valousek.xm@renesas.com>
> > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > ---
> > This patch is another approach to fixing this issue. I don't care too
> > much either way which approach we take, but this may fix the problem
> > for other filesystems too. Should we take a belt and suspenders
> > approach here and fix it in both places?
> > ---
> >  fs/posix_acl.c | 8 ++++++--
> >  1 file changed, 6 insertions(+), 2 deletions(-)
> > 
> > diff --git a/fs/posix_acl.c b/fs/posix_acl.c
> > index a05fe94970ce..4c7c62040c43 100644
> > --- a/fs/posix_acl.c
> > +++ b/fs/posix_acl.c
> > @@ -130,8 +130,12 @@ static struct posix_acl *__get_acl(struct mnt_idmap *idmap,
> >  	if (!is_uncached_acl(acl))
> >  		return acl;
> >  
> > -	if (!IS_POSIXACL(inode))
> > -		return NULL;
> > +	/*
> > +	 * NB: checking this after checking for a cached ACL allows tmpfs
> > +	 * (which doesn't specify a get_acl operation) to work properly.
> > +	 */
> > +	if (!IS_POSIXACL(inode) || (!inode->i_op->get_acl && !inode->i_op->get_inode_acl))
> > +		return ERR_PTR(-EOPNOTSUPP);
> 
> Hmmm, I think that'll cause issues for permission checking during
> lookup:
> 
> generic_permission()
> -> acl_permission_check()
>    -> check_acl()
>       -> get_inode_acl()
>          -> __get_acl()
>             // return ERR_PTR(-EOPNOTSUPP) instead of NULL
> 
> Before this change this would've returned NULL and thus check_acl()
> would've returned EAGAIN which would've informed acl_permission_check()
> to continue with non-ACL based permission checking.
> 
> Now you're going to error out with EOPNOTSUPP and cause permission
> checking to fallback to CAP_DAC_READ_SEARCH/CAP_DAC_OVERRIDE.
> 
> So if you want this change you'll either need to change check_acl() as well.
> Unless I'm misreading.

Ok, I didn't see problems in testing this with xfstests, but maybe it
didn't tickle that bug in the right way.

Instead of this, what if we were to add a new SB_NOUMASK flag? NFS could
set that, and then we could fix the places that NFS needs to use that
instead. That might bring more clarity to this code -- SB_POSIXACL would
really mean that ACLs were supported.

I'll see what I can put together...

Thanks!
--
Jeff Layton <jlayton@kernel.org>
diff mbox series

Patch

diff --git a/fs/posix_acl.c b/fs/posix_acl.c
index a05fe94970ce..4c7c62040c43 100644
--- a/fs/posix_acl.c
+++ b/fs/posix_acl.c
@@ -130,8 +130,12 @@  static struct posix_acl *__get_acl(struct mnt_idmap *idmap,
 	if (!is_uncached_acl(acl))
 		return acl;
 
-	if (!IS_POSIXACL(inode))
-		return NULL;
+	/*
+	 * NB: checking this after checking for a cached ACL allows tmpfs
+	 * (which doesn't specify a get_acl operation) to work properly.
+	 */
+	if (!IS_POSIXACL(inode) || (!inode->i_op->get_acl && !inode->i_op->get_inode_acl))
+		return ERR_PTR(-EOPNOTSUPP);
 
 	sentinel = uncached_acl_sentinel(current);
 	p = acl_by_type(inode, type);