netdev - Re: [PATCH bpf-next 1/3] bpf: Introduce bpf

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] [day] [month] [year] [list]

Message-ID: <CAADnVQJiXNU7O=56U-5RP0MycY4knzi556rzdoBHKp_dZrzZ4A@mail.gmail.com>
Date:   Fri, 4 Jun 2021 11:16:31 -0700
From:   Alexei Starovoitov <alexei.starovoitov@...il.com>
To:     Andrii Nakryiko <andrii.nakryiko@...il.com>
Cc:     "David S. Miller" <davem@...emloft.net>,
        Daniel Borkmann <daniel@...earbox.net>,
        Andrii Nakryiko <andrii@...nel.org>,
        Networking <netdev@...r.kernel.org>, bpf <bpf@...r.kernel.org>,
        Kernel Team <kernel-team@...com>
Subject: Re: [PATCH bpf-next 1/3] bpf: Introduce bpf_timer

On Thu, Jun 3, 2021 at 9:17 PM Andrii Nakryiko
<andrii.nakryiko@...il.com> wrote:
> >
> > > So your idea is to cmpxchg() to NULL while bpf_timer_start() or
> > > bpf_timer_cancel() works with the timer? Wouldn't that cause
> > > bpf_timer_init() believe that that timer is not yet initialized and
> > > not return -EBUSY. Granted that's a corner-case race, but still.
> >
> > Not following.
> > bpf prog should do bpf_timer_init only once.
> > bpf_timer_init after bpf_timer_cancel is a wrong usage.
> > hrtimer api doesn't have any protection for such use.
> > while bpf_timer_init returns EBUSY.
> > 2nd bpf_timer_init is just a misuse of bpf_timer api.
>
> Yes, clearly a bad use of API, but it's not prevented by verifier.

not prevented only because it's hard to do in the verifier.

> > > > Currently thinking to do cmpxchg in bpf_timer_start() and
> > > > bpf_timer_cancel*() similar to bpf_timer_init() to address it.
>
> because that seemed like you were going to exchange (temporarily) a
> pointer to NULL while doing bpf_timer_start() or bpf_timer_cancel(),
> and then setting NULL -> valid ptr back again (this sequence would
> open up a window when bpf_timer_init() can be used twice on the same
> element). But again, with spinlock embedded doesn't matter anymore.

Right, except bpf_timer_start and bpf_timer_cancel would xchg with -1 or similar
and bpf_timer_init won't get confused.
If two bpf_timer_start()s race on the same timer one would receive
-EMISUSEOFAPI right away.
Whereas with spin_lock inside bpf_timer both will be serialized and
both will succeed.
One can argue that bpf_timer_start and bpf_timer_cancel on different cpus
is a realistic scenario. So xchg approach would need two special
pointers -1 and -2
to distinguish start/start bad race vs start/cancel good race.
And everything gets too clever. spin_lock is "obviously correct",
though it doesn't have an advantage of informing users of api misuse.
I coded it up and it's surviving the tests so far :)