> > +static u32 context_to_sid(struct sidtab *s, struct context *context)
> > +{
> > + struct sidtab_entry_leaf *entry;
> > +
> > + hash_for_each_possible(s->context_to_sid, entry, list, context->hash) {
> > + if (context_cmp(&entry->context, context))
> > + return entry->sid;
>
> For this to be semantically safe against entries being added, it would
> need the hlist head -> first member to be read with
> smp_load_acquire(). Probably in practice you would always get fresh
> data when dereferencing a pointer, but I don't think we should rely on
> that... In fact it looks like hash_for_each_possible() wasn't really
> written with lockless traversal in mind - I checked a few random
> places that call it and they all either do it under some lock or
> expect the table to be immutable. I believe you will need to introduce
> a new equivalents of hash_add()/hlist_add_head() (that does
> smp_store_release() instead of WRITE_ONCE(), because the 'next'
> pointer of the node being added must be written before the head's
> first pointer is set to point to the new node) and
> hash_for_each_possible()/hlist_for_each_entry() (that loads
> (head)->first with smp_load_acquire()).
>
> Then again, I'm not an expert on this synchronization stuff, so I
> might be wrong... But you will have to prove it :)
I spoke with Will Deacon about this and he confirmed your concerns that
this cannot be done safely without locking. I'll migrate to the "rcu" hashtable
variant.
>
> > + }
> > + return 0;
> > +}
> > +
>
> I am a little uneasy about this function having the same name as the
> context_to_sid() in security.c. It could be confusing when debugging.
> Maybe you could name the security.c function to
> context_struct_to_sid()?
>
> > int sidtab_set_initial(struct sidtab *s, u32 sid, struct context *context)
> > {
> > struct sidtab_isid_entry *entry;
>
> <snip>
>
> > @@ -305,29 +275,35 @@ static int sidtab_reverse_lookup(struct sidtab *s, struct context *context,
> > rc = -ENOMEM;
> > dst_convert = sidtab_do_lookup(convert->target, count, 1);
> > if (!dst_convert) {
> > - context_destroy(dst);
> > + context_destroy(&dst->context);
> > goto out_unlock;
> > }
> >
> > - rc = convert->func(context, dst_convert, convert->args);
> > + rc = convert->func(context, &dst_convert->context,
> > + convert->args);
> > if (rc) {
> > - context_destroy(dst);
> > + context_destroy(&dst->context);
> > goto out_unlock;
> > }
> > + dst_convert->sid = index_to_sid(count);
> >
> > /* at this point we know the insert won't fail */
> > + spin_lock_irqsave(&convert->target->lock, flags);
> > convert->target->count = count + 1;
> > + hash_add(convert->target->context_to_sid,
> > + &dst_convert->list, dst_convert->context.hash);
> > + spin_unlock_irqrestore(&convert->target->lock, flags);
>
> Do we really need to lock the conversion target here? The
> (undocumented) contract here was that as long as the conversion is in
> progress, the target sidtab is "owned" by the master sidtab and any
> modifications of it would be done under the master's spinlock (the
> target's spinlock wouldn't be used until it is swapped-in to be the
> new master). Did this contract change with your patch? I think this
> nested locking is the cause of the lock warning that Stephen is
> getting so hopefully you can fix it by simply removing this extra
> locking.
>
Some form of locking is necessary. Otherwise there can be concurrent
additions to the hashtable, which is not safe. My goal here is to avoid having
a single lock which is held for the duration of the conversion because that is
what caused issues previously during policy reload.
> <snip>
>
> --
> Ondrej Mosnacek <omosnace at redhat dot com>
> Software Engineer, Security Technologies
> Red Hat, Inc.
>
