fs: RESOLVE_CACHED final path component fix

  Jens,

since your commit 99668f618062, applications can request cached lookups
with the RESOLVE_CACHED openat2() flag.  When adding support for that in
gfs2, we found that this causes the ->permission inode operation to be
called with the MAY_NOT_BLOCK flag set for directories along the path,
which is good, but the ->permission check on the final path component is
missing that flag.  The filesystem will then sleep when it needs to read
in the ACL, for example.

This doesn't look like the intended RESOLVE_CACHED behavior.

The file permission checks in path_openat() happen as follows:

(1) link_path_walk() -> may_lookup() -> inode_permission() is called for
each but the final path component. If the LOOKUP_RCU nameidata flag is
set, may_lookup() passes the MAY_NOT_BLOCK flag on to
inode_permission(), which passes it on to the permission inode
operation.

(2) do_open() -> may_open() -> inode_permission() is called for the
final path component. The MAY_* flags passed to inode_permission() are
computed by build_open_flags(), outside of do_open(), and passed down
from there. The MAY_NOT_BLOCK flag doesn't get set.

I think we can fix this in build_open_flags(), by setting the
MAY_NOT_BLOCK flag when a RESOLVE_CACHED lookup is requested, right
where RESOLVE_CACHED is mapped to LOOKUP_CACHED as well.

Fixes: 99668f618062 ("fs: expose LOOKUP_CACHED through openat2() RESOLVE_CACHED")
Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
  

On Thu, Nov 09, 2023 at 08:08:44PM +0100, Andreas Gruenbacher wrote:
> Jens,
> 
> since your commit 99668f618062, applications can request cached lookups
> with the RESOLVE_CACHED openat2() flag.  When adding support for that in
> gfs2, we found that this causes the ->permission inode operation to be
> called with the MAY_NOT_BLOCK flag set for directories along the path,
> which is good, but the ->permission check on the final path component is
> missing that flag.  The filesystem will then sleep when it needs to read
> in the ACL, for example.
> 
> This doesn't look like the intended RESOLVE_CACHED behavior.
> 
> The file permission checks in path_openat() happen as follows:
> 
> (1) link_path_walk() -> may_lookup() -> inode_permission() is called for
> each but the final path component. If the LOOKUP_RCU nameidata flag is
> set, may_lookup() passes the MAY_NOT_BLOCK flag on to
> inode_permission(), which passes it on to the permission inode
> operation.
> 
> (2) do_open() -> may_open() -> inode_permission() is called for the
> final path component. The MAY_* flags passed to inode_permission() are
> computed by build_open_flags(), outside of do_open(), and passed down
> from there. The MAY_NOT_BLOCK flag doesn't get set.
> 
> I think we can fix this in build_open_flags(), by setting the
> MAY_NOT_BLOCK flag when a RESOLVE_CACHED lookup is requested, right
> where RESOLVE_CACHED is mapped to LOOKUP_CACHED as well.

No.  This will expose ->permission() instances to previously impossible
cases of MAY_NOT_BLOCK lookups, and we already have enough trouble
in that area.  See RCU pathwalk patches I posted last cycle; I'm
planning to rebase what still needs to be rebased and feed the
fixes into mainline, but that won't happen until the end of this
week *AND* ->permission()-related part of code audit will need
to be repeated and extended.

Until then - no, with the side of fuck, no.
  

On Thu, Nov 09, 2023 at 10:00:18PM +0000, Al Viro wrote:
> On Thu, Nov 09, 2023 at 08:08:44PM +0100, Andreas Gruenbacher wrote:
> > Jens,
> > 
> > since your commit 99668f618062, applications can request cached lookups
> > with the RESOLVE_CACHED openat2() flag.  When adding support for that in
> > gfs2, we found that this causes the ->permission inode operation to be
> > called with the MAY_NOT_BLOCK flag set for directories along the path,
> > which is good, but the ->permission check on the final path component is
> > missing that flag.  The filesystem will then sleep when it needs to read
> > in the ACL, for example.
> > 
> > This doesn't look like the intended RESOLVE_CACHED behavior.
> > 
> > The file permission checks in path_openat() happen as follows:
> > 
> > (1) link_path_walk() -> may_lookup() -> inode_permission() is called for
> > each but the final path component. If the LOOKUP_RCU nameidata flag is
> > set, may_lookup() passes the MAY_NOT_BLOCK flag on to
> > inode_permission(), which passes it on to the permission inode
> > operation.
> > 
> > (2) do_open() -> may_open() -> inode_permission() is called for the
> > final path component. The MAY_* flags passed to inode_permission() are
> > computed by build_open_flags(), outside of do_open(), and passed down
> > from there. The MAY_NOT_BLOCK flag doesn't get set.
> > 
> > I think we can fix this in build_open_flags(), by setting the
> > MAY_NOT_BLOCK flag when a RESOLVE_CACHED lookup is requested, right
> > where RESOLVE_CACHED is mapped to LOOKUP_CACHED as well.
> 
> No.  This will expose ->permission() instances to previously impossible
> cases of MAY_NOT_BLOCK lookups, and we already have enough trouble
> in that area.  See RCU pathwalk patches I posted last cycle; I'm
> planning to rebase what still needs to be rebased and feed the
> fixes into mainline, but that won't happen until the end of this
> week *AND* ->permission()-related part of code audit will need
> to be repeated and extended.
> 
> Until then - no, with the side of fuck, no.

Note that it's not just "->permission() might get called in RCU mode with
combination of flags it never had seen before"; it actually would be
called with MAY_NOT_BLOCK and without rcu_read_lock() held.

Which means that it can't make the usual assumptions about the objects
not getting freed under it in such mode.  Sure, the inode itself is
pinned in your new case, but anything that goes
	if !MAY_NOT_BLOCK
		grab a spinlock
		grab a reference to X from inode
		drop spinlock
		use X, it's pinned down
		drop a reference to X
	else
		READ_ONCE the reference to X
		use X, its freeing is RCU-delayed
would get screwed.  And that would need to be audited for all instances
of ->permission() in the tree, along with all weird shit they might be
pulling off.
  

Am Do., 9. Nov. 2023 um 23:00 Uhr schrieb Al Viro <viro@zeniv.linux.org.uk>:
> On Thu, Nov 09, 2023 at 08:08:44PM +0100, Andreas Gruenbacher wrote:
> > Jens,
> >
> > since your commit 99668f618062, applications can request cached lookups
> > with the RESOLVE_CACHED openat2() flag.  When adding support for that in
> > gfs2, we found that this causes the ->permission inode operation to be
> > called with the MAY_NOT_BLOCK flag set for directories along the path,
> > which is good, but the ->permission check on the final path component is
> > missing that flag.  The filesystem will then sleep when it needs to read
> > in the ACL, for example.
> >
> > This doesn't look like the intended RESOLVE_CACHED behavior.
> >
> > The file permission checks in path_openat() happen as follows:
> >
> > (1) link_path_walk() -> may_lookup() -> inode_permission() is called for
> > each but the final path component. If the LOOKUP_RCU nameidata flag is
> > set, may_lookup() passes the MAY_NOT_BLOCK flag on to
> > inode_permission(), which passes it on to the permission inode
> > operation.
> >
> > (2) do_open() -> may_open() -> inode_permission() is called for the
> > final path component. The MAY_* flags passed to inode_permission() are
> > computed by build_open_flags(), outside of do_open(), and passed down
> > from there. The MAY_NOT_BLOCK flag doesn't get set.
> >
> > I think we can fix this in build_open_flags(), by setting the
> > MAY_NOT_BLOCK flag when a RESOLVE_CACHED lookup is requested, right
> > where RESOLVE_CACHED is mapped to LOOKUP_CACHED as well.
>
> No.  This will expose ->permission() instances to previously impossible
> cases of MAY_NOT_BLOCK lookups, and we already have enough trouble
> in that area.

True, lockdep wouldn't be happy.

>  See RCU pathwalk patches I posted last cycle;

Do you have a pointer? Thanks.

> I'm
> planning to rebase what still needs to be rebased and feed the
> fixes into mainline, but that won't happen until the end of this
> week *AND* ->permission()-related part of code audit will need
> to be repeated and extended.
>
> Until then - no, with the side of fuck, no.
>
  

On Thu, Nov 09, 2023 at 11:12:32PM +0100, Andreas Grünbacher wrote:
> Am Do., 9. Nov. 2023 um 23:00 Uhr schrieb Al Viro <viro@zeniv.linux.org.uk>:
> > On Thu, Nov 09, 2023 at 08:08:44PM +0100, Andreas Gruenbacher wrote:
> > > Jens,
> > >
> > > since your commit 99668f618062, applications can request cached lookups
> > > with the RESOLVE_CACHED openat2() flag.  When adding support for that in
> > > gfs2, we found that this causes the ->permission inode operation to be
> > > called with the MAY_NOT_BLOCK flag set for directories along the path,
> > > which is good, but the ->permission check on the final path component is
> > > missing that flag.  The filesystem will then sleep when it needs to read
> > > in the ACL, for example.
> > >
> > > This doesn't look like the intended RESOLVE_CACHED behavior.
> > >
> > > The file permission checks in path_openat() happen as follows:
> > >
> > > (1) link_path_walk() -> may_lookup() -> inode_permission() is called for
> > > each but the final path component. If the LOOKUP_RCU nameidata flag is
> > > set, may_lookup() passes the MAY_NOT_BLOCK flag on to
> > > inode_permission(), which passes it on to the permission inode
> > > operation.
> > >
> > > (2) do_open() -> may_open() -> inode_permission() is called for the
> > > final path component. The MAY_* flags passed to inode_permission() are
> > > computed by build_open_flags(), outside of do_open(), and passed down
> > > from there. The MAY_NOT_BLOCK flag doesn't get set.
> > >
> > > I think we can fix this in build_open_flags(), by setting the
> > > MAY_NOT_BLOCK flag when a RESOLVE_CACHED lookup is requested, right
> > > where RESOLVE_CACHED is mapped to LOOKUP_CACHED as well.
> >
> > No.  This will expose ->permission() instances to previously impossible
> > cases of MAY_NOT_BLOCK lookups, and we already have enough trouble
> > in that area.
> 
> True, lockdep wouldn't be happy.
> 
> >  See RCU pathwalk patches I posted last cycle;
> 
> Do you have a pointer? Thanks.

Thread starting with Message-ID: <20231002022815.GQ800259@ZenIV>
I don't remember if I posted the audit notes into it; I'll get around
to resurrecting that stuff this weekend, when the mainline settles down
enough to bother with that.
  

Hi Al,

Did you get a chance to look into the RCU pathwalk stuff a bit more?
Any ideas on how to allow may_open() to indicate to inode_permission()
that it's part of a RESOLVE_CACHED lookup?

Cheers!
--Abhi


On Thu, Nov 9, 2023 at 4:23 PM Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> On Thu, Nov 09, 2023 at 11:12:32PM +0100, Andreas Grünbacher wrote:
> > Am Do., 9. Nov. 2023 um 23:00 Uhr schrieb Al Viro <viro@zeniv.linux.org.uk>:
> > > On Thu, Nov 09, 2023 at 08:08:44PM +0100, Andreas Gruenbacher wrote:
> > > > Jens,
> > > >
> > > > since your commit 99668f618062, applications can request cached lookups
> > > > with the RESOLVE_CACHED openat2() flag.  When adding support for that in
> > > > gfs2, we found that this causes the ->permission inode operation to be
> > > > called with the MAY_NOT_BLOCK flag set for directories along the path,
> > > > which is good, but the ->permission check on the final path component is
> > > > missing that flag.  The filesystem will then sleep when it needs to read
> > > > in the ACL, for example.
> > > >
> > > > This doesn't look like the intended RESOLVE_CACHED behavior.
> > > >
> > > > The file permission checks in path_openat() happen as follows:
> > > >
> > > > (1) link_path_walk() -> may_lookup() -> inode_permission() is called for
> > > > each but the final path component. If the LOOKUP_RCU nameidata flag is
> > > > set, may_lookup() passes the MAY_NOT_BLOCK flag on to
> > > > inode_permission(), which passes it on to the permission inode
> > > > operation.
> > > >
> > > > (2) do_open() -> may_open() -> inode_permission() is called for the
> > > > final path component. The MAY_* flags passed to inode_permission() are
> > > > computed by build_open_flags(), outside of do_open(), and passed down
> > > > from there. The MAY_NOT_BLOCK flag doesn't get set.
> > > >
> > > > I think we can fix this in build_open_flags(), by setting the
> > > > MAY_NOT_BLOCK flag when a RESOLVE_CACHED lookup is requested, right
> > > > where RESOLVE_CACHED is mapped to LOOKUP_CACHED as well.
> > >
> > > No.  This will expose ->permission() instances to previously impossible
> > > cases of MAY_NOT_BLOCK lookups, and we already have enough trouble
> > > in that area.
> >
> > True, lockdep wouldn't be happy.
> >
> > >  See RCU pathwalk patches I posted last cycle;
> >
> > Do you have a pointer? Thanks.
>
> Thread starting with Message-ID: <20231002022815.GQ800259@ZenIV>
> I don't remember if I posted the audit notes into it; I'll get around
> to resurrecting that stuff this weekend, when the mainline settles down
> enough to bother with that.
>