Hi,
On 8/30/2022 8:52 AM, Martin KaFai Lau wrote:
> On Mon, Aug 29, 2022 at 10:27:50PM +0800, Hou Tao wrote:
>> From: Hou Tao <houtao1@xxxxxxxxxx>
>>
>> Now migrate_disable() does not disable preemption and under some
>> architecture (e.g. arm64) __this_cpu_{inc|dec|inc_return} are neither
>> preemption-safe nor IRQ-safe, so the concurrent lookups or updates on
>> the same task local storage and the same CPU may make
>> bpf_task_storage_busy be imbalanced, and bpf_task_storage_trylock()
>> will always fail.
>>
>> Fixing it by using this_cpu_{inc|dec|inc_return} when manipulating
>> bpf_task_storage_busy.
SNIP
>> static bool bpf_task_storage_trylock(void)
>> {
>> migrate_disable();
>> - if (unlikely(__this_cpu_inc_return(bpf_task_storage_busy) != 1)) {
>> - __this_cpu_dec(bpf_task_storage_busy);
>> + if (unlikely(this_cpu_inc_return(bpf_task_storage_busy) != 1)) {
>> + this_cpu_dec(bpf_task_storage_busy);
> This change is only needed here but not in the htab fix [0]
> or you are planning to address it separately ?
>
> [0]: https://lore.kernel.org/bpf/20220829023709.1958204-2-houtao@xxxxxxxxxxxxxxx/
> .
For htab_lock_bucket() in hash-table, in theory there will be problem as shown
below, but I can not reproduce it on ARM64 host:
*p = 0
// process A
r0 = *p
r0 += 1
// process B
r1 = *p
// *p = 1
*p = r0
r1 += 1
// *p = 1
*p = r1
// r0 = 1
r0 = *p
// r1 = 1
r1 = *p
In hash table fixes, migrate_disable() in htab_lock_bucket() is replaced by
preempt_disable(), so the above case will be impossible. And if process A is
preempted by IRQ, __this_cpu_inc_return will be OK.
Beside bpf hash-table, there are also other __this_cpu_inc_return users in bpf
(e.g. __bpf_prog_enter), maybe I should fix these usage as well ?
