On 2019-07-12, Al Viro <viro@xxxxxxxxxxxxxxxxxx> wrote:
On Fri, Jul 12, 2019 at 10:20:17PM +1000, Aleksa Sarai wrote:On 2019-07-12, Al Viro <viro@xxxxxxxxxxxxxxxxxx> wrote:On Sun, Jul 07, 2019 at 12:57:28AM +1000, Aleksa Sarai wrote:@@ -514,7 +516,14 @@ static void set_nameidata(struct nameidata *p, int dfd, struct filename *name) p->stack = p->internal; p->dfd = dfd; p->name = name; - p->total_link_count = old ? old->total_link_count : 0; + p->total_link_count = 0; + p->acc_mode = 0; + p->opath_mask = FMODE_PATH_READ | FMODE_PATH_WRITE; + if (old) { + p->total_link_count = old->total_link_count; + p->acc_mode = old->acc_mode; + p->opath_mask = old->opath_mask; + }Huh? Could somebody explain why traversals of NFS4 referrals should inherit ->acc_mode and ->opath_mask?I'll be honest -- I don't understand what set_nameidata() did so I just did what I thought would be an obvious change (to just copy the contents). I thought it was related to some aspect of the symlink stack handling.No. It's handling of (very rare) nested pathwalk. The only case I can think of is handling of NFS4 referrals - they are triggered by ->d_automount() and include NFS4 mount. Which does internal pathwalk of its own, to get to the root of subtree being automounted. NFS has its own recursion protection on that path (no deeper nesting than one level of referral traversals), but there some nesting is inevitable; we do get another nameidata instance on stack. And for nd_jump_link() we need to keep track of the innermost one. For symlinks nothing of that sort happens - they are dealt with on the same struct nameidata. ->total_link_count copying is there for one reason only - we want the total amount of symlinks traversed during the pathwalk (including the referral processing, etc.) to count towards MAXSYMLINKS check. It could've been moved from nameidata to task_struct, but it's cheaper to handle it that way. Again, nesting is *rare*.
Thanks for the explanation, much appreciated. I will drop the old->... copying hunk.
In that case, should they both be set to 0 on set_nameidata()? This will mean that fd re-opening (or magic-link opening) through a set_nameidata() would always fail.Huh? set_nameidata() is done for *all* instances - it's pretty much the constructor of that object (and restore_nameidata() - a destructor). Everything goes through it.
Sorry, I meant to drop the copy-from-old logic -- not set it to zero explicitly in set_nameidata().
And again, I'm not sure we want these fields in nameidata - IMO they belong in open_flags. Things like e.g. stat() don't need them at all.
Yup, I'll work up a version that does the consolidation you mentioned in your other mail.
Incidentally, O_PATH opening of symlinks combined with subsequent procfs symlink traversals is worth testing - that's where the things get subtle and that's where it's easy to get in trouble on modifications.
I have some self-tests of a symlink-to-a-magic-link in the last patch of the series. Did you mean something even more chained like a symlink to a /proc/self/fd/$n of an O_NOFOLLOW|O_PATH of a symlink? -- Aleksa Sarai Senior Software Engineer (Containers) SUSE Linux GmbH <https://www.cyphar.com/>
Attachment:
signature.asc
Description: PGP signature
