diff mbox

[RFC,nft,userspace] nft: connlabel matching support

Message ID 20140216172339.GA18357@macbook.localnet
State Not Applicable
Headers show

Commit Message

Patrick McHardy Feb. 16, 2014, 5:23 p.m. UTC 
On Sun, Feb 16, 2014 at 05:59:57PM +0100, Florian Westphal wrote:
> Patrick McHardy <kaber@trash.net> wrote:
> > Ok I misunderstood your initial problem statement. So basically what it
> > should currently do:
> > 
> > ct labels foo => test whether that bit is set
> > ct labels == foo => test whether foo and only foo is set
> > 
> > Ok I can see the problem :)
> > 
> > The implicit op only selects FLAGCMP for EXPR_LIST (see
> > expr_evaluate_relational()). That should probably be changed to take the
> > base type into account. This also seems wrong for the ct state expression,
> > we currently use equality if only one state is specified but use a flag
> > comparison if multiple flags are specified.
> 
> I hacked something up to also select FLAGCMP for bitmask type.
> 
> $ nft --debug=netlink add rule filter output ct labels foo
> ip filter output 0 0
>  [ ct load labels => reg 1 ]
>  [ bitwise reg 1 = (reg=1 & 0x00000001 0x00000000 0x00000000 0x00000000 ) ^ 0x00000000 0x00000000 0x00000000 0x00000000 ]
>  [ cmp neq reg 1 0x00000001 0x00000000 0x00000000 0x00000000 ]
> 
> looks better.  Still not exactly the same though.
> The cmp neq will cause it to match when the label is not set.
> 
> I then tried again with vanilla master branch:
> tcp flags syn counter packets 0 bytes 0
> tcp flags fin,syn counter packets 184 bytes 24880
> 
> So, same problem there: EXPR_LIST == cmp neq.  Is that intentional?
> It seems wrong to me, e.g.  "tcp flags fin,syn" will match virtually all
> tcp packets.
> 
> Maybe netlink_gen_flagcmp() should generate NFT_CMP_GT i.e.:
>  [ bitwise reg 1 = (reg=1 & 0x00000012 ) ^ 0x00000000 ]
>  [ cmp gt reg 1 0x00000000 ]
> 
> At least that would be what I would have expected :-}
> 
> Am I wrong?

It should actually generate a NEQ 0. Seems this was broken in commit
aae836a7 (src: use libnftables).

Try the attached patch please.

> As a side note, experimenting a bit with tcp flags:
> 
> add rule filter output tcp flags & (syn|ack) == (syn|ack)
> 
> works fine with current master branch.  But list shows
> 
> "tcp flags & 18 == 18", i.e. no symbol translation.
> 
> Shouldn't it restore the symbolic names?
> I think this is the very same problem that I had with my connlabel
> dabbling, so it would be nice if it could be solved in generic way.

Most likely. Please see if the attached patch solves this.

--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Florian Westphal Feb. 16, 2014, 5:51 p.m. UTC | #1 
Patrick McHardy <kaber@trash.net> wrote:
> > So, same problem there: EXPR_LIST == cmp neq.  Is that intentional?
> > It seems wrong to me, e.g.  "tcp flags fin,syn" will match virtually all
> > tcp packets.
> > 
> > Maybe netlink_gen_flagcmp() should generate NFT_CMP_GT i.e.:
> >  [ bitwise reg 1 = (reg=1 & 0x00000012 ) ^ 0x00000000 ]
> >  [ cmp gt reg 1 0x00000000 ]
> > 
> > At least that would be what I would have expected :-}
> > 
> > Am I wrong?
> 
> It should actually generate a NEQ 0. Seems this was broken in commit
> aae836a7 (src: use libnftables).
> 
> Try the attached patch please.

Thanks, 'tcp flags syn,fin' now behaves as expected.

> > As a side note, experimenting a bit with tcp flags:
> > 
> > add rule filter output tcp flags & (syn|ack) == (syn|ack)
> > 
> > works fine with current master branch.  But list shows
> > 
> > "tcp flags & 18 == 18", i.e. no symbol translation.
> > 
> > Shouldn't it restore the symbolic names?
> > I think this is the very same problem that I had with my connlabel
> > dabbling, so it would be nice if it could be solved in generic way.
> 
> Most likely. Please see if the attached patch solves this.

No:
add rule filter output tcp flags & (syn|ack) == (syn|ack)
list table filter
tcp flags & 18 == 18

The expression works.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Patrick McHardy Feb. 16, 2014, 5:53 p.m. UTC | #2 
On 16. Februar 2014 17:51:32 GMT+00:00, Florian Westphal <fw@strlen.de> wrote:
>Patrick McHardy <kaber@trash.net> wrote:
>> > So, same problem there: EXPR_LIST == cmp neq.  Is that intentional?
>> > It seems wrong to me, e.g.  "tcp flags fin,syn" will match
>virtually all
>> > tcp packets.
>> > 
>> > Maybe netlink_gen_flagcmp() should generate NFT_CMP_GT i.e.:
>> >  [ bitwise reg 1 = (reg=1 & 0x00000012 ) ^ 0x00000000 ]
>> >  [ cmp gt reg 1 0x00000000 ]
>> > 
>> > At least that would be what I would have expected :-}
>> > 
>> > Am I wrong?
>> 
>> It should actually generate a NEQ 0. Seems this was broken in commit
>> aae836a7 (src: use libnftables).
>> 
>> Try the attached patch please.
>
>Thanks, 'tcp flags syn,fin' now behaves as expected.

I'll push the patch, thanks.

>> > As a side note, experimenting a bit with tcp flags:
>> > 
>> > add rule filter output tcp flags & (syn|ack) == (syn|ack)
>> > 
>> > works fine with current master branch.  But list shows
>> > 
>> > "tcp flags & 18 == 18", i.e. no symbol translation.
>> > 
>> > Shouldn't it restore the symbolic names?
>> > I think this is the very same problem that I had with my connlabel
>> > dabbling, so it would be nice if it could be solved in generic way.
>> 
>> Most likely. Please see if the attached patch solves this.
>
>No:
>add rule filter output tcp flags & (syn|ack) == (syn|ack)
>list table filter
>tcp flags & 18 == 18
>
>The expression works.

I'll look into it in about an hour.

--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Patrick McHardy Feb. 16, 2014, 8:41 p.m. UTC | #3 
On Sun, Feb 16, 2014 at 05:53:33PM +0000, Patrick McHardy wrote:
> On 16. Februar 2014 17:51:32 GMT+00:00, Florian Westphal <fw@strlen.de> wrote:
> >> > As a side note, experimenting a bit with tcp flags:
> >> > 
> >> > add rule filter output tcp flags & (syn|ack) == (syn|ack)
> >> > 
> >> > works fine with current master branch.  But list shows
> >> > 
> >> > "tcp flags & 18 == 18", i.e. no symbol translation.
> >> > 
> >> > Shouldn't it restore the symbolic names?
> >> > I think this is the very same problem that I had with my connlabel
> >> > dabbling, so it would be nice if it could be solved in generic way.
> >> 
> >> Most likely. Please see if the attached patch solves this.
> >
> >No:
> >add rule filter output tcp flags & (syn|ack) == (syn|ack)
> >list table filter
> >tcp flags & 18 == 18
> >
> >The expression works.
> 
> I'll look into it in about an hour.

It requires a bit more work. I'm half way through, but if this is holding
you up, I'd suggest to just proceed as if it were working already.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/src/netlink_linearize.c b/src/netlink_linearize.c
index 332383a..3e9a078 100644
--- a/src/netlink_linearize.c
+++ b/src/netlink_linearize.c
@@ -285,7 +285,6 @@  static void netlink_gen_flagcmp(struct netlink_linearize_ctx *ctx,
 	netlink_gen_raw_data(zero, expr->right->byteorder, len, &nld);
 	nft_rule_expr_set_u32(nle, NFT_EXPR_CMP_SREG, sreg);
 	nft_rule_expr_set_u32(nle, NFT_EXPR_CMP_OP, NFT_CMP_NEQ);
-	netlink_gen_data(expr->right, &nld);
 	nft_rule_expr_set(nle, NFT_EXPR_CMP_DATA, nld.value, nld.len);
 	nft_rule_add_expr(ctx->nlr, nle);