On 2019-11-26, Aleksa Sarai <cyp...@cyphar.com> wrote:
> On 2019-11-25, Al Viro <v...@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/>
signature.asc
Description: PGP signature
