| Message ID | 20241031210938.1696639-1-andrii@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [trace/for-next,1/3] bpf: put bpf_link's program when link is safe to be deallocated | expand |
On Thu, 31 Oct 2024 14:09:36 -0700 Andrii Nakryiko <andrii@kernel.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@kernel.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. -- Steve
On Fri, Nov 1, 2024 at 3:55 AM Steven Rostedt <rostedt@goodmis.org> wrote: > > On Thu, 31 Oct 2024 14:09:36 -0700 > Andrii Nakryiko <andrii@kernel.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@kernel.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
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c index a8f1808a1ca5..aa7246a399f3 100644 --- a/kernel/bpf/syscall.c +++ b/kernel/bpf/syscall.c @@ -2976,12 +2976,24 @@ void bpf_link_inc(struct bpf_link *link) atomic64_inc(&link->refcnt); } +static void bpf_link_dealloc(struct bpf_link *link) +{ + /* now that we know that bpf_link itself can't be reached, put underlying BPF program */ + if (link->prog) + bpf_prog_put(link->prog); + + /* free bpf_link and its containing memory */ + if (link->ops->dealloc_deferred) + link->ops->dealloc_deferred(link); + else + link->ops->dealloc(link); +} + static void bpf_link_defer_dealloc_rcu_gp(struct rcu_head *rcu) { struct bpf_link *link = container_of(rcu, struct bpf_link, rcu); - /* free bpf_link and its containing memory */ - link->ops->dealloc_deferred(link); + bpf_link_dealloc(link); } static void bpf_link_defer_dealloc_mult_rcu_gp(struct rcu_head *rcu) @@ -3003,7 +3015,6 @@ static void bpf_link_free(struct bpf_link *link) sleepable = link->prog->sleepable; /* detach BPF program, clean up used resources */ ops->release(link); - bpf_prog_put(link->prog); } if (ops->dealloc_deferred) { /* schedule BPF link deallocation; if underlying BPF program @@ -3014,8 +3025,9 @@ static void bpf_link_free(struct bpf_link *link) call_rcu_tasks_trace(&link->rcu, bpf_link_defer_dealloc_mult_rcu_gp); else call_rcu(&link->rcu, bpf_link_defer_dealloc_rcu_gp); - } else if (ops->dealloc) - ops->dealloc(link); + } else if (ops->dealloc) { + bpf_link_dealloc(link); + } } static void bpf_link_put_deferred(struct work_struct *work)
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@kernel.org> --- kernel/bpf/syscall.c | 22 +++++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-)