On Sun, Nov 05, 2023 at 06:41:39PM +0100, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@xxxxxxxxxxxxx> wrote:
> > > > if (shift) {
> > > > - if (ctx->stmt_len > 0 && div_round_up(masklen, BITS_PER_BYTE) > 1) {
> > > > + if (masklen > BITS_PER_BYTE) {
> > >
> > > I think this is right but binop xfer
> > > won't remove the inserted byteorder conversion
> > > in case the shift is to be removed by adjusting
> > > a constant right hand side value.
> >
> > ctx->stmt_len > 0 is also set from stmt_evaluate_{ct,meta}() so this
> > transformation is restricted to statements.
> >
> > I was conservative edecd58755a8 with ("evaluate: support shifts larger than
> > the width of the left operand") to restrict this transformation to
> > statements only.
> >
> > Do you think think this applies to other cases too?
> >
> > BTW, whatever the final fix is, Fixes: tag for this should be:
> >
> > Fixes: 668c18f67203 ("evaluate: place byteorder conversion before rshift in payload statement")
>
> I don't think this is a regression, I do not think it has worked before.
>
> table ip6 t {
> map mapv6 {
> typeof ip6 dscp : meta mark
> elements = { cs1 : 0x0000002a }
> }
>
> chain output {
> type filter hook output priority filter; policy accept;
> meta mark set ip6 dscp map @mapv6
> meta mark 0x0000002a counter
> ip6 dscp @mapv6 counter
> ip6 dscp cs1 counter
> }
> }
>
> A "ping6 -q -Q 0x20 ::1 -c1" should cause all counters
> in above ruleset to increment, but that is not the case.
Thanks for your simple test, it is passing with these two patches:
https://patchwork.ozlabs.org/project/netfilter-devel/patch/20231105174148.160390-1-pablo@xxxxxxxxxxxxx/
https://patchwork.ozlabs.org/project/netfilter-devel/patch/20231105212846.311755-1-pablo@xxxxxxxxxxxxx/
I have extended it to also cover for dynamic sets:
table ip6 t {
map mapv6 {
typeof ip6 dscp : meta mark
elements = { cs1 : 0x0000002a }
}
map mapv62 {
typeof ip6 dscp : meta mark
size 65535
flags dynamic
}
chain output {
type filter hook output priority filter; policy accept;
meta mark set ip6 dscp map @mapv6
meta mark 0x0000002a counter
ip6 dscp @mapv6 counter
ip6 dscp cs1 counter
update @mapv62 { ip6 dscp : meta mark }
}
}
I just quickly checked for concatenations with ip6 dscp, it seems the
byteorder conversion is also missing there, so v2 will follow up.
