| Message ID | b7fed809fc44d0b439adeb69d0fe98dd036b05e8.1541876179.git.mirq-linux@rere.qmqm.pl (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | Remove VLAN.CFI overload | expand |
On 11/10/2018 07:58 PM, Michał Mirosław wrote: > Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl> Why you have empty commit messages for non-trivial changes like this in 4 out of 6 of your patches ... How was it tested on the JITs you were changing? Did you test on both, big and little endian machines? > --- > net/core/filter.c | 40 +++++++++++++++++++++------------------- > 1 file changed, 21 insertions(+), 19 deletions(-) > > diff --git a/net/core/filter.c b/net/core/filter.c > index e521c5ebc7d1..c151b906df53 100644 > --- a/net/core/filter.c > +++ b/net/core/filter.c > @@ -296,22 +296,21 @@ static u32 convert_skb_access(int skb_field, int dst_reg, int src_reg, > break; > > case SKF_AD_VLAN_TAG: > - case SKF_AD_VLAN_TAG_PRESENT: > BUILD_BUG_ON(FIELD_SIZEOF(struct sk_buff, vlan_tci) != 2); > - BUILD_BUG_ON(VLAN_TAG_PRESENT != 0x1000); > > /* dst_reg = *(u16 *) (src_reg + offsetof(vlan_tci)) */ > *insn++ = BPF_LDX_MEM(BPF_H, dst_reg, src_reg, > offsetof(struct sk_buff, vlan_tci)); > - if (skb_field == SKF_AD_VLAN_TAG) { > - *insn++ = BPF_ALU32_IMM(BPF_AND, dst_reg, > - ~VLAN_TAG_PRESENT); > - } else { > - /* dst_reg >>= 12 */ > - *insn++ = BPF_ALU32_IMM(BPF_RSH, dst_reg, 12); > - /* dst_reg &= 1 */ > +#ifdef VLAN_TAG_PRESENT > + *insn++ = BPF_ALU32_IMM(BPF_AND, dst_reg, ~VLAN_TAG_PRESENT); > +#endif > + break; > + case SKF_AD_VLAN_TAG_PRESENT: > + *insn++ = BPF_LDX_MEM(BPF_B, dst_reg, src_reg, PKT_VLAN_PRESENT_OFFSET()); > + if (PKT_VLAN_PRESENT_BIT) > + *insn++ = BPF_ALU32_IMM(BPF_RSH, dst_reg, PKT_VLAN_PRESENT_BIT); > + if (PKT_VLAN_PRESENT_BIT < 7) > *insn++ = BPF_ALU32_IMM(BPF_AND, dst_reg, 1); > - } > break; > } > > @@ -6140,19 +6139,22 @@ static u32 bpf_convert_ctx_access(enum bpf_access_type type, > break; > > case offsetof(struct __sk_buff, vlan_present): > + *target_size = 1; > + *insn++ = BPF_LDX_MEM(BPF_B, si->dst_reg, si->src_reg, > + PKT_VLAN_PRESENT_OFFSET()); > + if (PKT_VLAN_PRESENT_BIT) > + *insn++ = BPF_ALU32_IMM(BPF_RSH, si->dst_reg, PKT_VLAN_PRESENT_BIT); > + if (PKT_VLAN_PRESENT_BIT < 7) > + *insn++ = BPF_ALU32_IMM(BPF_AND, si->dst_reg, 1); > + break; > + > case offsetof(struct __sk_buff, vlan_tci): > - BUILD_BUG_ON(VLAN_TAG_PRESENT != 0x1000); > - > *insn++ = BPF_LDX_MEM(BPF_H, si->dst_reg, si->src_reg, > bpf_target_off(struct sk_buff, vlan_tci, 2, > target_size)); > - if (si->off == offsetof(struct __sk_buff, vlan_tci)) { > - *insn++ = BPF_ALU32_IMM(BPF_AND, si->dst_reg, > - ~VLAN_TAG_PRESENT); > - } else { > - *insn++ = BPF_ALU32_IMM(BPF_RSH, si->dst_reg, 12); > - *insn++ = BPF_ALU32_IMM(BPF_AND, si->dst_reg, 1); > - } > +#ifdef VLAN_TAG_PRESENT > + *insn++ = BPF_ALU32_IMM(BPF_AND, si->dst_reg, ~VLAN_TAG_PRESENT); > +#endif > break; > > case offsetof(struct __sk_buff, cb[0]) ... >
On Mon, Nov 19, 2018 at 12:26:46PM +0100, Daniel Borkmann wrote: > On 11/10/2018 07:58 PM, Michał Mirosław wrote: > > Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl> > > Why you have empty commit messages for non-trivial changes like this in > 4 out of 6 of your patches ... > > How was it tested on the JITs you were changing? Did you test on both, > big and little endian machines? I have only x86 boxes currently so didn't try to test others. I hope upstreaming the series through net-next will allow us to find any fallouts, if any. The changes are very simple, though: they move code around (the "splitting" part) and eventually change a vlan_present flag's position in a skbuff structure. Dependency on CPU endianness is removed by using byte loads for the flag. Best Regards, Michał Mirosław
diff --git a/net/core/filter.c b/net/core/filter.c index e521c5ebc7d1..c151b906df53 100644 --- a/net/core/filter.c +++ b/net/core/filter.c @@ -296,22 +296,21 @@ static u32 convert_skb_access(int skb_field, int dst_reg, int src_reg, break; case SKF_AD_VLAN_TAG: - case SKF_AD_VLAN_TAG_PRESENT: BUILD_BUG_ON(FIELD_SIZEOF(struct sk_buff, vlan_tci) != 2); - BUILD_BUG_ON(VLAN_TAG_PRESENT != 0x1000); /* dst_reg = *(u16 *) (src_reg + offsetof(vlan_tci)) */ *insn++ = BPF_LDX_MEM(BPF_H, dst_reg, src_reg, offsetof(struct sk_buff, vlan_tci)); - if (skb_field == SKF_AD_VLAN_TAG) { - *insn++ = BPF_ALU32_IMM(BPF_AND, dst_reg, - ~VLAN_TAG_PRESENT); - } else { - /* dst_reg >>= 12 */ - *insn++ = BPF_ALU32_IMM(BPF_RSH, dst_reg, 12); - /* dst_reg &= 1 */ +#ifdef VLAN_TAG_PRESENT + *insn++ = BPF_ALU32_IMM(BPF_AND, dst_reg, ~VLAN_TAG_PRESENT); +#endif + break; + case SKF_AD_VLAN_TAG_PRESENT: + *insn++ = BPF_LDX_MEM(BPF_B, dst_reg, src_reg, PKT_VLAN_PRESENT_OFFSET()); + if (PKT_VLAN_PRESENT_BIT) + *insn++ = BPF_ALU32_IMM(BPF_RSH, dst_reg, PKT_VLAN_PRESENT_BIT); + if (PKT_VLAN_PRESENT_BIT < 7) *insn++ = BPF_ALU32_IMM(BPF_AND, dst_reg, 1); - } break; } @@ -6140,19 +6139,22 @@ static u32 bpf_convert_ctx_access(enum bpf_access_type type, break; case offsetof(struct __sk_buff, vlan_present): + *target_size = 1; + *insn++ = BPF_LDX_MEM(BPF_B, si->dst_reg, si->src_reg, + PKT_VLAN_PRESENT_OFFSET()); + if (PKT_VLAN_PRESENT_BIT) + *insn++ = BPF_ALU32_IMM(BPF_RSH, si->dst_reg, PKT_VLAN_PRESENT_BIT); + if (PKT_VLAN_PRESENT_BIT < 7) + *insn++ = BPF_ALU32_IMM(BPF_AND, si->dst_reg, 1); + break; + case offsetof(struct __sk_buff, vlan_tci): - BUILD_BUG_ON(VLAN_TAG_PRESENT != 0x1000); - *insn++ = BPF_LDX_MEM(BPF_H, si->dst_reg, si->src_reg, bpf_target_off(struct sk_buff, vlan_tci, 2, target_size)); - if (si->off == offsetof(struct __sk_buff, vlan_tci)) { - *insn++ = BPF_ALU32_IMM(BPF_AND, si->dst_reg, - ~VLAN_TAG_PRESENT); - } else { - *insn++ = BPF_ALU32_IMM(BPF_RSH, si->dst_reg, 12); - *insn++ = BPF_ALU32_IMM(BPF_AND, si->dst_reg, 1); - } +#ifdef VLAN_TAG_PRESENT + *insn++ = BPF_ALU32_IMM(BPF_AND, si->dst_reg, ~VLAN_TAG_PRESENT); +#endif break; case offsetof(struct __sk_buff, cb[0]) ...
Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl> --- net/core/filter.c | 40 +++++++++++++++++++++------------------- 1 file changed, 21 insertions(+), 19 deletions(-)