On Mon, May 18, 2026 at 7:48 PM Leon Hwang <[email protected]> wrote: > > On 18/5/26 23:40, [email protected] wrote: > >> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c > >> index 83de8fb9b9aa..322865a88b3a 100644 > >> --- a/kernel/bpf/syscall.c > >> +++ b/kernel/bpf/syscall.c > >> @@ -1647,11 +1647,18 @@ static int map_create(union bpf_attr *attr, > >> bpfptr_t uattr, struct bpf_common_at > >> > >> /* preserve original error even if log finalization is successful */ > >> ret = bpf_log_attr_finalize(&attr_log, log); > >> - if (ret) { > >> - if (err >= 0) > >> - close_fd(err); > >> + if (ret && err < 0) > >> + /* > >> + * Failed to finalize the log. > >> + * Should not close_fd(err) here. Since the bpf_map_new_fd() > >> + * has published the map fd, if a concurrent thread closes the > >> + * fd, then opens new, unrelated file that receives the exact > >> + * same fd number, close_fd(err) might inadvertently close the > >> + * unrelated file. > >> + * As a trade-off, override the err only when failed to > >> finalize > >> + * the log and failed to create map. > >> + */ > >> err = ret; > > > > In the v13 review thread, Alexei Starovoitov suggested moving > > bpf_log_attr_finalize() earlier in the flow to eliminate the race > > window entirely: > > > > "bot is correct. Let's avoid these races. Pls move > > bpf_log_attr_finalize() into map_create and do it before > > security_bpf_map_create." > > > > > > https://lore.kernel.org/bpf/CAADnVQ+XR3kyqizGgGhtG5xiBu3oK3O+COUMfPDQDBSJbm=5...@mail.gmail.com/ > > > > Does the current approach fully address the race concern? While removing > > close_fd() prevents the specific file descriptor hijacking issue, the > > suggestion was to finalize the log before security_bpf_map_create(), > > which would avoid creating the race window in the first place. > > > > Is there a reason why the log finalization cannot be moved earlier as > > Alexei suggested? > > > I've tried his suggestion, attached below. > > However, I think there should be a simpler approach to fix it. > > I preferred this simple one, so I posted it. > > Should I follow the suggestion instead of this trade-off approach? > > Thanks, > Leon > > --- > > From 7ea7ab53d24b7cf34bd2775ef5c871aa312d07c6 Mon Sep 17 00:00:00 2001 > From: Leon Hwang <[email protected]> > Date: Tue, 19 May 2026 10:32:01 +0800 > Subject: [PATCH bpf-next 2/5] bpf: Fix concurrent regression in map_create() > > Because there is time gap between bpf_map_new_fd() and close_fd(), a > concurrent thread is able to close the new fd and opens a new, unrelated > file with the exact same fd number. Thereafter, this close_fd() might > inadvertently close the unrelated file. > > To avoid such regression, add bpf_log_attr_finalize() before > security_bpf_map_create() to report log_true_size on check-success path. > > While keep the original bpf_log_attr_finalize() and avoid finalizing > bpf_log_attr twice, guard the finalization using attr_log->finalized. > > Fixes: 49f9b2b2a18c ("bpf: Add syscall common attributes support for > map_create") > Signed-off-by: Leon Hwang <[email protected]> > --- > include/linux/bpf_verifier.h | 1 + > kernel/bpf/log.c | 4 ++++ > kernel/bpf/syscall.c | 21 ++++++++++++++------- > 3 files changed, 19 insertions(+), 7 deletions(-) > > diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h > index 20c421b43849..a10ef58bb6ea 100644 > --- a/include/linux/bpf_verifier.h > +++ b/include/linux/bpf_verifier.h > @@ -788,6 +788,7 @@ struct bpf_log_attr { > u32 level; > u32 offsetof_true_size; > bpfptr_t uattr; > + bool finalized; > };
No. That looks worse. The suggestion was to do the finalize log _once_ before allocating FD. Why are you doing it twice?
