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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 28 Jun 2021 18:39:55 -0700
From:   Alexei Starovoitov <alexei.starovoitov@...il.com>
To:     Yonghong Song <yhs@...com>
Cc:     Alexei Starovoitov <ast@...com>, davem@...emloft.net,
        daniel@...earbox.net, andrii@...nel.org, netdev@...r.kernel.org,
        bpf@...r.kernel.org, kernel-team@...com
Subject: 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:
> > On 6/24/21 11:25 PM, Yonghong Song wrote:
> > > 
> > > > +
> > > > +    ____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.

> > 
> > > 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.