| Message ID | 20231102135258.17214-1-phil@nwl.cc |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [nft] tproxy: Drop artificial port printing restriction | expand |
On Thu, Nov 02, 2023 at 02:52:58PM +0100, Phil Sutter wrote: > It does not make much sense to omit printing the port expression if it's > not a value expression: On one hand, input allows for more advanced > uses. On the other, if it is in-kernel, best nft can do is to try and > print it no matter what. Just ignoring ruleset elements can't be > correct. > > Fixes: 2be1d52644cf7 ("src: Add tproxy support") > Closes: https://bugzilla.netfilter.org/show_bug.cgi?id=1721 > Signed-off-by: Phil Sutter <phil@nwl.cc> Great work Phil. Reviewed-by: Pablo Neira Ayuso <pablo@netfilter.org> > --- > src/statement.c | 2 +- > tests/py/inet/tproxy.t | 2 ++ > tests/py/inet/tproxy.t.json | 35 ++++++++++++++++++++++++++++++++++ > tests/py/inet/tproxy.t.payload | 12 ++++++++++++ > 4 files changed, 50 insertions(+), 1 deletion(-) > > diff --git a/src/statement.c b/src/statement.c > index 475611664946a..f5176e6d87f95 100644 > --- a/src/statement.c > +++ b/src/statement.c > @@ -989,7 +989,7 @@ static void tproxy_stmt_print(const struct stmt *stmt, struct output_ctx *octx) > expr_print(stmt->tproxy.addr, octx); > } > } > - if (stmt->tproxy.port && stmt->tproxy.port->etype == EXPR_VALUE) { > + if (stmt->tproxy.port) { > if (!stmt->tproxy.addr) > nft_print(octx, " "); > nft_print(octx, ":"); > diff --git a/tests/py/inet/tproxy.t b/tests/py/inet/tproxy.t > index d23bbcb56cdcd..9901df75a91a8 100644 > --- a/tests/py/inet/tproxy.t > +++ b/tests/py/inet/tproxy.t > @@ -19,3 +19,5 @@ meta l4proto 17 tproxy ip to :50080;ok > meta l4proto 17 tproxy ip6 to :50080;ok > meta l4proto 17 tproxy to :50080;ok > ip daddr 0.0.0.0/0 meta l4proto 6 tproxy ip to :2000;ok > + > +meta l4proto 6 tproxy ip to 127.0.0.1:symhash mod 2 map { 0 : 23, 1 : 42 };ok > diff --git a/tests/py/inet/tproxy.t.json b/tests/py/inet/tproxy.t.json > index 7b3b11c49205a..71b6fd2f678dd 100644 > --- a/tests/py/inet/tproxy.t.json > +++ b/tests/py/inet/tproxy.t.json > @@ -183,3 +183,38 @@ > } > } > ] > + > +# meta l4proto 6 tproxy ip to 127.0.0.1:symhash mod 2 map { 0 : 23, 1 : 42 } > +[ > + { > + "match": { > + "left": { > + "meta": { > + "key": "l4proto" > + } > + }, > + "op": "==", > + "right": 6 > + } > + }, > + { > + "tproxy": { > + "addr": "127.0.0.1", > + "family": "ip", > + "port": { > + "map": { > + "data": { > + "set": [ > + [ 0, 23 ], > + [ 1, 42 ] > + ] > + }, > + "key": { > + "symhash": { "mod": 2 } > + } > + } > + } > + } > + } > +] > + > diff --git a/tests/py/inet/tproxy.t.payload b/tests/py/inet/tproxy.t.payload > index 24bf8f6002f8f..2f41904261144 100644 > --- a/tests/py/inet/tproxy.t.payload > +++ b/tests/py/inet/tproxy.t.payload > @@ -61,3 +61,15 @@ inet x y > [ immediate reg 1 0x0000d007 ] > [ tproxy ip port reg 1 ] > > +# meta l4proto 6 tproxy ip to 127.0.0.1:symhash mod 2 map { 0 : 23, 1 : 42 } > +__map%d x b size 2 > +__map%d x 0 > + element 00000000 : 00001700 0 [end] element 00000001 : 00002a00 0 [end] > +inet x y > + [ meta load l4proto => reg 1 ] > + [ cmp eq reg 1 0x00000006 ] > + [ immediate reg 1 0x0100007f ] > + [ hash reg 2 = symhash() % mod 2 ] > + [ lookup reg 2 set __map%d dreg 2 ] > + [ tproxy ip addr reg 1 port reg 2 ] > + > -- > 2.41.0 >
On Thu, Nov 02, 2023 at 04:56:37PM +0100, Pablo Neira Ayuso wrote: > On Thu, Nov 02, 2023 at 02:52:58PM +0100, Phil Sutter wrote: [...] > > diff --git a/src/statement.c b/src/statement.c > > index 475611664946a..f5176e6d87f95 100644 > > --- a/src/statement.c > > +++ b/src/statement.c > > @@ -989,7 +989,7 @@ static void tproxy_stmt_print(const struct stmt *stmt, struct output_ctx *octx) > > expr_print(stmt->tproxy.addr, octx); > > } > > } > > - if (stmt->tproxy.port && stmt->tproxy.port->etype == EXPR_VALUE) { > > + if (stmt->tproxy.port) { Question: is this pattern used elsewhere? The original author of this might have taken (copied) this code from an existing statement?
On Thu, Nov 02, 2023 at 04:58:43PM +0100, Pablo Neira Ayuso wrote: > On Thu, Nov 02, 2023 at 04:56:37PM +0100, Pablo Neira Ayuso wrote: > > On Thu, Nov 02, 2023 at 02:52:58PM +0100, Phil Sutter wrote: > [...] > > > diff --git a/src/statement.c b/src/statement.c > > > index 475611664946a..f5176e6d87f95 100644 > > > --- a/src/statement.c > > > +++ b/src/statement.c > > > @@ -989,7 +989,7 @@ static void tproxy_stmt_print(const struct stmt *stmt, struct output_ctx *octx) > > > expr_print(stmt->tproxy.addr, octx); > > > } > > > } > > > - if (stmt->tproxy.port && stmt->tproxy.port->etype == EXPR_VALUE) { > > > + if (stmt->tproxy.port) { > > Question: is this pattern used elsewhere? > > The original author of this might have taken (copied) this code from > an existing statement? A quick grep for EXPR_VALUE didn't turn up anything suspicious. Maybe Máté recalls why he did things this way back in 2018. FWIW, the EXPR_VALUE check was already there in v1 of his patch. Cheers, Phil
On Thu, Nov 02, 2023 at 04:56:32PM +0100, Pablo Neira Ayuso wrote: > On Thu, Nov 02, 2023 at 02:52:58PM +0100, Phil Sutter wrote: > > It does not make much sense to omit printing the port expression if it's > > not a value expression: On one hand, input allows for more advanced > > uses. On the other, if it is in-kernel, best nft can do is to try and > > print it no matter what. Just ignoring ruleset elements can't be > > correct. > > > > Fixes: 2be1d52644cf7 ("src: Add tproxy support") > > Closes: https://bugzilla.netfilter.org/show_bug.cgi?id=1721 > > Signed-off-by: Phil Sutter <phil@nwl.cc> > > Great work Phil. Hey, I'm just deleting code which gets in the way. ;) > Reviewed-by: Pablo Neira Ayuso <pablo@netfilter.org> Patch applied, thanks for your review.
diff --git a/src/statement.c b/src/statement.c index 475611664946a..f5176e6d87f95 100644 --- a/src/statement.c +++ b/src/statement.c @@ -989,7 +989,7 @@ static void tproxy_stmt_print(const struct stmt *stmt, struct output_ctx *octx) expr_print(stmt->tproxy.addr, octx); } } - if (stmt->tproxy.port && stmt->tproxy.port->etype == EXPR_VALUE) { + if (stmt->tproxy.port) { if (!stmt->tproxy.addr) nft_print(octx, " "); nft_print(octx, ":"); diff --git a/tests/py/inet/tproxy.t b/tests/py/inet/tproxy.t index d23bbcb56cdcd..9901df75a91a8 100644 --- a/tests/py/inet/tproxy.t +++ b/tests/py/inet/tproxy.t @@ -19,3 +19,5 @@ meta l4proto 17 tproxy ip to :50080;ok meta l4proto 17 tproxy ip6 to :50080;ok meta l4proto 17 tproxy to :50080;ok ip daddr 0.0.0.0/0 meta l4proto 6 tproxy ip to :2000;ok + +meta l4proto 6 tproxy ip to 127.0.0.1:symhash mod 2 map { 0 : 23, 1 : 42 };ok diff --git a/tests/py/inet/tproxy.t.json b/tests/py/inet/tproxy.t.json index 7b3b11c49205a..71b6fd2f678dd 100644 --- a/tests/py/inet/tproxy.t.json +++ b/tests/py/inet/tproxy.t.json @@ -183,3 +183,38 @@ } } ] + +# meta l4proto 6 tproxy ip to 127.0.0.1:symhash mod 2 map { 0 : 23, 1 : 42 } +[ + { + "match": { + "left": { + "meta": { + "key": "l4proto" + } + }, + "op": "==", + "right": 6 + } + }, + { + "tproxy": { + "addr": "127.0.0.1", + "family": "ip", + "port": { + "map": { + "data": { + "set": [ + [ 0, 23 ], + [ 1, 42 ] + ] + }, + "key": { + "symhash": { "mod": 2 } + } + } + } + } + } +] + diff --git a/tests/py/inet/tproxy.t.payload b/tests/py/inet/tproxy.t.payload index 24bf8f6002f8f..2f41904261144 100644 --- a/tests/py/inet/tproxy.t.payload +++ b/tests/py/inet/tproxy.t.payload @@ -61,3 +61,15 @@ inet x y [ immediate reg 1 0x0000d007 ] [ tproxy ip port reg 1 ] +# meta l4proto 6 tproxy ip to 127.0.0.1:symhash mod 2 map { 0 : 23, 1 : 42 } +__map%d x b size 2 +__map%d x 0 + element 00000000 : 00001700 0 [end] element 00000001 : 00002a00 0 [end] +inet x y + [ meta load l4proto => reg 1 ] + [ cmp eq reg 1 0x00000006 ] + [ immediate reg 1 0x0100007f ] + [ hash reg 2 = symhash() % mod 2 ] + [ lookup reg 2 set __map%d dreg 2 ] + [ tproxy ip addr reg 1 port reg 2 ] +
It does not make much sense to omit printing the port expression if it's not a value expression: On one hand, input allows for more advanced uses. On the other, if it is in-kernel, best nft can do is to try and print it no matter what. Just ignoring ruleset elements can't be correct. Fixes: 2be1d52644cf7 ("src: Add tproxy support") Closes: https://bugzilla.netfilter.org/show_bug.cgi?id=1721 Signed-off-by: Phil Sutter <phil@nwl.cc> --- src/statement.c | 2 +- tests/py/inet/tproxy.t | 2 ++ tests/py/inet/tproxy.t.json | 35 ++++++++++++++++++++++++++++++++++ tests/py/inet/tproxy.t.payload | 12 ++++++++++++ 4 files changed, 50 insertions(+), 1 deletion(-)