| Message ID | 20240927033904.2702474-1-yonghong.song@linux.dev (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF |
| Headers | show |
| Series | [bpf-next] docs/bpf: Document some special sdiv/smod operations | expand |
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
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.
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. 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@ietf.org 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
On Tue, Oct 1, 2024 at 12:54 PM Dave Thaler <dthaler1968@googlemail.com> 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. > > 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@ietf.org 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'.
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.
Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: > On Tue, Oct 1, 2024 at 12:54 PM Dave Thaler <dthaler1968@googlemail.com> > wrote: [...] > > I'm adding bpf@ietf.org 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'. My concern is that index.rst says: > This directory contains documents that are being iterated on as part of the BPF > standardization effort with the IETF. See the `IETF BPF Working Group`_ page > for the working group charter, documents, and more. So having a document that is NOT part of the IETF BPF Working Group would seem out of place and, in my view, better located up a level (outside standardization). Here’s some examples of delta-based RFCs which explain the gap and provide the addition or clarification, and formally Update (not replace/obsolete) the original RFC: * https://www.rfc-editor.org/rfc/rfc6585.html: Additional HTTP Status Codes * https://www.rfc-editor.org/rfc/rfc6840.html: Clarifications and Implementation Notes for DNS Security (DNSSEC) * https://www.rfc-editor.org/rfc/rfc9295.html: Clarifications for Ed25519, Ed448, X25519, and X448 Algorithm Identifiers * https://www.rfc-editor.org/rfc/rfc5756.html: Updates for RSAES-OAEP and RSASSA-PSS Algorithm Parameters Having a full document too is valuable but unless the IETF BPF WG decides to take on a -bis document, I'd suggest keeping it out of the "standardization" (say up 1 level) to avoid confusion, and just have one or more delta-based rst files in the standardization directory. Dave
On Thu, Nov 7, 2024 at 6:30 PM Dave Thaler <dthaler1968@googlemail.com> wrote: > > > Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: > > On Tue, Oct 1, 2024 at 12:54 PM Dave Thaler <dthaler1968@googlemail.com> > > wrote: > [...] > > > I'm adding bpf@ietf.org 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'. > > My concern is that index.rst says: > > This directory contains documents that are being iterated on as part of the BPF > > standardization effort with the IETF. See the `IETF BPF Working Group`_ page > > for the working group charter, documents, and more. > > So having a document that is NOT part of the IETF BPF Working Group would seem > out of place and, in my view, better located up a level (outside standardization). It's a part of bpf wg. It's not a new document. > Here’s some examples of delta-based RFCs which explain the gap and provide > the addition or clarification, and formally Update (not replace/obsolete) the original > RFC: > * https://www.rfc-editor.org/rfc/rfc6585.html: Additional HTTP Status Codes > * https://www.rfc-editor.org/rfc/rfc6840.html: Clarifications and Implementation Notes > for DNS Security (DNSSEC) > * https://www.rfc-editor.org/rfc/rfc9295.html: Clarifications for Ed25519, Ed448, > X25519, and X448 Algorithm Identifiers > * https://www.rfc-editor.org/rfc/rfc5756.html: Updates for RSAES-OAEP and > RSASSA-PSS Algorithm Parameters > > Having a full document too is valuable but unless the IETF BPF WG > decides to take on a -bis document, I'd suggest keeping it out of the "standardization" > (say up 1 level) to avoid confusion, and just have one or more delta-based rst files > in the standardization directory. This patch is effectively a fix to the standard. It's a standard git development process when fixes are applied to the existing document. Forking the whole doc into a different file just to apply fixes makes no sense to me. The formal delta-s for IETF can be created out of git. We only need to tag the current version and then git diff rfc9669_tag..HEAD will give us that delta. That will satisfy IETF process and won't mess up normal git style kernel development. btw do we still need to do any minor edit/fixes to instruction-set.rst before tagging it as RFC9669 ?
> -----Original Message----- > From: Alexei Starovoitov <alexei.starovoitov@gmail.com> > Sent: Friday, November 8, 2024 10:38 AM > To: Dave Thaler <dthaler1968@googlemail.com> > Cc: Yonghong Song <yonghong.song@linux.dev>; bpf@ietf.org; bpf > <bpf@vger.kernel.org>; Alexei Starovoitov <ast@kernel.org>; Andrii Nakryiko > <andrii@kernel.org>; Daniel Borkmann <daniel@iogearbox.net>; Martin KaFai Lau > <martin.lau@kernel.org> > Subject: Re: [PATCH bpf-next] docs/bpf: Document some special sdiv/smod > operations > > On Thu, Nov 7, 2024 at 6:30 PM Dave Thaler <dthaler1968@googlemail.com> > wrote: > > > > > > Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: > > > On Tue, Oct 1, 2024 at 12:54 PM Dave Thaler > > > <dthaler1968@googlemail.com> > > > wrote: > > [...] > > > > I'm adding bpf@ietf.org 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'. > > > > My concern is that index.rst says: > > > This directory contains documents that are being iterated on as part > > > of the BPF standardization effort with the IETF. See the `IETF BPF > > > Working Group`_ page for the working group charter, documents, and more. > > > > So having a document that is NOT part of the IETF BPF Working Group > > would seem out of place and, in my view, better located up a level (outside > standardization). > > It's a part of bpf wg. It's not a new document. RFC 9669 is immutable. Any additions require a new document, in IETF terminology, since would result in a new RFC number. > > Here’s some examples of delta-based RFCs which explain the gap and > > provide the addition or clarification, and formally Update (not > > replace/obsolete) the original > > RFC: > > * https://www.rfc-editor.org/rfc/rfc6585.html: Additional HTTP Status > > Codes > > * https://www.rfc-editor.org/rfc/rfc6840.html: Clarifications and Implementation > Notes > > for DNS Security (DNSSEC) > > * https://www.rfc-editor.org/rfc/rfc9295.html: Clarifications for Ed25519, Ed448, > > X25519, and X448 Algorithm Identifiers > > * https://www.rfc-editor.org/rfc/rfc5756.html: Updates for RSAES-OAEP and > > RSASSA-PSS Algorithm Parameters > > > > Having a full document too is valuable but unless the IETF BPF WG > > decides to take on a -bis document, I'd suggest keeping it out of the > "standardization" > > (say up 1 level) to avoid confusion, and just have one or more > > delta-based rst files in the standardization directory. > > This patch is effectively a fix to the standard. Two of the examples I provided above fit into that category. Two are examples of adding new codepoints. > It's a standard git development process when fixes are applied to the existing > document. > Forking the whole doc into a different file just to apply fixes makes no sense to me. Welcome to the IETF and immutable RFCs
On 11/8/24 10:53 AM, Dave Thaler wrote: >> -----Original Message----- >> From: Alexei Starovoitov <alexei.starovoitov@gmail.com> >> Sent: Friday, November 8, 2024 10:38 AM >> To: Dave Thaler <dthaler1968@googlemail.com> >> Cc: Yonghong Song <yonghong.song@linux.dev>; bpf@ietf.org; bpf >> <bpf@vger.kernel.org>; Alexei Starovoitov <ast@kernel.org>; Andrii Nakryiko >> <andrii@kernel.org>; Daniel Borkmann <daniel@iogearbox.net>; Martin KaFai Lau >> <martin.lau@kernel.org> >> Subject: Re: [PATCH bpf-next] docs/bpf: Document some special sdiv/smod >> operations >> >> On Thu, Nov 7, 2024 at 6:30 PM Dave Thaler <dthaler1968@googlemail.com> >> wrote: >>> >>> Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: >>>> On Tue, Oct 1, 2024 at 12:54 PM Dave Thaler >>>> <dthaler1968@googlemail.com> >>>> wrote: >>> [...] >>>>> I'm adding bpf@ietf.org 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'. >>> My concern is that index.rst says: >>>> This directory contains documents that are being iterated on as part >>>> of the BPF standardization effort with the IETF. See the `IETF BPF >>>> Working Group`_ page for the working group charter, documents, and more. >>> So having a document that is NOT part of the IETF BPF Working Group >>> would seem out of place and, in my view, better located up a level (outside >> standardization). >> >> It's a part of bpf wg. It's not a new document. > RFC 9669 is immutable. Any additions require a new document, in > IETF terminology, since would result in a new RFC number. > >>> Here’s some examples of delta-based RFCs which explain the gap and >>> provide the addition or clarification, and formally Update (not >>> replace/obsolete) the original >>> RFC: >>> * https://www.rfc-editor.org/rfc/rfc6585.html: Additional HTTP Status >>> Codes >>> * https://www.rfc-editor.org/rfc/rfc6840.html: Clarifications and Implementation >> Notes >>> for DNS Security (DNSSEC) >>> * https://www.rfc-editor.org/rfc/rfc9295.html: Clarifications for Ed25519, Ed448, >>> X25519, and X448 Algorithm Identifiers >>> * https://www.rfc-editor.org/rfc/rfc5756.html: Updates for RSAES-OAEP and >>> RSASSA-PSS Algorithm Parameters >>> >>> Having a full document too is valuable but unless the IETF BPF WG >>> decides to take on a -bis document, I'd suggest keeping it out of the >> "standardization" >>> (say up 1 level) to avoid confusion, and just have one or more >>> delta-based rst files in the standardization directory. >> This patch is effectively a fix to the standard. > Two of the examples I provided above fit into that category. > Two are examples of adding new codepoints. > >> It's a standard git development process when fixes are applied to the existing >> document. >> Forking the whole doc into a different file just to apply fixes makes no sense to me. > Welcome to the IETF and immutable RFCs
On Fri, Nov 8, 2024 at 10:53 AM Dave Thaler <dthaler1968@googlemail.com> wrote: > > > > > > > My concern is that index.rst says: > > > > This directory contains documents that are being iterated on as part > > > > of the BPF standardization effort with the IETF. See the `IETF BPF > > > > Working Group`_ page for the working group charter, documents, and more. > > > > > > So having a document that is NOT part of the IETF BPF Working Group > > > would seem out of place and, in my view, better located up a level (outside > > standardization). > > > > It's a part of bpf wg. It's not a new document. > > RFC 9669 is immutable. Any additions require a new document, in > IETF terminology, since would result in a new RFC number. Sure. It's an IETF process. Not arguing about that. > > > Here’s some examples of delta-based RFCs which explain the gap and > > > provide the addition or clarification, and formally Update (not > > > replace/obsolete) the original > > > RFC: > > > * https://www.rfc-editor.org/rfc/rfc6585.html: Additional HTTP Status > > > Codes > > > * https://www.rfc-editor.org/rfc/rfc6840.html: Clarifications and Implementation > > Notes > > > for DNS Security (DNSSEC) > > > * https://www.rfc-editor.org/rfc/rfc9295.html: Clarifications for Ed25519, Ed448, > > > X25519, and X448 Algorithm Identifiers > > > * https://www.rfc-editor.org/rfc/rfc5756.html: Updates for RSAES-OAEP and > > > RSASSA-PSS Algorithm Parameters > > > > > > Having a full document too is valuable but unless the IETF BPF WG > > > decides to take on a -bis document, I'd suggest keeping it out of the > > "standardization" > > > (say up 1 level) to avoid confusion, and just have one or more > > > delta-based rst files in the standardization directory. > > > > This patch is effectively a fix to the standard. > > Two of the examples I provided above fit into that category. > Two are examples of adding new codepoints. > > > It's a standard git development process when fixes are applied to the existing > > document. > > Forking the whole doc into a different file just to apply fixes makes no sense to me. > > Welcome to the IETF and immutable RFCs
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 + ===== ========== ============================= =========================== ``{ADD, X, ALU}``, where 'code' = ``ADD``, 'source' = ``X``, and 'class' = ``ALU``, means::
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(-)