[bpf-next,v2,0/2] Use overflow.h helpers to check for overflows

Message

Shung-Hsi Yu July 1, 2024, 5:59 a.m. UTC

This patch set refactors kernel/bpf/verifier.c to use type-agnostic,
generic overflow-check helpers defined in include/linux/overflow.h to
check for addition and subtraction overflow, and drop the
signed_*_overflows() helpers we currently have in kernel/bpf/verifier.c.
There should be no functional change in how the verifier works.

The main motivation is to make future refactoring[1] easier.

While check_mul_overflow() also exists and could potentially replace what
we have in scalar*_min_max_mul(), it does not help with refactoring and
would either change how the verifier works (e.g. lifting restriction on
umax<=U32_MAX and u32_max<=U16_MAX) or make the code slightly harder to
read, so it is left for future endeavour.

Changes from v1 <https://lore.kernel.org/r/[e-mail]>:
- use pointers to values in dst_reg directly as the sum/diff pointer and
  remove the else branch (Jiri)
- change local variables to be dst_reg pointers instead of src_reg values
- include comparison of generated assembly before & after the change
  (Alexei)

1: https://github.com/kernel-patches/bpf/pull/7205/commits

Shung-Hsi Yu (2):
  bpf: use check_add_overflow() to check for addition overflows
  bpf: use check_sub_overflow() to check for subtraction overflows

 kernel/bpf/verifier.c | 151 ++++++++++++------------------------------
 1 file changed, 42 insertions(+), 109 deletions(-)

Comments

Jiri Olsa July 1, 2024, 9:18 a.m. UTC | #1

On Mon, Jul 01, 2024 at 01:59:03PM +0800, Shung-Hsi Yu wrote:
> This patch set refactors kernel/bpf/verifier.c to use type-agnostic,
> generic overflow-check helpers defined in include/linux/overflow.h to
> check for addition and subtraction overflow, and drop the
> signed_*_overflows() helpers we currently have in kernel/bpf/verifier.c.
> There should be no functional change in how the verifier works.
> 
> The main motivation is to make future refactoring[1] easier.
> 
> While check_mul_overflow() also exists and could potentially replace what
> we have in scalar*_min_max_mul(), it does not help with refactoring and
> would either change how the verifier works (e.g. lifting restriction on
> umax<=U32_MAX and u32_max<=U16_MAX) or make the code slightly harder to
> read, so it is left for future endeavour.
> 
> Changes from v1 <https://lore.kernel.org/r/[e-mail]>:
> - use pointers to values in dst_reg directly as the sum/diff pointer and
>   remove the else branch (Jiri)
> - change local variables to be dst_reg pointers instead of src_reg values
> - include comparison of generated assembly before & after the change
>   (Alexei)
> 
> 1: https://github.com/kernel-patches/bpf/pull/7205/commits

CI failed, but it looks like aws hiccup:
  https://github.com/kernel-patches/bpf/actions/runs/9739067425/job/26873810583

lgtm

Acked-by: Jiri Olsa <[e-mail]>

jirka

> 
> Shung-Hsi Yu (2):
>   bpf: use check_add_overflow() to check for addition overflows
>   bpf: use check_sub_overflow() to check for subtraction overflows
> 
>  kernel/bpf/verifier.c | 151 ++++++++++++------------------------------
>  1 file changed, 42 insertions(+), 109 deletions(-)
> 
> -- 
> 2.45.2
>

Shung-Hsi Yu July 1, 2024, 9:45 a.m. UTC | #2

On Mon, Jul 01, 2024 at 11:18:39AM GMT, Jiri Olsa wrote:
> On Mon, Jul 01, 2024 at 01:59:03PM +0800, Shung-Hsi Yu wrote:
> > This patch set refactors kernel/bpf/verifier.c to use type-agnostic,
> > generic overflow-check helpers defined in include/linux/overflow.h to
> > check for addition and subtraction overflow, and drop the
> > signed_*_overflows() helpers we currently have in kernel/bpf/verifier.c.
> > There should be no functional change in how the verifier works.
> > 
> > The main motivation is to make future refactoring[1] easier.
> > 
> > While check_mul_overflow() also exists and could potentially replace what
> > we have in scalar*_min_max_mul(), it does not help with refactoring and
> > would either change how the verifier works (e.g. lifting restriction on
> > umax<=U32_MAX and u32_max<=U16_MAX) or make the code slightly harder to
> > read, so it is left for future endeavour.
> > 
> > Changes from v1 <https://lore.kernel.org/r/[e-mail]>:
> > - use pointers to values in dst_reg directly as the sum/diff pointer and
> >   remove the else branch (Jiri)
> > - change local variables to be dst_reg pointers instead of src_reg values
> > - include comparison of generated assembly before & after the change
> >   (Alexei)
> > 
> > 1: https://github.com/kernel-patches/bpf/pull/7205/commits
> 
> CI failed, but it looks like aws hiccup:
>   https://github.com/kernel-patches/bpf/actions/runs/9739067425/job/26873810583

Should be, before submitting I've checked that it passed in the BPF
CI[1] through manually opened PR#7280.

> lgtm
> 
> Acked-by: Jiri Olsa <[e-mail]>

Thanks!

1: https://github.com/kernel-patches/bpf/actions/runs/9738751746