On Tue, Aug 30, 2022 at 10:21:30AM +0800, Hou Tao wrote:
> 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.
hmm... iiuc, it is fine for the preempted by IRQ case because the
operations won't be interleaved as the process A/B example above?
That should also be true for the task-storage case here,
meaning only CONFIG_PREEMPT can reproduce it? If that is the
case, please also mention that in the commit message.
