* [PATCH bpf-next] libbpf: fix errno is overwritten after being closed. @ 2022-12-23 13:36 Xin Liu 2022-12-28 22:20 ` patchwork-bot+netdevbpf 2022-12-29 21:43 ` Andrii Nakryiko 0 siblings, 2 replies; 6+ messages in thread From: Xin Liu @ 2022-12-23 13:36 UTC (permalink / raw) To: andrii, ast, daniel, martin.lau, song, yhs, john.fastabend, kpsingh, sdf, haoluo, jolsa Cc: bpf, linux-kernel, yanan, wuchangye, xiesongyang, kongweibin2, liuxin350, zhangmingyi5 In the ensure_good_fd function, if the fcntl function succeeds but the close function fails, ensure_good_fd returns a normal fd and sets errno, which may cause users to misunderstand. The close failure is not a serious problem, and the correct FD has been handed over to the upper-layer application. Let's restore errno here. Signed-off-by: Xin Liu <liuxin350@huawei.com> --- tools/lib/bpf/libbpf_internal.h | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/tools/lib/bpf/libbpf_internal.h b/tools/lib/bpf/libbpf_internal.h index 377642ff51fc..98333a6c38e9 100644 --- a/tools/lib/bpf/libbpf_internal.h +++ b/tools/lib/bpf/libbpf_internal.h @@ -543,10 +543,9 @@ static inline int ensure_good_fd(int fd) fd = fcntl(fd, F_DUPFD_CLOEXEC, 3); saved_errno = errno; close(old_fd); - if (fd < 0) { + errno = saved_errno; + if (fd < 0) pr_warn("failed to dup FD %d to FD > 2: %d\n", old_fd, -saved_errno); - errno = saved_errno; - } } return fd; } -- 2.33.0 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH bpf-next] libbpf: fix errno is overwritten after being closed. 2022-12-23 13:36 [PATCH bpf-next] libbpf: fix errno is overwritten after being closed Xin Liu @ 2022-12-28 22:20 ` patchwork-bot+netdevbpf 2022-12-29 21:43 ` Andrii Nakryiko 1 sibling, 0 replies; 6+ messages in thread From: patchwork-bot+netdevbpf @ 2022-12-28 22:20 UTC (permalink / raw) To: Xin Liu Cc: andrii, ast, daniel, martin.lau, song, yhs, john.fastabend, kpsingh, sdf, haoluo, jolsa, bpf, linux-kernel, yanan, wuchangye, xiesongyang, kongweibin2, zhangmingyi5 Hello: This patch was applied to bpf/bpf-next.git (master) by Alexei Starovoitov <ast@kernel.org>: On Fri, 23 Dec 2022 21:36:18 +0800 you wrote: > In the ensure_good_fd function, if the fcntl function succeeds but > the close function fails, ensure_good_fd returns a normal fd and > sets errno, which may cause users to misunderstand. The close > failure is not a serious problem, and the correct FD has been > handed over to the upper-layer application. Let's restore errno here. > > Signed-off-by: Xin Liu <liuxin350@huawei.com> > > [...] Here is the summary with links: - [bpf-next] libbpf: fix errno is overwritten after being closed. https://git.kernel.org/bpf/bpf-next/c/07453245620c You are awesome, thank you! -- Deet-doot-dot, I am a bot. https://korg.docs.kernel.org/patchwork/pwbot.html ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH bpf-next] libbpf: fix errno is overwritten after being closed. 2022-12-23 13:36 [PATCH bpf-next] libbpf: fix errno is overwritten after being closed Xin Liu 2022-12-28 22:20 ` patchwork-bot+netdevbpf @ 2022-12-29 21:43 ` Andrii Nakryiko 2022-12-29 21:49 ` Alexei Starovoitov 1 sibling, 1 reply; 6+ messages in thread From: Andrii Nakryiko @ 2022-12-29 21:43 UTC (permalink / raw) To: Xin Liu Cc: andrii, ast, daniel, martin.lau, song, yhs, john.fastabend, kpsingh, sdf, haoluo, jolsa, bpf, linux-kernel, yanan, wuchangye, xiesongyang, kongweibin2, zhangmingyi5 On Fri, Dec 23, 2022 at 5:36 AM Xin Liu <liuxin350@huawei.com> wrote: > > In the ensure_good_fd function, if the fcntl function succeeds but > the close function fails, ensure_good_fd returns a normal fd and > sets errno, which may cause users to misunderstand. The close > failure is not a serious problem, and the correct FD has been > handed over to the upper-layer application. Let's restore errno here. > > Signed-off-by: Xin Liu <liuxin350@huawei.com> > --- > tools/lib/bpf/libbpf_internal.h | 5 ++--- > 1 file changed, 2 insertions(+), 3 deletions(-) > > diff --git a/tools/lib/bpf/libbpf_internal.h b/tools/lib/bpf/libbpf_internal.h > index 377642ff51fc..98333a6c38e9 100644 > --- a/tools/lib/bpf/libbpf_internal.h > +++ b/tools/lib/bpf/libbpf_internal.h > @@ -543,10 +543,9 @@ static inline int ensure_good_fd(int fd) > fd = fcntl(fd, F_DUPFD_CLOEXEC, 3); > saved_errno = errno; > close(old_fd); > - if (fd < 0) { > + errno = saved_errno; > + if (fd < 0) > pr_warn("failed to dup FD %d to FD > 2: %d\n", old_fd, -saved_errno); > - errno = saved_errno; pr_warn calls into user-provided callback, which can clobber errno, so `errno = saved_errno` should happen after pr_warn. With your change there is even higher chance of errno clobbering. Please send a follow up fix to unconditionally restore errno *after* pr_warn, thanks. > - } > } > return fd; > } > -- > 2.33.0 > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH bpf-next] libbpf: fix errno is overwritten after being closed. 2022-12-29 21:43 ` Andrii Nakryiko @ 2022-12-29 21:49 ` Alexei Starovoitov 2022-12-29 23:21 ` Andrii Nakryiko 0 siblings, 1 reply; 6+ messages in thread From: Alexei Starovoitov @ 2022-12-29 21:49 UTC (permalink / raw) To: Andrii Nakryiko Cc: Xin Liu, Andrii Nakryiko, Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend, KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa, bpf, LKML, yanan, wuchangye, xiesongyang, kongweibin2, zhangmingyi5 On Thu, Dec 29, 2022 at 1:44 PM Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > On Fri, Dec 23, 2022 at 5:36 AM Xin Liu <liuxin350@huawei.com> wrote: > > > > In the ensure_good_fd function, if the fcntl function succeeds but > > the close function fails, ensure_good_fd returns a normal fd and > > sets errno, which may cause users to misunderstand. The close > > failure is not a serious problem, and the correct FD has been > > handed over to the upper-layer application. Let's restore errno here. > > > > Signed-off-by: Xin Liu <liuxin350@huawei.com> > > --- > > tools/lib/bpf/libbpf_internal.h | 5 ++--- > > 1 file changed, 2 insertions(+), 3 deletions(-) > > > > diff --git a/tools/lib/bpf/libbpf_internal.h b/tools/lib/bpf/libbpf_internal.h > > index 377642ff51fc..98333a6c38e9 100644 > > --- a/tools/lib/bpf/libbpf_internal.h > > +++ b/tools/lib/bpf/libbpf_internal.h > > @@ -543,10 +543,9 @@ static inline int ensure_good_fd(int fd) > > fd = fcntl(fd, F_DUPFD_CLOEXEC, 3); > > saved_errno = errno; > > close(old_fd); > > - if (fd < 0) { > > + errno = saved_errno; > > + if (fd < 0) > > pr_warn("failed to dup FD %d to FD > 2: %d\n", old_fd, -saved_errno); > > - errno = saved_errno; > > pr_warn calls into user-provided callback, which can clobber errno, so > `errno = saved_errno` should happen after pr_warn. With your change > there is even higher chance of errno clobbering. > > Please send a follow up fix to unconditionally restore errno *after* > pr_warn, thanks. Good point. I can follow up with one line fix too. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH bpf-next] libbpf: fix errno is overwritten after being closed. 2022-12-29 21:49 ` Alexei Starovoitov @ 2022-12-29 23:21 ` Andrii Nakryiko 2022-12-30 3:20 ` Alexei Starovoitov 0 siblings, 1 reply; 6+ messages in thread From: Andrii Nakryiko @ 2022-12-29 23:21 UTC (permalink / raw) To: Alexei Starovoitov Cc: Xin Liu, Andrii Nakryiko, Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend, KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa, bpf, LKML, yanan, wuchangye, xiesongyang, kongweibin2, zhangmingyi5 On Thu, Dec 29, 2022 at 1:49 PM Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: > > On Thu, Dec 29, 2022 at 1:44 PM Andrii Nakryiko > <andrii.nakryiko@gmail.com> wrote: > > > > On Fri, Dec 23, 2022 at 5:36 AM Xin Liu <liuxin350@huawei.com> wrote: > > > > > > In the ensure_good_fd function, if the fcntl function succeeds but > > > the close function fails, ensure_good_fd returns a normal fd and > > > sets errno, which may cause users to misunderstand. The close > > > failure is not a serious problem, and the correct FD has been > > > handed over to the upper-layer application. Let's restore errno here. > > > > > > Signed-off-by: Xin Liu <liuxin350@huawei.com> > > > --- > > > tools/lib/bpf/libbpf_internal.h | 5 ++--- > > > 1 file changed, 2 insertions(+), 3 deletions(-) > > > > > > diff --git a/tools/lib/bpf/libbpf_internal.h b/tools/lib/bpf/libbpf_internal.h > > > index 377642ff51fc..98333a6c38e9 100644 > > > --- a/tools/lib/bpf/libbpf_internal.h > > > +++ b/tools/lib/bpf/libbpf_internal.h > > > @@ -543,10 +543,9 @@ static inline int ensure_good_fd(int fd) > > > fd = fcntl(fd, F_DUPFD_CLOEXEC, 3); > > > saved_errno = errno; > > > close(old_fd); > > > - if (fd < 0) { > > > + errno = saved_errno; > > > + if (fd < 0) > > > pr_warn("failed to dup FD %d to FD > 2: %d\n", old_fd, -saved_errno); > > > - errno = saved_errno; > > > > pr_warn calls into user-provided callback, which can clobber errno, so > > `errno = saved_errno` should happen after pr_warn. With your change > > there is even higher chance of errno clobbering. > > > > Please send a follow up fix to unconditionally restore errno *after* > > pr_warn, thanks. > > Good point. I can follow up with one line fix too. that would be simplest, probably, thanks! ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH bpf-next] libbpf: fix errno is overwritten after being closed. 2022-12-29 23:21 ` Andrii Nakryiko @ 2022-12-30 3:20 ` Alexei Starovoitov 0 siblings, 0 replies; 6+ messages in thread From: Alexei Starovoitov @ 2022-12-30 3:20 UTC (permalink / raw) To: Andrii Nakryiko Cc: Xin Liu, Andrii Nakryiko, Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend, KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa, bpf, LKML, yanan, wuchangye, xiesongyang, kongweibin2, zhangmingyi5 On Thu, Dec 29, 2022 at 3:21 PM Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > On Thu, Dec 29, 2022 at 1:49 PM Alexei Starovoitov > <alexei.starovoitov@gmail.com> wrote: > > > > On Thu, Dec 29, 2022 at 1:44 PM Andrii Nakryiko > > <andrii.nakryiko@gmail.com> wrote: > > > > > > On Fri, Dec 23, 2022 at 5:36 AM Xin Liu <liuxin350@huawei.com> wrote: > > > > > > > > In the ensure_good_fd function, if the fcntl function succeeds but > > > > the close function fails, ensure_good_fd returns a normal fd and > > > > sets errno, which may cause users to misunderstand. The close > > > > failure is not a serious problem, and the correct FD has been > > > > handed over to the upper-layer application. Let's restore errno here. > > > > > > > > Signed-off-by: Xin Liu <liuxin350@huawei.com> > > > > --- > > > > tools/lib/bpf/libbpf_internal.h | 5 ++--- > > > > 1 file changed, 2 insertions(+), 3 deletions(-) > > > > > > > > diff --git a/tools/lib/bpf/libbpf_internal.h b/tools/lib/bpf/libbpf_internal.h > > > > index 377642ff51fc..98333a6c38e9 100644 > > > > --- a/tools/lib/bpf/libbpf_internal.h > > > > +++ b/tools/lib/bpf/libbpf_internal.h > > > > @@ -543,10 +543,9 @@ static inline int ensure_good_fd(int fd) > > > > fd = fcntl(fd, F_DUPFD_CLOEXEC, 3); > > > > saved_errno = errno; > > > > close(old_fd); > > > > - if (fd < 0) { > > > > + errno = saved_errno; > > > > + if (fd < 0) > > > > pr_warn("failed to dup FD %d to FD > 2: %d\n", old_fd, -saved_errno); > > > > - errno = saved_errno; > > > > > > pr_warn calls into user-provided callback, which can clobber errno, so > > > `errno = saved_errno` should happen after pr_warn. With your change > > > there is even higher chance of errno clobbering. > > > > > > Please send a follow up fix to unconditionally restore errno *after* > > > pr_warn, thanks. > > > > Good point. I can follow up with one line fix too. > > that would be simplest, probably, thanks! Pushed trivial fix to bpf-next. ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-12-30 3:20 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2022-12-23 13:36 [PATCH bpf-next] libbpf: fix errno is overwritten after being closed Xin Liu 2022-12-28 22:20 ` patchwork-bot+netdevbpf 2022-12-29 21:43 ` Andrii Nakryiko 2022-12-29 21:49 ` Alexei Starovoitov 2022-12-29 23:21 ` Andrii Nakryiko 2022-12-30 3:20 ` Alexei Starovoitov
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox