Re: [PATCH v3 bpf-next 1/8] bpf: Introduce bpf timers.

On Fri, Jun 25, 2021 at 08:54:55AM -0700, Yonghong Song wrote:

On 6/25/21 7:57 AM, Alexei Starovoitov wrote:

quoted

On 6/24/21 11:25 PM, Yonghong Song wrote:

quoted

quoted

+
+    ____bpf_spin_lock(&timer->lock);

I think we may still have some issues.
Case 1:
   1. one bpf program is running in process context,
      bpf_timer_start() is called and timer->lock is taken
   2. timer softirq is triggered and this callback is called

___bpf_spin_lock is actually irqsave version of spin_lock.
So this race is not possible.

Sorry I missed that ____bpf_spin_lock() has local_irq_save(),
so yes. the above situation cannot happen.

Yeah. It was confusing. I'll add a comment.

quoted

quoted

Case 2:
   1. this callback is called, timer->lock is taken
   2. a nmi happens and some bpf program is called (kprobe, tracepoint,
      fentry/fexit or perf_event, etc.) and that program calls
      bpf_timer_start()

So we could have deadlock in both above cases?

Shouldn't be possible either because bpf timers are not allowed
in nmi-bpf-progs. I'll double check that it's the case.
Pretty much the same restrictions are with bpf_spin_lock.

The patch added bpf_base_func_proto() to bpf_tracing_func_proto:

Also, we have some functions inside ____bpf_spin_lock() e.g.,
bpf_prog_inc(), hrtimer_start(), etc. If we want to be absolutely safe,
we need to mark them not tracable for kprobe/kretprobe/fentry/fexit/...
But I am not sure whether this is really needed or not.

Probably not.
I'll add in_nmi() runtime check to prevent nmi and kprobes.