On Fri, Jul 12, 2019 at 10:20:17PM +1000, Aleksa Sarai wrote:
On 2019-07-12, Al Viro viro@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*.
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.
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.
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.