On 2020-01-18, Al Viro viro@zeniv.linux.org.uk wrote:
On Sat, Jan 18, 2020 at 03:28:33PM +0000, Al Viro wrote:
#work.openat2 updated, #for-next rebuilt and force-pushed. There's a massive update of #work.namei as well, also pushed out; not in #for-next yet, will post the patch series for review later today.
BTW, looking through that code again, how could this static bool legitimize_root(struct nameidata *nd) { /* * For scoped-lookups (where nd->root has been zeroed), we need to * restart the whole lookup from scratch -- because set_root() is wrong * for these lookups (nd->dfd is the root, not the filesystem root). */ if (!nd->root.mnt && (nd->flags & LOOKUP_IS_SCOPED)) return false;
possibly trigger? The only things that ever clean ->root.mnt are
You're quite right -- the codepath I was worried about was pick_link() failing (which *does* clear nd->path.mnt, and I must've misread it at the time as nd->root.mnt).
We can drop this check, though now complete_walk()'s main defence against a NULL nd->root.mnt is that path_is_under() will fail and trigger -EXDEV (or set_root() will fail at some point in the future). However, as you pointed out, a NULL nd->root.mnt won't happen with things as they stand today -- I might be a little too paranoid. :P
This is really, really fundamental for understanding the whole thing - a failure of unlazy_walk/unlazy_child means that we are through with that attempt.
Yup -- see above, the worry was about pick_link() not about how the RCU-walk and REF-walk dances operate.
The same, BTW, goes for the check you've added in the beginning of set_root() - set_root() is called only with NULL nd->root.mnt (trivial to prove) and that is incompatible with LOOKUP_IS_SCOPED. I'm kinda-sorta OK with having WARN_ON() there for a while, but IMO the check in the beginning of legitimize_root() should go away -
You're quite right about dropping the legitimize_root() check, but I'd like to keep the WARN_ON() in set_root(). The main reason being that it makes us very damn sure that a future change won't accidentally break the nd->root contract which all of the LOOKUP_IS_SCOPED changes rely on. Then again, this might be my paranoia popping up again.
this kind of defensive programming only makes harder to reason about the behaviour of the entire thing. And fs/namei.c is too convoluted as it is...
If you feel that dropping some of these more defensive checks is better for the codebase as a whole, then I defer to your judgement. I completely agree that namei is a pretty complicated chunk of code.