Re: [tip:locking/urgent] locking/static_key: Fix concurrent static_key_slow_inc()

[Dima Zavin]

On Mon, Jul 31, 2017 at 6:33 AM, Peter Zijlstra <peterz@xxxxxxxxxxxxx> wrote:
> On Mon, Jul 31, 2017 at 03:04:06PM +0200, Paolo Bonzini wrote:
>> - key->enabled cannot go from 0 to nonzero outside jump_label_mutex.
>> For this reason the atomic_try_cmpxchg is unnecessary.
>
> Agreed, the only reason was the implied barrier.
>
>> - the (implied) smp_mb before jump_label_update is not interesting, but
>> I don't think it is useful because: 1) during the jump_label_update
>> there is no correspondence between what static_key_enabled returns and
>> what the text look like; 2) what would it even be pairing with?
>
> Ah, indeed. So I was worried about the text changes escaping upwards,
> but you're right in that there's no harm in that because there's nothing
> that cares.
>
> Another inc would see 0 and still serialize on the mutex.
>
>> - the smp_mb (though it could be a smp_wmb or atomic_set_release)
>> initially triggered my paranoia indeed.  But even then, I can't see why
>> you would need it because there's nothing it pairs with.
>
> So this one would pair with the mb implied by the cmpxchg loop for
> inc-if-positive. The ordering being that if we see a positive v, we must
> then also see all the text modifications.
>
> So if jump_label_update() were to not fully serialize things, it would
> be possible for the v=1 store to appear before the last text changes.
> And in that case we'd allow the fast path to complete
> static_key_slow_inc() before it was in fact done with changing all text.
>
> Now, I suspect (but did not audit) that anything that changes text must
> in fact serialize world, but I wasn't sure.
>
>> Rather, it's *any use of key->enabled outside jump_label_lock*
>> (meaning: any use of static_key_enabled and static_key_count outside
>> the core jump_label.c code) that should be handled with care.
>
>> And indeed, while there aren't many, I think two of them are wrong (and
>> not fixed by your patch):
>>
>> - include/linux/cpuset.h defines nr_cpusets which uses static_key_count.
>>  It makes no sense to call it outside cpuset_mutex, and indeed that's
>> how kernel/cgroup/cpuset.c uses it (nr_cpusets <- generate_sched_domains
>> <- rebuild_sched_domains_locked).  The function should be moved inside
>> kernel/cgroup/cpuset.c since the mutex is static.
>
> Dima was poking at that code.

As I understand it, in the case of read_mems_allowed, the value of the
key enabled count itself is not used. We just use the branch rewrite
to avoid seqcount lookups, so only the text changes actually matter
there. We'd still need my fix since the problem was ordering of the
text changes.

Thanks!

--Dima

>
>> - net/ipv4/udp.c and net/ipv6/udp.c want to implement a "once-only"
>> increment of the static key.  It's racy and maybe we should provide a
>> new API static_key_enable_forever:
>>
>>       void static_key_enable_forever(struct static_key *key)
>>       {
>>               STATIC_KEY_CHECK_USE();
>>               if (atomic_read(&key->enabled) > 0)
>>                       return;
>>
>>               cpus_read_lock();
>>               jump_label_lock();
>>               if (atomic_read(&key->enabled) == 0) {
>>                       atomic_set(&key->enabled, -1);
>>                       jump_label_update(key);
>>                       atomic_set(&key->enabled, 1);
>>               }
>>               jump_label_unlock();
>>               cpus_read_unlock();
>>       }
>>       EXPORT_SYMBOL_GPL(static_key_enable_forever);
>>
>> I can prepare a patch if you agree.
>
> Isn't that what we have static_key_enable() for? Which btw also uses
> static_key_count() outside of the mutex.