| Message ID | 20230509180845.1236-1-dthaler1968@googlemail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 8819495a754e71d3c3fde991c26ad832af995136 |
| Delegated to: | BPF |
| Headers | show |
| Series | [bpf-next] Shift operations are defined to use a mask | expand |
On 5/9/23 11:08 AM, Dave Thaler wrote: > From: Dave Thaler <dthaler@microsoft.com> > > Update the documentation regarding shift operations to explain the > use of a mask, since otherwise shifting by a value out of range > (like negative) is undefined. > > Signed-off-by: Dave Thaler <dthaler@microsoft.com> LGTM with a few nit below. Acked-by: Yonghong Song <yhs@fb.com> > --- > Documentation/bpf/instruction-set.rst | 9 ++++++--- > 1 file changed, 6 insertions(+), 3 deletions(-) > > diff --git a/Documentation/bpf/instruction-set.rst b/Documentation/bpf/instruction-set.rst > index 492980ece1a..6644842cd3e 100644 > --- a/Documentation/bpf/instruction-set.rst > +++ b/Documentation/bpf/instruction-set.rst > @@ -163,13 +163,13 @@ BPF_MUL 0x20 dst \*= src > BPF_DIV 0x30 dst = (src != 0) ? (dst / src) : 0 > BPF_OR 0x40 dst \|= src > BPF_AND 0x50 dst &= src > -BPF_LSH 0x60 dst <<= src > -BPF_RSH 0x70 dst >>= src > +BPF_LSH 0x60 dst <<= (src & mask) > +BPF_RSH 0x70 dst >>= (src & mask) > BPF_NEG 0x80 dst = ~src > BPF_MOD 0x90 dst = (src != 0) ? (dst % src) : dst > BPF_XOR 0xa0 dst ^= src > BPF_MOV 0xb0 dst = src > -BPF_ARSH 0xc0 sign extending shift right > +BPF_ARSH 0xc0 sign extending dst >>= (src & mask) dst s>>= (src & mask) ? > BPF_END 0xd0 byte swap operations (see `Byte swap instructions`_ below) > ======== ===== ========================================================== > > @@ -204,6 +204,9 @@ for ``BPF_ALU64``, 'imm' is first sign extended to 64 bits and the result > interpreted as an unsigned 64-bit value. There are no instructions for > signed division or modulo. > > +Shift operations use a mask of 0x3F (63) for 64-bit operations and 0x1F (31) > +for 32-bit operations. > + > Byte swap instructions > ~~~~~~~~~~~~~~~~~~~~~~ >
Yonghong Song <yhs@meta.com> wrote: > On 5/9/23 11:08 AM, Dave Thaler wrote: > > From: Dave Thaler <dthaler@microsoft.com> > > > > Update the documentation regarding shift operations to explain the use > > of a mask, since otherwise shifting by a value out of range (like > > negative) is undefined. > > > > Signed-off-by: Dave Thaler <dthaler@microsoft.com> > > LGTM with a few nit below. > > Acked-by: Yonghong Song <yhs@fb.com> [...] > > -BPF_ARSH 0xc0 sign extending shift right > > +BPF_ARSH 0xc0 sign extending dst >>= (src & mask) > > dst s>>= (src & mask) > ? I had thought about that, but based on Jose's LSF/MM/BPF presentation yesterday there are multiple such syntaxes. ">>=" vs "s>>=" is only one of several. There's ">>" vs ">>>", there's assembly-like, etc. So I thought that it would take more text to define "s>>" as meaning signing extending right shift, than just saying sign extending ">>=" here. And I didn't want to just assume the reader knows what "s>>" means without defining it since neither the C standard nor gcc use "s>>". Thanks for the Ack. Dave
On 5/10/23 8:45 AM, Dave Thaler wrote: > Yonghong Song <yhs@meta.com> wrote: >> On 5/9/23 11:08 AM, Dave Thaler wrote: >>> From: Dave Thaler <dthaler@microsoft.com> >>> >>> Update the documentation regarding shift operations to explain the use >>> of a mask, since otherwise shifting by a value out of range (like >>> negative) is undefined. >>> >>> Signed-off-by: Dave Thaler <dthaler@microsoft.com> >> >> LGTM with a few nit below. >> >> Acked-by: Yonghong Song <yhs@fb.com> > [...] >>> -BPF_ARSH 0xc0 sign extending shift right >>> +BPF_ARSH 0xc0 sign extending dst >>= (src & mask) >> >> dst s>>= (src & mask) >> ? > > I had thought about that, but based on Jose's LSF/MM/BPF > presentation yesterday there are multiple such syntaxes. > > ">>=" vs "s>>=" is only one of several. There's ">>" vs ">>>", > there's assembly-like, etc. So I thought that it would take > more text to define "s>>" as meaning signing extending right > shift, than just saying sign extending ">>=" here. And I didn't > want to just assume the reader knows what "s>>" means > without defining it since neither the C standard nor gcc use > "s>>". gcc will implement clang asm syntax as well. So for the consistency of verifier log, bpftool xlated dump, and llvm-objdump result. I think using "s>>=" syntax is the best. The following table is the alu opcode in kernel/bpf/disasm.c (used by both kernel verifier and bpftool xlated dump): const char *const bpf_alu_string[16] = { [BPF_ADD >> 4] = "+=", [BPF_SUB >> 4] = "-=", [BPF_MUL >> 4] = "*=", [BPF_DIV >> 4] = "/=", [BPF_OR >> 4] = "|=", [BPF_AND >> 4] = "&=", [BPF_LSH >> 4] = "<<=", [BPF_RSH >> 4] = ">>=", [BPF_NEG >> 4] = "neg", [BPF_MOD >> 4] = "%=", [BPF_XOR >> 4] = "^=", [BPF_MOV >> 4] = "=", [BPF_ARSH >> 4] = "s>>=", [BPF_END >> 4] = "endian", }; Also, in Documentation/bpf/instruction-set.rst: ======== ===== ========================================================== code value description ======== ===== ========================================================== BPF_ADD 0x00 dst += src BPF_SUB 0x10 dst -= src BPF_MUL 0x20 dst \*= src BPF_DIV 0x30 dst = (src != 0) ? (dst / src) : 0 BPF_OR 0x40 dst \|= src BPF_AND 0x50 dst &= src BPF_LSH 0x60 dst <<= src BPF_RSH 0x70 dst >>= src BPF_NEG 0x80 dst = ~src BPF_MOD 0x90 dst = (src != 0) ? (dst % src) : dst BPF_XOR 0xa0 dst ^= src BPF_MOV 0xb0 dst = src BPF_ARSH 0xc0 sign extending shift right BPF_END 0xd0 byte swap operations (see `Byte swap instructions`_ below) ======== ===== ========================================================== In the above, the BPF_NEG is specified as 'dst = ~src`, which is not correct, it should be 'dst = -dst'. See kernel/bpf/core.c: ALU_NEG: DST = (u32) -DST; CONT; ALU64_NEG: DST = -DST; CONT; Could you help fix it? > > Thanks for the Ack. > > Dave >
Yonghong Song <yhs@meta.com> wrote: > On 5/10/23 8:45 AM, Dave Thaler wrote: > > Yonghong Song <yhs@meta.com> wrote: > >> On 5/9/23 11:08 AM, Dave Thaler wrote: > >>> From: Dave Thaler <dthaler@microsoft.com> > >>> > >>> Update the documentation regarding shift operations to explain the > >>> use of a mask, since otherwise shifting by a value out of range > >>> (like > >>> negative) is undefined. > >>> > >>> Signed-off-by: Dave Thaler <dthaler@microsoft.com> > >> > >> LGTM with a few nit below. > >> > >> Acked-by: Yonghong Song <yhs@fb.com> > > [...] > >>> -BPF_ARSH 0xc0 sign extending shift right > >>> +BPF_ARSH 0xc0 sign extending dst >>= (src & mask) > >> > >> dst s>>= (src & mask) > >> ? > > > > I had thought about that, but based on Jose's LSF/MM/BPF presentation > > yesterday there are multiple such syntaxes. > > > > ">>=" vs "s>>=" is only one of several. There's ">>" vs ">>>", > > there's assembly-like, etc. So I thought that it would take > > more text to define "s>>" as meaning signing extending right shift, > > than just saying sign extending ">>=" here. And I didn't want to just > > assume the reader knows what "s>>" means without defining it since > > neither the C standard nor gcc use "s>>". > > gcc will implement clang asm syntax as well. So for the consistency of verifier > log, bpftool xlated dump, and llvm-objdump result. > I think using "s>>=" syntax is the best. Just posting to the list what we discussed in person today. I will do this in a subsequent submission since that comment also affects the comparison operators, so treating it as separate from this patch. > The following table is the alu opcode in kernel/bpf/disasm.c (used by both > kernel verifier and bpftool xlated dump): > > const char *const bpf_alu_string[16] = { > [BPF_ADD >> 4] = "+=", > [BPF_SUB >> 4] = "-=", > [BPF_MUL >> 4] = "*=", > [BPF_DIV >> 4] = "/=", > [BPF_OR >> 4] = "|=", > [BPF_AND >> 4] = "&=", > [BPF_LSH >> 4] = "<<=", > [BPF_RSH >> 4] = ">>=", > [BPF_NEG >> 4] = "neg", > [BPF_MOD >> 4] = "%=", > [BPF_XOR >> 4] = "^=", > [BPF_MOV >> 4] = "=", > [BPF_ARSH >> 4] = "s>>=", > [BPF_END >> 4] = "endian", > }; > > Also, in Documentation/bpf/instruction-set.rst: > > ======== ===== > ========================================================== > code value description > ======== ===== > ========================================================== > BPF_ADD 0x00 dst += src > BPF_SUB 0x10 dst -= src > BPF_MUL 0x20 dst \*= src > BPF_DIV 0x30 dst = (src != 0) ? (dst / src) : 0 > BPF_OR 0x40 dst \|= src > BPF_AND 0x50 dst &= src > BPF_LSH 0x60 dst <<= src > BPF_RSH 0x70 dst >>= src > BPF_NEG 0x80 dst = ~src > BPF_MOD 0x90 dst = (src != 0) ? (dst % src) : dst > BPF_XOR 0xa0 dst ^= src > BPF_MOV 0xb0 dst = src > BPF_ARSH 0xc0 sign extending shift right > BPF_END 0xd0 byte swap operations (see `Byte swap instructions`_ below) > ======== ===== > ========================================================== > > In the above, the BPF_NEG is specified as 'dst = ~src`, which is not correct, it > should be 'dst = -dst'. > > See kernel/bpf/core.c: > ALU_NEG: > DST = (u32) -DST; > CONT; > ALU64_NEG: > DST = -DST; > CONT; > > Could you help fix it? Yes I can put it into the same patch as the other change discussed above. Would like to see the current one merged so I can base the next one on top of this one. Thanks, Dave
Hello: This patch was applied to bpf/bpf-next.git (master) by Daniel Borkmann <daniel@iogearbox.net>: On Tue, 9 May 2023 18:08:45 +0000 you wrote: > From: Dave Thaler <dthaler@microsoft.com> > > Update the documentation regarding shift operations to explain the > use of a mask, since otherwise shifting by a value out of range > (like negative) is undefined. > > Signed-off-by: Dave Thaler <dthaler@microsoft.com> > > [...] Here is the summary with links: - [bpf-next] Shift operations are defined to use a mask https://git.kernel.org/bpf/bpf-next/c/8819495a754e You are awesome, thank you!
diff --git a/Documentation/bpf/instruction-set.rst b/Documentation/bpf/instruction-set.rst index 492980ece1a..6644842cd3e 100644 --- a/Documentation/bpf/instruction-set.rst +++ b/Documentation/bpf/instruction-set.rst @@ -163,13 +163,13 @@ BPF_MUL 0x20 dst \*= src BPF_DIV 0x30 dst = (src != 0) ? (dst / src) : 0 BPF_OR 0x40 dst \|= src BPF_AND 0x50 dst &= src -BPF_LSH 0x60 dst <<= src -BPF_RSH 0x70 dst >>= src +BPF_LSH 0x60 dst <<= (src & mask) +BPF_RSH 0x70 dst >>= (src & mask) BPF_NEG 0x80 dst = ~src BPF_MOD 0x90 dst = (src != 0) ? (dst % src) : dst BPF_XOR 0xa0 dst ^= src BPF_MOV 0xb0 dst = src -BPF_ARSH 0xc0 sign extending shift right +BPF_ARSH 0xc0 sign extending dst >>= (src & mask) BPF_END 0xd0 byte swap operations (see `Byte swap instructions`_ below) ======== ===== ========================================================== @@ -204,6 +204,9 @@ for ``BPF_ALU64``, 'imm' is first sign extended to 64 bits and the result interpreted as an unsigned 64-bit value. There are no instructions for signed division or modulo. +Shift operations use a mask of 0x3F (63) for 64-bit operations and 0x1F (31) +for 32-bit operations. + Byte swap instructions ~~~~~~~~~~~~~~~~~~~~~~