netdev - Re: [PATCH v2 bpf-next 02/10] bpf: allocate ID for 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] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAEf4Bza-gqQHz3_9RyX7pKo_2kYeh7cCmNRAxExx48JQdOpfDQ@mail.gmail.com>
Date:   Tue, 28 Apr 2020 11:56:52 -0700
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     Alexei Starovoitov <alexei.starovoitov@...il.com>
Cc:     Andrii Nakryiko <andriin@...com>, bpf <bpf@...r.kernel.org>,
        Networking <netdev@...r.kernel.org>,
        Alexei Starovoitov <ast@...com>,
        Daniel Borkmann <daniel@...earbox.net>,
        Kernel Team <kernel-team@...com>
Subject: Re: [PATCH v2 bpf-next 02/10] bpf: allocate ID for bpf_link

On Tue, Apr 28, 2020 at 10:31 AM Alexei Starovoitov
<alexei.starovoitov@...il.com> wrote:
>
> On Mon, Apr 27, 2020 at 10:49:36PM -0700, Andrii Nakryiko wrote:
> > +int bpf_link_settle(struct bpf_link_primer *primer)
> > +{
> > +     /* make bpf_link fetchable by ID */
> > +     WRITE_ONCE(primer->link->id, primer->id);
>
> what does WRITE_ONCE serve here?

To prevent compiler reordering this write with fd_install. So that by
the time FD is exposed to user-space, link has properly set ID.

> bpf_link_settle can only be called at the end of attach.
> If attach is slow than parallel get_fd_by_id can get an new FD
> instance for link with zero id.
> In such case deref of link->id will race with above assignment?

Yes, it does race, but it can either see zero and assume bpf_link is
not ready (which is fine to do) or will see correct link ID and will
proceed to create new FD for it. By the time we do context switch back
to user-space and return link FD, ID will definitely be visible due to
context switch and associated memory barriers. If anyone is guessing
FD and trying to create FD_BY_ID before LINK_CREATE syscall returns --
then returning failure due to link ID not yet set is totally fine,
IMO.

> But I don't see READ_ONCE in patch 3.
> It's under link_idr_lock there.

It doesn't need READ_ONCE because it does read under spinlock, so
compiler can't re-order it with code outside of spinlock.

> How about grabbing link_idr_lock here as well ?
> otherwise it's still racy since WRITE_ONCE is not paired.

As indicated above, seems unnecessary? But I also don't object
strongly, I don't expect this lock for links to be a major bottleneck
or anything like that.

>
> The mix of spin_lock_irqsave(&link_idr_lock)
> and spin_lock_bh(&link_idr_lock) looks weird.
> We do the same for map_idr because maps have complicated freeing logic,
> but prog_idr is consistent.
> If you see the need for irqsave variant then please use it in all cases.

No, my bad, I don't see any need to intermix them. I'll stick to
spin_lock_bh, thanks for catching!