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