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

[Yonghong Song <yonghong.song@linux.dev>]

On 10/1/24 12:54 PM, Dave Thaler wrote:
> 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.

This tries to separate cases between ALU and ALU64. But I agree that the table
5 should be good enough.

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

Thanks. This indeed simpler. I can do this.