* [PATCH] fs: RESOLVE_CACHED final path component fix
@ 2023-11-09 19:08 Andreas Gruenbacher
2023-11-09 22:00 ` Al Viro
0 siblings, 1 reply; 6+ messages in thread
From: Andreas Gruenbacher @ 2023-11-09 19:08 UTC (permalink / raw)
To: Jens Axboe
Cc: Andreas Gruenbacher, Alexander Viro, Christian Brauner, Abhi Das,
linux-fsdevel, linux-kernel
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>
diff --git a/fs/open.c b/fs/open.c
index 98f6601fbac6..61311c9845bd 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -1340,6 +1340,7 @@ inline int build_open_flags(const struct open_how *how, struct open_flags *op)
if (flags & (O_TRUNC | O_CREAT | __O_TMPFILE))
return -EAGAIN;
lookup_flags |= LOOKUP_CACHED;
+ op->acc_mode |= MAY_NOT_BLOCK;
}
op->lookup_flags = lookup_flags;
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] fs: RESOLVE_CACHED final path component fix
2023-11-09 19:08 [PATCH] fs: RESOLVE_CACHED final path component fix Andreas Gruenbacher
@ 2023-11-09 22:00 ` Al Viro
2023-11-09 22:11 ` Al Viro
2023-11-09 22:12 ` Andreas Grünbacher
0 siblings, 2 replies; 6+ messages in thread
From: Al Viro @ 2023-11-09 22:00 UTC (permalink / raw)
To: Andreas Gruenbacher
Cc: Jens Axboe, Christian Brauner, Abhi Das, linux-fsdevel,
linux-kernel
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] fs: RESOLVE_CACHED final path component fix
2023-11-09 22:00 ` Al Viro
@ 2023-11-09 22:11 ` Al Viro
2023-11-09 22:12 ` Andreas Grünbacher
1 sibling, 0 replies; 6+ messages in thread
From: Al Viro @ 2023-11-09 22:11 UTC (permalink / raw)
To: Andreas Gruenbacher
Cc: Jens Axboe, Christian Brauner, Abhi Das, linux-fsdevel,
linux-kernel
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] fs: RESOLVE_CACHED final path component fix
2023-11-09 22:00 ` Al Viro
2023-11-09 22:11 ` Al Viro
@ 2023-11-09 22:12 ` Andreas Grünbacher
2023-11-09 22:22 ` Al Viro
1 sibling, 1 reply; 6+ messages in thread
From: Andreas Grünbacher @ 2023-11-09 22:12 UTC (permalink / raw)
To: Al Viro
Cc: Andreas Gruenbacher, Jens Axboe, Christian Brauner, Abhi Das,
linux-fsdevel, linux-kernel
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.
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] fs: RESOLVE_CACHED final path component fix
2023-11-09 22:12 ` Andreas Grünbacher
@ 2023-11-09 22:22 ` Al Viro
2023-12-12 19:37 ` Abhi Das
0 siblings, 1 reply; 6+ messages in thread
From: Al Viro @ 2023-11-09 22:22 UTC (permalink / raw)
To: Andreas Grünbacher
Cc: Andreas Gruenbacher, Jens Axboe, Christian Brauner, Abhi Das,
linux-fsdevel, linux-kernel
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] fs: RESOLVE_CACHED final path component fix
2023-11-09 22:22 ` Al Viro
@ 2023-12-12 19:37 ` Abhi Das
0 siblings, 0 replies; 6+ messages in thread
From: Abhi Das @ 2023-12-12 19:37 UTC (permalink / raw)
To: Al Viro
Cc: Andreas Grünbacher, Andreas Gruenbacher, Jens Axboe,
Christian Brauner, linux-fsdevel, linux-kernel, Eric Sandeen
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.
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-12-12 19:38 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-11-09 19:08 [PATCH] fs: RESOLVE_CACHED final path component fix Andreas Gruenbacher
2023-11-09 22:00 ` Al Viro
2023-11-09 22:11 ` Al Viro
2023-11-09 22:12 ` Andreas Grünbacher
2023-11-09 22:22 ` Al Viro
2023-12-12 19:37 ` Abhi Das
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).