diff mbox series

[bpf-next] libbpf: fix errno is overwritten after being closed.

Message ID 20221223133618.10323-1-liuxin350@huawei.com (mailing list archive)
State Accepted
Commit 07453245620c075779abefa2a9f469fa336e7510
Delegated to: BPF
Headers show
Series [bpf-next] libbpf: fix errno is overwritten after being closed. | expand

Checks

Context Check Description
netdev/tree_selection success Clearly marked for bpf-next
netdev/fixes_present success Fixes tag not required for -next series
netdev/subject_prefix success Link
netdev/cover_letter success Single patches do not need cover letters
netdev/patch_count success Link
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/cc_maintainers success CCed 12 of 12 maintainers
netdev/build_clang success Errors and warnings before: 0 this patch: 0
netdev/module_param success Was 0 now: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 0 this patch: 0
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 12 lines checked
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
bpf/vmtest-bpf-next-VM_Test-1 success Logs for ShellCheck
bpf/vmtest-bpf-next-VM_Test-2 success Logs for build for aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-3 success Logs for build for aarch64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-5 success Logs for build for x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-6 success Logs for build for x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-7 success Logs for llvm-toolchain
bpf/vmtest-bpf-next-VM_Test-8 success Logs for set-matrix
bpf/vmtest-bpf-next-VM_Test-4 success Logs for build for s390x with gcc
bpf/vmtest-bpf-next-VM_Test-9 success Logs for test_maps on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-10 success Logs for test_maps on aarch64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-12 success Logs for test_maps on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-13 success Logs for test_maps on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-14 fail Logs for test_progs on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-15 success Logs for test_progs on aarch64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-17 success Logs for test_progs on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-18 success Logs for test_progs on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-19 success Logs for test_progs_no_alu32 on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-20 success Logs for test_progs_no_alu32 on aarch64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-22 success Logs for test_progs_no_alu32 on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-23 success Logs for test_progs_no_alu32 on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-24 success Logs for test_progs_no_alu32_parallel on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-25 success Logs for test_progs_no_alu32_parallel on aarch64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-27 success Logs for test_progs_no_alu32_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-28 success Logs for test_progs_no_alu32_parallel on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-29 success Logs for test_progs_parallel on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-32 success Logs for test_progs_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-33 success Logs for test_progs_parallel on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-34 success Logs for test_verifier on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-35 success Logs for test_verifier on aarch64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-37 success Logs for test_verifier on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-38 success Logs for test_verifier on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-30 success Logs for test_progs_parallel on aarch64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-21 success Logs for test_progs_no_alu32 on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-36 success Logs for test_verifier on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-26 success Logs for test_progs_no_alu32_parallel on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-31 success Logs for test_progs_parallel on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-16 success Logs for test_progs on s390x with gcc
bpf/vmtest-bpf-next-PR fail PR summary
bpf/vmtest-bpf-next-VM_Test-11 success Logs for test_maps on s390x with gcc

Commit Message

Xin Liu Dec. 23, 2022, 1:36 p.m. UTC 
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(-)

Comments

patchwork-bot+netdevbpf@kernel.org Dec. 28, 2022, 10:20 p.m. UTC | #1 
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!
Andrii Nakryiko Dec. 29, 2022, 9:43 p.m. UTC | #2 
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
>
Alexei Starovoitov Dec. 29, 2022, 9:49 p.m. UTC | #3 
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.
Andrii Nakryiko Dec. 29, 2022, 11:21 p.m. UTC | #4 
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!
Alexei Starovoitov Dec. 30, 2022, 3:20 a.m. UTC | #5 
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.
diff mbox series

Patch

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;
 }