On 2019-11-25, Al Viro <viro@xxxxxxxxxxxxxxxxxx> 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();
Given this code only runs once at the start of each lookup, I'm not sure
it makes much sense to expand it like that and make it look uglier.
If you really want to avoid the duplicate memory barriers in the common
case I could instead gate the rename_lock grab behind LOOKUP_IS_SCOPED
(since that's the only time it's used).
--
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
<https://www.cyphar.com/>
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ Containers mailing list Containers@xxxxxxxxxxxxxxxxxxxxxxxxxx https://lists.linuxfoundation.org/mailman/listinfo/containers
