* [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