RE: [PATCH bpf-next] docs/bpf: Document some special sdiv/smod operations

[Dave Thaler <dthaler1968@googlemail.com>]

Yonghong Song <yonghong.song@linux.dev> wrote: 
> On 9/30/24 6:50 PM, Alexei Starovoitov wrote:
> > On Thu, Sep 26, 2024 at 8:39 PM Yonghong Song <yonghong.song@linux.dev>
> wrote:
> >> Patch [1] fixed possible kernel crash due to specific sdiv/smod
> >> operations in bpf program. The following are related operations and
> >> the expected results of those operations:
> >>    - LLONG_MIN/-1 = LLONG_MIN
> >>    - INT_MIN/-1 = INT_MIN
> >>    - LLONG_MIN%-1 = 0
> >>    - INT_MIN%-1 = 0
> >>
> >> Those operations are replaced with codes which won't cause kernel
> >> crash. This patch documents what operations may cause exception and
> >> what replacement operations are.
> >>
> >>    [1]
> >> https://lore.kernel.org/all/20240913150326.1187788-1-yonghong.song@li
> >> nux.dev/
> >>
> >> Signed-off-by: Yonghong Song <yonghong.song@linux.dev>
> >> ---
> >>   .../bpf/standardization/instruction-set.rst   | 25 +++++++++++++++----
> >>   1 file changed, 20 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/Documentation/bpf/standardization/instruction-set.rst
> >> b/Documentation/bpf/standardization/instruction-set.rst
> >> index ab820d565052..d150c1d7ad3b 100644
> >> --- a/Documentation/bpf/standardization/instruction-set.rst
> >> +++ b/Documentation/bpf/standardization/instruction-set.rst
> >> @@ -347,11 +347,26 @@ register.
> >>     =====  =====  =======
> >> ==========================================================
> >>
> >>   Underflow and overflow are allowed during arithmetic operations,
> >> meaning -the 64-bit or 32-bit value will wrap. If BPF program
> >> execution would -result in division by zero, the destination register is instead set
> to zero.
> >> -If execution would result in modulo by zero, for ``ALU64`` the value of
> >> -the destination register is unchanged whereas for ``ALU`` the upper
> >> -32 bits of the destination register are zeroed.
> >> +the 64-bit or 32-bit value will wrap. There are also a few
> >> +arithmetic operations which may cause exception for certain
> >> +architectures. Since crashing the kernel is not an option, those operations are
> replaced with alternative operations.
> >> +
> >> +.. table:: Arithmetic operations with possible exceptions
> >> +
> >> +  =====  ==========  =============================
> ==========================
> >> +  name   class       original                       replacement
> >> +  =====  ==========  =============================
> ==========================
> >> +  DIV    ALU64/ALU   dst /= 0                       dst = 0
> >> +  SDIV   ALU64/ALU   dst s/= 0                      dst = 0
> >> +  MOD    ALU64       dst %= 0                       dst = dst (no replacement)
> >> +  MOD    ALU         dst %= 0                       dst = (u32)dst
> >> +  SMOD   ALU64       dst s%= 0                      dst = dst (no replacement)
> >> +  SMOD   ALU         dst s%= 0                      dst = (u32)dst

All of the above are already covered in existing Table 5 and in my opinion
don't need to be repeated.

That is, the "original" is not what Table 5 has, so just introduces confusion
in the document in my opinion.

> >> +  SDIV   ALU64       dst s/= -1 (dst = LLONG_MIN)   dst = LLONG_MIN
> >> +  SDIV   ALU         dst s/= -1 (dst = INT_MIN)     dst = (u32)INT_MIN
> >> +  SMOD   ALU64       dst s%= -1 (dst = LLONG_MIN)   dst = 0
> >> +  SMOD   ALU         dst s%= -1 (dst = INT_MIN)     dst = 0

The above four are the new ones and I'd prefer a solution that modifies
existing table 5.  E.g. table 5 has now for SMOD:

dst = (src != 0) ? (dst s% src) : dst

and could have something like this:

dst = (src == 0) ? dst : ((src == -1 && dst == INT_MIN) ? 0 : (dst s% src))

> > This is a great addition to the doc, but this file is currently being
> > used as a base for IETF standard which is in its final "edit" stage
> > which may require few patches, so we cannot land any changes to
> > instruction-set.rst not related to standardization until RFC number is
> > issued and it becomes immutable. After that the same
> > instruction-set.rst file can be reused for future revisions on the
> > standard.
> > Hopefully the draft will clear the final hurdle in a couple weeks.
> > Until then:
> > pw-bot: cr
> 
> Sure. No problem. Will resubmit once the RFC number is issued.

I'm adding bpf@ietf.org to the To line since all changes in the standardization
directory should include that mailing list.

The WG should discuss whether any changes should be done via a new RFC
that obsoletes the first one, or as RFCs that Update and just describe deltas
(additions, etc.).

There are precedents both ways and I don't have a strong preference, but I
have a weak preference for delta-based ones since they're shorter and are
less likely to re-open discussion on previously resolved issues, thus often
saving the WG time.

Also FYI to Linux kernel folks:
With WG and AD approval, it's also possible (but not ideal) to take changes
at AUTH48.  That'd be up to the chairs and AD to decide though, and normally
that's just for purely editorial clarifications, e.g., to confusion called out by the
RFC editor pass.

Dave