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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, Oct 1, 2024 at 12:54 PM Dave Thaler <dthaler1968@xxxxxxxxxxxxxx> wrote:
>
> Yonghong Song <yonghong.song@xxxxxxxxx> wrote:
> > On 9/30/24 6:50 PM, Alexei Starovoitov wrote:
> > > On Thu, Sep 26, 2024 at 8:39 PM Yonghong Song <yonghong.song@xxxxxxxxx>
> > 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@xxxxxxxxx>
> > >> ---
> > >>   .../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@xxxxxxxx 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.

Delta-based additions make sense to me.

>
> 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.

Also agree. We should keep AUTH going its course as-is.
All ISA additions can be in the future delta RFC.

As far as file logistics... my preference is to keep
Documentation/bpf/standardization/instruction-set.rst
up to date.
Right now it's effectively frozen while awaiting changes (if any)
necessary for AUTH. After official RFC is issued
we can start landing patches into instruction-set.rst and
git diff 04efaebd72d1..whatever_future_sha instruction-set.rst
will automatically generate the future delta RFC.
Once RFC number is issued we can add a git tag for the particular
sha that was the base for RFC as a documentation step and to
simplify future 'git diff'.





[Index of Archives]     [Linux Samsung SoC]     [Linux Rockchip SoC]     [Linux Actions SoC]     [Linux for Synopsys ARC Processors]     [Linux NFS]     [Linux NILFS]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]


  Powered by Linux