Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Thu, 30 Jan 2020 15:38:25 +0000
From:   Al Viro <viro@zeniv.linux.org.uk>
To:     Christian Brauner <christian.brauner@ubuntu.com>
Cc:     linux-fsdevel@vger.kernel.org,
        Linus Torvalds <torvalds@linux-foundation.org>,
        linux-kernel@vger.kernel.org, Aleksa Sarai <cyphar@cyphar.com>,
        David Howells <dhowells@redhat.com>,
        Eric Biederman <ebiederm@xmission.com>
Subject: Re: [PATCH 04/17] follow_automount() doesn't need the entire
 nameidata
Message-ID: <20200130153825.GA23230@ZenIV.linux.org.uk>
References: <20200119031423.GV8904@ZenIV.linux.org.uk>
 <20200119031738.2681033-1-viro@ZenIV.linux.org.uk>
 <20200119031738.2681033-4-viro@ZenIV.linux.org.uk>
 <20200130144520.hnf2yk5tjalxfddn@wittgenstein>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20200130144520.hnf2yk5tjalxfddn@wittgenstein>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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().