Re: [PATCH v17 10/13] namei: LOOKUP_{IN_ROOT,BENEATH}: permit limited ".." resolution

[Aleksa Sarai <cyphar@cyphar.com>]

[-- Attachment #1: Type: text/plain, Size: 3231 bytes --]

On 2019-11-26, Aleksa Sarai <cyphar@cyphar.com> wrote:
> On 2019-11-25, Al Viro <viro@zeniv.linux.org.uk> wrote:
> > On Sun, Nov 17, 2019 at 12:17:10PM +1100, Aleksa Sarai wrote:
> > > +		if (unlikely(nd->flags & LOOKUP_IS_SCOPED)) {
> > > +			/*
> > > +			 * If there was a racing rename or mount along our
> > > +			 * path, then we can't be sure that ".." hasn't jumped
> > > +			 * above nd->root (and so userspace should retry or use
> > > +			 * some fallback).
> > > +			 */
> > > +			if (unlikely(read_seqretry(&mount_lock, nd->m_seq)))
> > > +				return -EAGAIN;
> > > +			if (unlikely(read_seqretry(&rename_lock, nd->r_seq)))
> > > +				return -EAGAIN;
> > > +		}
> > 
> > Looks like excessive barriers to me - it's
> > 	rmb
> > 	check mount_lock.sequence
> > 	rmb
> > 	check rename_lock.sequence
> 
> If you like, I can switch this to
> 
> 	smp_rmb();
> 	if (unlikely(__read_seqcount_retry(&mount_lock.seqcount, nd->m_seq)))
> 		return -EAGAIN;
> 	if (unlikely(__read_seqcount_retry(&rename_lock.seqcount, nd->r_seq)))
> 		return -EAGAIN;
> 
> Though I think it makes it more noisy (and this code-path will only be
> hit for ".." and LOOKUP_IS_SCOPED).
> 
> > > @@ -2266,6 +2274,10 @@ static const char *path_init(struct nameidata *nd, unsigned flags)
> > >  	nd->last_type = LAST_ROOT; /* if there are only slashes... */
> > >  	nd->flags = flags | LOOKUP_JUMPED | LOOKUP_PARENT;
> > >  	nd->depth = 0;
> > > +
> > > +	nd->m_seq = read_seqbegin(&mount_lock);
> > > +	nd->r_seq = read_seqbegin(&rename_lock);
> > 
> > Same here, pretty much (fetch/rmb/fetch/rmb)
> 
> Unless I'm mistaken, wouldn't we have to do
> seqcount_lockdep_reader_access() explicitly -- so it would end up
> looking something like:
> 
> 	seqcount_lockdep_reader_access(&mount_lock.seqcount);
> 	nd->m_seq = __read_seqcount_begin(&mount_lock.seqcount);
> 	seqcount_lockdep_reader_access(&mount_lock.seqcount);
> 	nd->r_seq = __read_seqcount_begin(&rename_lock.seqcount);
> 	smp_rmb();

Actually, looking it again (unless I'm mistaken) the following should be
acceptable and it also avoids the extra fetch+rmb of mount_lock for
LOOKUP_ROOT. The only downside is that we don't get lockdep information
but path_init() already ignores lockdep when grabbing d_seq.

I will include the following in v18, but let me know if I'm missing
something obvious:

 >>	nd->m_seq = __read_seqcount_begin(&mount_lock);
 >>	nd->r_seq = __read_seqcount_begin(&rename_lock);
 >>	smp_rmb();

	if (flags & LOOKUP_ROOT) {
		struct dentry *root = nd->root.dentry;
		struct inode *inode = root->d_inode;
		if (*s && unlikely(!d_can_lookup(root)))
			return ERR_PTR(-ENOTDIR);
		nd->path = nd->root;
		nd->inode = inode;
		if (flags & LOOKUP_RCU) {
 >>			nd->seq = raw_read_seqcount_begin(&nd->path.dentry->d_seq);
			nd->root_seq = nd->seq;
		} else {
			path_get(&nd->path);
		}
		return s;
	}

I could also move the smp_rmb() to after LOOKUP_ROOT (and add an
smp_rmb() at the end of LOOKUP_ROOT) which would avoid a double-rmb for
LOOKUP_ROOT -- but it makes it harder to read IMHO.

-- 
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
<https://www.cyphar.com/>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]