| Message ID | 20231109190844.2044940-1-agruenba@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fs: RESOLVE_CACHED final path component fix | expand |
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. >
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;
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>