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