linux-kernel - Re: [PATCH trace/for-next 1/3] bpf: put bpf_link's program when link is safe to be deallocated

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: <CAEf4BzZrYM6HdoxLB1ru7SV6rH_-LTkS7JC0qCyUZSz-LW-wsg@mail.gmail.com>
Date: Fri, 1 Nov 2024 10:54:40 -0700
From: Andrii Nakryiko <andrii.nakryiko@...il.com>
To: Steven Rostedt <rostedt@...dmis.org>
Cc: Andrii Nakryiko <andrii@...nel.org>, linux-trace-kernel@...r.kernel.org, 
	bpf@...r.kernel.org, ast@...nel.org, daniel@...earbox.net, 
	martin.lau@...nel.org, mathieu.desnoyers@...icios.com, 
	linux-kernel@...r.kernel.org, mhiramat@...nel.org, peterz@...radead.org, 
	paulmck@...nel.org, jrife@...gle.com
Subject: Re: [PATCH trace/for-next 1/3] bpf: put bpf_link's program when link
 is safe to be deallocated

On Fri, Nov 1, 2024 at 3:55 AM Steven Rostedt <rostedt@...dmis.org> wrote:
>
> On Thu, 31 Oct 2024 14:09:36 -0700
> Andrii Nakryiko <andrii@...nel.org> wrote:
>
> > In general, BPF link's underlying BPF program should be considered to be
> > reachable through attach hook -> link -> prog chain, and, pessimistically,
> > we have to assume that as long as link's memory is not safe to free,
> > attach hook's code might hold a pointer to BPF program and use it.
> >
> > As such, it's not (generally) correct to put link's program early before
> > waiting for RCU GPs to go through. More eager bpf_prog_put() that we
> > currently do is mostly correct due to BPF program's release code doing
> > similar RCU GP waiting, but as will be shown in the following patches,
> > BPF program can be non-sleepable (and, thus, reliant on only "classic"
> > RCU GP), while BPF link's attach hook can have sleepable semantics and
> > needs to be protected by RCU Tasks Trace, and for such cases BPF link
> > has to go through RCU Tasks Trace + "classic" RCU GPs before being
> > deallocated. And so, if we put BPF program early, we might free BPF
> > program before we free BPF link, leading to use-after-free situation.
> >
> > So, this patch defers bpf_prog_put() until we are ready to perform
> > bpf_link's deallocation. At worst, this delays BPF program freeing by
> > one extra RCU GP, but that seems completely acceptable. Alternatively,
> > we'd need more elaborate ways to determine BPF hook, BPF link, and BPF
> > program lifetimes, and how they relate to each other, which seems like
> > an unnecessary complication.
> >
> > Note, for most BPF links we still will perform eager bpf_prog_put() and
> > link dealloc, so for those BPF links there are no observable changes
> > whatsoever. Only BPF links that use deferred dealloc might notice
> > slightly delayed freeing of BPF programs.
> >
> > Also, to reduce code and logic duplication, extract program put + link
> > dealloc logic into bpf_link_dealloc() helper.
> >
> > Signed-off-by: Andrii Nakryiko <andrii@...nel.org>
>
>
> Hi Andrii,
>
> Do you want me to add this on top of my queue? If so, would it be possible
> that I can get a tested-by from someone? As I don't do much to test BPF
> patches.

Hey Steven,

Yep, this should go on top of Mathieu's patch set. Let me send v2 with
fixed up stub definition. Jordan gave his Tested-By and Alexei seems
fine with this as well, so I think it should be good to go.

>
> -- Steve