On 2019-07-12, Al Viro <v...@zeniv.linux.org.uk> wrote:
> On Fri, Jul 12, 2019 at 10:20:17PM +1000, Aleksa Sarai wrote:
> > On 2019-07-12, Al Viro <v...@zeniv.linux.org.uk> 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/>
signature.asc
Description: PGP signature
