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

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

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@linux.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
>> +  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
> 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.