On 2020-01-18, Al Viro 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. -- Aleksa Sarai Senior Software Engineer (Containers) SUSE Linux GmbH