| Message ID | 20220928162300.1055-1-phil@nwl.cc |
|---|---|
| State | Accepted |
| Delegated to: | Florian Westphal |
| Headers | show |
| Series | [iptables] nft: Fix meta statement parsing | expand |
Phil Sutter <phil@nwl.cc> wrote: > The function nft_meta_set_to_target() would always bail since nothing > sets 'sreg->meta_sreg.set' to true. This is obvious, as the immediate > expression "filling" the source register does not indicate its purpose. Hmm, is there a missing test case? I did not see any failures. > The whole source register purpose storing in meta_sreg seems to be > pointless, so drop it altogether. Yes; from iptables perspective a 'meta set' operation has to be mapped to a target, so there is no need to store this for subsequent consumption.
On Wed, Sep 28, 2022 at 07:57:23PM +0200, Florian Westphal wrote: > Phil Sutter <phil@nwl.cc> wrote: > > The function nft_meta_set_to_target() would always bail since nothing > > sets 'sreg->meta_sreg.set' to true. This is obvious, as the immediate > > expression "filling" the source register does not indicate its purpose. > > Hmm, is there a missing test case? I did not see any failures. extensions/libxt_TRACE.t was failing if I called iptables-test.py with '-n' option. > > The whole source register purpose storing in meta_sreg seems to be > > pointless, so drop it altogether. > > Yes; from iptables perspective a 'meta set' operation has to be mapped > to a target, so there is no need to store this for subsequent > consumption. OK, cool. I'll push this all upstream now. :) Thanks, Phil
Phil Sutter <phil@nwl.cc> wrote: > On Wed, Sep 28, 2022 at 07:57:23PM +0200, Florian Westphal wrote: > > Phil Sutter <phil@nwl.cc> wrote: > > > The function nft_meta_set_to_target() would always bail since nothing > > > sets 'sreg->meta_sreg.set' to true. This is obvious, as the immediate > > > expression "filling" the source register does not indicate its purpose. > > > > Hmm, is there a missing test case? I did not see any failures. > > extensions/libxt_TRACE.t was failing if I called iptables-test.py with > '-n' option. Argh, I only tested classic :-( > > Yes; from iptables perspective a 'meta set' operation has to be mapped > > to a target, so there is no need to store this for subsequent > > consumption. > > OK, cool. I'll push this all upstream now. :) Thanks!
On Wed, Sep 28, 2022 at 11:27:09PM +0200, Florian Westphal wrote: > Phil Sutter <phil@nwl.cc> wrote: > > On Wed, Sep 28, 2022 at 07:57:23PM +0200, Florian Westphal wrote: > > > Phil Sutter <phil@nwl.cc> wrote: > > > > The function nft_meta_set_to_target() would always bail since nothing > > > > sets 'sreg->meta_sreg.set' to true. This is obvious, as the immediate > > > > expression "filling" the source register does not indicate its purpose. > > > > > > Hmm, is there a missing test case? I did not see any failures. > > > > extensions/libxt_TRACE.t was failing if I called iptables-test.py with > > '-n' option. > > Argh, I only tested classic :-( Same here! Making nft changes and forgetting to run the testsuite in nft mode is a classic. :) I guess we need a button to run them all at once. Cheers, Phil
diff --git a/iptables/nft-shared.c b/iptables/nft-shared.c index 909fe6483205c..996cff996c151 100644 --- a/iptables/nft-shared.c +++ b/iptables/nft-shared.c @@ -503,10 +503,7 @@ static void nft_meta_set_to_target(struct nft_xt_ctx *ctx, if (!sreg) return; - if (sreg->meta_sreg.set == 0) - return; - - switch (sreg->meta_sreg.key) { + switch (nftnl_expr_get_u32(e, NFTNL_EXPR_META_KEY)) { case NFT_META_NFTRACE: if ((sreg->type != NFT_XT_REG_IMMEDIATE)) { ctx->errmsg = "meta nftrace but reg not immediate"; @@ -526,8 +523,10 @@ static void nft_meta_set_to_target(struct nft_xt_ctx *ctx, } target = xtables_find_target(targname, XTF_TRY_LOAD); - if (target == NULL) + if (target == NULL) { + ctx->errmsg = "target TRACE not found"; return; + } size = XT_ALIGN(sizeof(struct xt_entry_target)) + target->size; @@ -1303,6 +1302,11 @@ void nft_rule_to_iptables_command_state(struct nft_handle *h, else if (strcmp(name, "range") == 0) nft_parse_range(&ctx, expr); + if (ctx.errmsg) { + fprintf(stderr, "%s", ctx.errmsg); + ctx.errmsg = NULL; + } + expr = nftnl_expr_iter_next(iter); } diff --git a/iptables/nft-shared.h b/iptables/nft-shared.h index c07d3270a407e..3d935d5324b01 100644 --- a/iptables/nft-shared.h +++ b/iptables/nft-shared.h @@ -68,11 +68,6 @@ struct nft_xt_ctx_reg { uint32_t xor[4]; bool set; } bitwise; - - struct { - uint32_t key; - bool set; - } meta_sreg; }; struct nft_xt_ctx { @@ -118,7 +113,6 @@ static inline void nft_xt_reg_clear(struct nft_xt_ctx_reg *r) { r->type = 0; r->bitwise.set = false; - r->meta_sreg.set = false; } static inline struct nft_xt_ctx_reg *nft_xt_ctx_get_dreg(struct nft_xt_ctx *ctx, enum nft_registers reg)
The function nft_meta_set_to_target() would always bail since nothing sets 'sreg->meta_sreg.set' to true. This is obvious, as the immediate expression "filling" the source register does not indicate its purpose. The whole source register purpose storing in meta_sreg seems to be pointless, so drop it altogether. Fixes: f315af1cf8871 ("nft: track each register individually") Signed-off-by: Phil Sutter <phil@nwl.cc> --- iptables/nft-shared.c | 14 +++++++++----- iptables/nft-shared.h | 6 ------ 2 files changed, 9 insertions(+), 11 deletions(-)