[v2,bpf-next,5/6] bpf: fix potential error return

Commit Message

Anton Protopopov Nov. 19, 2024, 10:15 a.m. UTC

The bpf_remove_insns() function returns WARN_ON_ONCE(error), where
error is a result of bpf_adj_branches(), and thus should be always 0
However, if for any reason it is not 0, then it will be converted to
boolean by WARN_ON_ONCE and returned to user space as 1, not an actual
error value. Fix this by returning the original err after the WARN check.

Signed-off-by: Anton Protopopov <[e-mail]>
Acked-by: Jiri Olsa <[e-mail]>
---
 kernel/bpf/core.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

Comments

Alexei Starovoitov Nov. 26, 2024, 1:43 a.m. UTC | #1

On Tue, Nov 19, 2024 at 2:17 AM Anton Protopopov <[e-mail]> wrote:
>
> The bpf_remove_insns() function returns WARN_ON_ONCE(error), where
> error is a result of bpf_adj_branches(), and thus should be always 0
> However, if for any reason it is not 0, then it will be converted to
> boolean by WARN_ON_ONCE and returned to user space as 1, not an actual
> error value. Fix this by returning the original err after the WARN check.
>
> Signed-off-by: Anton Protopopov <[e-mail]>
> Acked-by: Jiri Olsa <[e-mail]>
> ---
>  kernel/bpf/core.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
> index 14d9288441f2..a15059918768 100644
> --- a/kernel/bpf/core.c
> +++ b/kernel/bpf/core.c
> @@ -539,6 +539,8 @@ struct bpf_prog *bpf_patch_insn_single(struct bpf_prog *prog, u32 off,
>
>  int bpf_remove_insns(struct bpf_prog *prog, u32 off, u32 cnt)
>  {
> +       int err;
> +
>         /* Branch offsets can't overflow when program is shrinking, no need
>          * to call bpf_adj_branches(..., true) here
>          */
> @@ -546,7 +548,12 @@ int bpf_remove_insns(struct bpf_prog *prog, u32 off, u32 cnt)
>                 sizeof(struct bpf_insn) * (prog->len - off - cnt));
>         prog->len -= cnt;
>
> -       return WARN_ON_ONCE(bpf_adj_branches(prog, off, off + cnt, off, false));
> +       err = bpf_adj_branches(prog, off, off + cnt, off, false);
> +       WARN_ON_ONCE(err);
> +       if (err)
> +               return err;
> +
> +       return 0;

That looks very odd. Just return err ?

Anton Protopopov Nov. 26, 2024, 4:36 p.m. UTC | #2

On 24/11/25 05:43PM, Alexei Starovoitov wrote:
> On Tue, Nov 19, 2024 at 2:17 AM Anton Protopopov <[e-mail]> wrote:
> >
> > The bpf_remove_insns() function returns WARN_ON_ONCE(error), where
> > error is a result of bpf_adj_branches(), and thus should be always 0
> > However, if for any reason it is not 0, then it will be converted to
> > boolean by WARN_ON_ONCE and returned to user space as 1, not an actual
> > error value. Fix this by returning the original err after the WARN check.
> >
> > Signed-off-by: Anton Protopopov <[e-mail]>
> > Acked-by: Jiri Olsa <[e-mail]>
> > ---
> >  kernel/bpf/core.c | 9 ++++++++-
> >  1 file changed, 8 insertions(+), 1 deletion(-)
> >
> > diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
> > index 14d9288441f2..a15059918768 100644
> > --- a/kernel/bpf/core.c
> > +++ b/kernel/bpf/core.c
> > @@ -539,6 +539,8 @@ struct bpf_prog *bpf_patch_insn_single(struct bpf_prog *prog, u32 off,
> >
> >  int bpf_remove_insns(struct bpf_prog *prog, u32 off, u32 cnt)
> >  {
> > +       int err;
> > +
> >         /* Branch offsets can't overflow when program is shrinking, no need
> >          * to call bpf_adj_branches(..., true) here
> >          */
> > @@ -546,7 +548,12 @@ int bpf_remove_insns(struct bpf_prog *prog, u32 off, u32 cnt)
> >                 sizeof(struct bpf_insn) * (prog->len - off - cnt));
> >         prog->len -= cnt;
> >
> > -       return WARN_ON_ONCE(bpf_adj_branches(prog, off, off + cnt, off, false));
> > +       err = bpf_adj_branches(prog, off, off + cnt, off, false);
> > +       WARN_ON_ONCE(err);
> > +       if (err)
> > +               return err;
> > +
> > +       return 0;
> 
> That looks very odd. Just return err ?

Ah, yes, thanks. This was supposed to be followed up by a patch which
adds code in between, but as this patch is out of scope of this set,
I will just return err here.

Patch

diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
index 14d9288441f2..a15059918768 100644
--- a/kernel/bpf/core.c
+++ b/kernel/bpf/core.c
@@ -539,6 +539,8 @@  struct bpf_prog *bpf_patch_insn_single(struct bpf_prog *prog, u32 off,
 
 int bpf_remove_insns(struct bpf_prog *prog, u32 off, u32 cnt)
 {
+	int err;
+
 	/* Branch offsets can't overflow when program is shrinking, no need
 	 * to call bpf_adj_branches(..., true) here
 	 */
@@ -546,7 +548,12 @@  int bpf_remove_insns(struct bpf_prog *prog, u32 off, u32 cnt)
 		sizeof(struct bpf_insn) * (prog->len - off - cnt));
 	prog->len -= cnt;
 
-	return WARN_ON_ONCE(bpf_adj_branches(prog, off, off + cnt, off, false));
+	err = bpf_adj_branches(prog, off, off + cnt, off, false);
+	WARN_ON_ONCE(err);
+	if (err)
+		return err;
+
+	return 0;
 }
 
 static void bpf_prog_kallsyms_del_subprogs(struct bpf_prog *fp)