| Message ID | 20240122164936.810117-2-aspsk@isovalent.com (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | BPF |
| Headers | show |
| Series | static branches | expand |
On Mon, Jan 22, 2024 at 04:49:32PM +0000, Anton Protopopov 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 <aspsk@isovalent.com> nice catch Acked-by: Jiri Olsa <jolsa@kernel.org> > --- > 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 fbb1d95a9b44..9ba9e0ea9c45 100644 > --- a/kernel/bpf/core.c > +++ b/kernel/bpf/core.c > @@ -532,6 +532,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 > */ > @@ -539,7 +541,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; could be just 'return err' jirka > } > > static void bpf_prog_kallsyms_del_subprogs(struct bpf_prog *fp) > -- > 2.34.1 >
On Tue, Jan 30, 2024 at 12:20:06PM +0100, Jiri Olsa wrote: > On Mon, Jan 22, 2024 at 04:49:32PM +0000, Anton Protopopov 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 <aspsk@isovalent.com> > > nice catch > > Acked-by: Jiri Olsa <jolsa@kernel.org> > > > --- > > 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 fbb1d95a9b44..9ba9e0ea9c45 100644 > > --- a/kernel/bpf/core.c > > +++ b/kernel/bpf/core.c > > @@ -532,6 +532,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 > > */ > > @@ -539,7 +541,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; > > could be just 'return err' Thanks. I am inserting some code in a consequent patch in between, so left this in this form > jirka > > > } > > > > static void bpf_prog_kallsyms_del_subprogs(struct bpf_prog *fp) > > -- > > 2.34.1 > >
diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c index fbb1d95a9b44..9ba9e0ea9c45 100644 --- a/kernel/bpf/core.c +++ b/kernel/bpf/core.c @@ -532,6 +532,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 */ @@ -539,7 +541,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)
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 <aspsk@isovalent.com> --- kernel/bpf/core.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)