| Message ID | 20200119031738.2681033-4-viro@ZenIV.linux.org.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [01/17] do_add_mount(): lift lock_mount/unlock_mount into callers | expand |
On Sun, Jan 19, 2020 at 03:17:16AM +0000, Al Viro wrote: > From: Al Viro <viro@zeniv.linux.org.uk> > > only the address of ->total_link_count and the flags > > Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> > --- > fs/namei.c | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/fs/namei.c b/fs/namei.c > index d30a74a18da9..3b6f60c02f8a 100644 > --- a/fs/namei.c > +++ b/fs/namei.c > @@ -1133,7 +1133,7 @@ EXPORT_SYMBOL(follow_up); > * - return -EISDIR to tell follow_managed() to stop and return the path we > * were called with. > */ > -static int follow_automount(struct path *path, struct nameidata *nd) > +static int follow_automount(struct path *path, int *count, unsigned lookup_flags) > { > struct dentry *dentry = path->dentry; > > @@ -1148,13 +1148,12 @@ static int follow_automount(struct path *path, struct nameidata *nd) > * as being automount points. These will need the attentions > * of the daemon to instantiate them before they can be used. > */ > - if (!(nd->flags & (LOOKUP_PARENT | LOOKUP_DIRECTORY | > + if (!(lookup_flags & (LOOKUP_PARENT | LOOKUP_DIRECTORY | > LOOKUP_OPEN | LOOKUP_CREATE | LOOKUP_AUTOMOUNT)) && > dentry->d_inode) > return -EISDIR; > > - nd->total_link_count++; > - if (nd->total_link_count >= 40) > + if (count && *count++ >= 40) He, side-effects galore. :) Isn't this incrementing the address but you want to increment the counter? Seems like this should be if (count && (*count)++ >= 40) and even then it seems to me not incrementing at all when we have hit the limit seems more natural? Christian
On Thu, Jan 30, 2020 at 03:45:20PM +0100, Christian Brauner wrote: > > - nd->total_link_count++; > > - if (nd->total_link_count >= 40) > > + if (count && *count++ >= 40) > > He, side-effects galore. :) > Isn't this incrementing the address but you want to increment the > counter? > Seems like this should be > > if (count && (*count)++ >= 40) Nice catch; incidentally, it means that usual testsuites (xfstests, LTP) are missing the automount loop detection. Hmm... Something like export $FOO over nfs4 to localhost mkdir $FOO/sub touch $FOO/a mount $SCRATCH_DEV $FOO/sub touch $FOO/sub/a cd $BAR mkdir nfs mount -t nfs localhost:$FOO nfs for i in `seq 40`; do ln -s l`expr $i - 1` l$i; done for i in `seq 40`; do ln -s m`expr $i - 1` m$i; done ln -s nfs/sub/a l0 ln -s nfs/a m0 for i in `seq 40`; do umount nfs/sub 2>/dev/null cat l$i m$i done BTW, the check of pre-increment value is more correct - it's accidental, but it does give consistency with the normal symlink following. We do allow up to 40 symlinks over the pathname resolution, not up to 39. The thing above should produce cat: l39: Too many levels of symbolic links cat: l40: Too many levels of symbolic links cat: m40: Too many levels of symbolic links Here l<n> and m<n> go through n + 1 symlink, ending at nfs/sub/a and nfs/a resp.; the former does trigger an automount, the latter does not. On mainline it actually starts to complain about l38, l39, l40 and m40, due to that off-by-one in follow_automount().
On Thu, Jan 30, 2020 at 03:38:25PM +0000, Al Viro wrote: > On Thu, Jan 30, 2020 at 03:45:20PM +0100, Christian Brauner wrote: > > > - nd->total_link_count++; > > > - if (nd->total_link_count >= 40) > > > + if (count && *count++ >= 40) > > > > He, side-effects galore. :) > > Isn't this incrementing the address but you want to increment the > > counter? > > Seems like this should be > > > > if (count && (*count)++ >= 40) > > Nice catch; incidentally, it means that usual testsuites (xfstests, > LTP) are missing the automount loop detection. Hmm... Fix folded and pushed (the series in #next.namei now, on top of #work.openat2 + #fixes)
diff --git a/fs/namei.c b/fs/namei.c index d30a74a18da9..3b6f60c02f8a 100644 --- a/fs/namei.c +++ b/fs/namei.c @@ -1133,7 +1133,7 @@ EXPORT_SYMBOL(follow_up); * - return -EISDIR to tell follow_managed() to stop and return the path we * were called with. */ -static int follow_automount(struct path *path, struct nameidata *nd) +static int follow_automount(struct path *path, int *count, unsigned lookup_flags) { struct dentry *dentry = path->dentry; @@ -1148,13 +1148,12 @@ static int follow_automount(struct path *path, struct nameidata *nd) * as being automount points. These will need the attentions * of the daemon to instantiate them before they can be used. */ - if (!(nd->flags & (LOOKUP_PARENT | LOOKUP_DIRECTORY | + if (!(lookup_flags & (LOOKUP_PARENT | LOOKUP_DIRECTORY | LOOKUP_OPEN | LOOKUP_CREATE | LOOKUP_AUTOMOUNT)) && dentry->d_inode) return -EISDIR; - nd->total_link_count++; - if (nd->total_link_count >= 40) + if (count && *count++ >= 40) return -ELOOP; return finish_automount(dentry->d_op->d_automount(path), path); @@ -1215,7 +1214,8 @@ static int follow_managed(struct path *path, struct nameidata *nd) /* Handle an automount point */ if (flags & DCACHE_NEED_AUTOMOUNT) { - ret = follow_automount(path, nd); + ret = follow_automount(path, &nd->total_link_count, + nd->flags); if (ret < 0) break; continue;