| Message ID | 20181230162743.20842-1-phil@nwl.cc |
|---|---|
| State | Accepted |
| Delegated to: | Pablo Neira |
| Headers | show |
| Series | net: nf_tables: Fix for endless loop when dumping ruleset | expand |
Phil Sutter <phil@nwl.cc> wrote: > __nf_tables_dump_rules() stores the current idx value into cb->args[0] > before returning to caller. With multiple chains present, cb->args[0] is > therefore updated after each chain's rules have been traversed. This > though causes the final nf_tables_dump_rules() run (which should return > an skb->len of zero since no rules are left to dump) to continue dumping > rules for each but the first chain. Fix this by moving the cb->args[0] > update to nf_tables_dump_rules(). > > With no final action to be performed anymore in > __nf_tables_dump_rules(), drop 'out_unfinished' jump label and 'rc' > variable - instead return the appropriate value directly. Looks good, but I think this is a bug too: list = rhltable_lookup(&table->chains_ht, ctx->chain, nft_chain_ht_params); if (!list) goto done; I think this should move to next table instead. (Its not related to the bug at hand though).
Hi Florian, On Sun, Dec 30, 2018 at 08:10:28PM +0100, Florian Westphal wrote: > Phil Sutter <phil@nwl.cc> wrote: > > __nf_tables_dump_rules() stores the current idx value into cb->args[0] > > before returning to caller. With multiple chains present, cb->args[0] is > > therefore updated after each chain's rules have been traversed. This > > though causes the final nf_tables_dump_rules() run (which should return > > an skb->len of zero since no rules are left to dump) to continue dumping > > rules for each but the first chain. Fix this by moving the cb->args[0] > > update to nf_tables_dump_rules(). > > > > With no final action to be performed anymore in > > __nf_tables_dump_rules(), drop 'out_unfinished' jump label and 'rc' > > variable - instead return the appropriate value directly. > > Looks good, but I think this is a bug too: > > list = rhltable_lookup(&table->chains_ht, ctx->chain, > nft_chain_ht_params); > if (!list) > goto done; > > I think this should move to next table instead. Hmm. Yes, assuming that specifying no table but only chain is a valid use-case, this should indeed continue with the next table. I'll send a v2 which includes that fix as well. > (Its not related to the bug at hand though). And not easy to trigger since all known users pass either both table and chain or none of them. :) Thanks, Phil
Hi! On Mon, Dec 31, 2018 at 12:28:52AM +0100, Phil Sutter wrote: > Hi Florian, > > On Sun, Dec 30, 2018 at 08:10:28PM +0100, Florian Westphal wrote: > > Phil Sutter <phil@nwl.cc> wrote: > > > __nf_tables_dump_rules() stores the current idx value into cb->args[0] > > > before returning to caller. With multiple chains present, cb->args[0] is > > > therefore updated after each chain's rules have been traversed. This > > > though causes the final nf_tables_dump_rules() run (which should return > > > an skb->len of zero since no rules are left to dump) to continue dumping > > > rules for each but the first chain. Fix this by moving the cb->args[0] > > > update to nf_tables_dump_rules(). > > > > > > With no final action to be performed anymore in > > > __nf_tables_dump_rules(), drop 'out_unfinished' jump label and 'rc' > > > variable - instead return the appropriate value directly. > > > > Looks good, but I think this is a bug too: > > > > list = rhltable_lookup(&table->chains_ht, ctx->chain, > > nft_chain_ht_params); > > if (!list) > > goto done; > > > > I think this should move to next table instead. > > Hmm. Yes, assuming that specifying no table but only chain is a valid > use-case, this should indeed continue with the next table. I'll send a > v2 which includes that fix as well. > > > (Its not related to the bug at hand though). > > And not easy to trigger since all known users pass either both table and > chain or none of them. :) We can probably fix the problem that Florian reports via something like the patch that I'm attaching? It is restricting the selecting chain dump to work with ctx->table. If fine with that, I would prefer to take your patch v1 to fix the endless loop problem (ie. this patch). Thanks! diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c index e3ddd8e95e58..04d504a31e60 100644 --- a/net/netfilter/nf_tables_api.c +++ b/net/netfilter/nf_tables_api.c @@ -2350,7 +2350,7 @@ static int nf_tables_dump_rules(struct sk_buff *skb, if (ctx && ctx->table && strcmp(ctx->table, table->name) != 0) continue; - if (ctx && ctx->chain) { + if (ctx && ctx->table && ctx->chain) { struct rhlist_head *list, *tmp; list = rhltable_lookup(&table->chains_ht, ctx->chain,
Pablo Neira Ayuso <pablo@netfilter.org> wrote: > If fine with that, I would prefer to take your patch v1 to fix the > endless loop problem (ie. this patch). > diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c > index e3ddd8e95e58..04d504a31e60 100644 > --- a/net/netfilter/nf_tables_api.c > +++ b/net/netfilter/nf_tables_api.c > @@ -2350,7 +2350,7 @@ static int nf_tables_dump_rules(struct sk_buff *skb, > if (ctx && ctx->table && strcmp(ctx->table, table->name) != 0) > continue; > > - if (ctx && ctx->chain) { > + if (ctx && ctx->table && ctx->chain) { LGTM.
On Sun, Dec 30, 2018 at 05:27:43PM +0100, Phil Sutter wrote: > __nf_tables_dump_rules() stores the current idx value into cb->args[0] > before returning to caller. With multiple chains present, cb->args[0] is > therefore updated after each chain's rules have been traversed. This > though causes the final nf_tables_dump_rules() run (which should return > an skb->len of zero since no rules are left to dump) to continue dumping > rules for each but the first chain. Fix this by moving the cb->args[0] > update to nf_tables_dump_rules(). > > With no final action to be performed anymore in > __nf_tables_dump_rules(), drop 'out_unfinished' jump label and 'rc' > variable - instead return the appropriate value directly. Applied patch v1, thanks Phil.
On Tue, Jan 08, 2019 at 11:15:41PM +0100, Florian Westphal wrote: > Pablo Neira Ayuso <pablo@netfilter.org> wrote: > > If fine with that, I would prefer to take your patch v1 to fix the > > endless loop problem (ie. this patch). > > > diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c > > index e3ddd8e95e58..04d504a31e60 100644 > > --- a/net/netfilter/nf_tables_api.c > > +++ b/net/netfilter/nf_tables_api.c > > @@ -2350,7 +2350,7 @@ static int nf_tables_dump_rules(struct sk_buff *skb, > > if (ctx && ctx->table && strcmp(ctx->table, table->name) != 0) > > continue; > > > > - if (ctx && ctx->chain) { > > + if (ctx && ctx->table && ctx->chain) { > > LGTM. Thanks Florian, sending a follow up separated patch for this.
diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c index 09ad796d9904e..68d55f122259e 100644 --- a/net/netfilter/nf_tables_api.c +++ b/net/netfilter/nf_tables_api.c @@ -2304,7 +2304,6 @@ static int __nf_tables_dump_rules(struct sk_buff *skb, struct net *net = sock_net(skb->sk); unsigned int s_idx = cb->args[0]; const struct nft_rule *rule; - int rc = 1; list_for_each_entry_rcu(rule, &chain->rules, list) { if (!nft_is_active(net, rule)) @@ -2321,16 +2320,13 @@ static int __nf_tables_dump_rules(struct sk_buff *skb, NLM_F_MULTI | NLM_F_APPEND, table->family, table, chain, rule) < 0) - goto out_unfinished; + return 1; nl_dump_check_consistent(cb, nlmsg_hdr(skb)); cont: (*idx)++; } - rc = 0; -out_unfinished: - cb->args[0] = *idx; - return rc; + return 0; } static int nf_tables_dump_rules(struct sk_buff *skb, @@ -2382,6 +2378,8 @@ static int nf_tables_dump_rules(struct sk_buff *skb, } done: rcu_read_unlock(); + + cb->args[0] = idx; return skb->len; }
__nf_tables_dump_rules() stores the current idx value into cb->args[0] before returning to caller. With multiple chains present, cb->args[0] is therefore updated after each chain's rules have been traversed. This though causes the final nf_tables_dump_rules() run (which should return an skb->len of zero since no rules are left to dump) to continue dumping rules for each but the first chain. Fix this by moving the cb->args[0] update to nf_tables_dump_rules(). With no final action to be performed anymore in __nf_tables_dump_rules(), drop 'out_unfinished' jump label and 'rc' variable - instead return the appropriate value directly. Fixes: 241faeceb849c ("netfilter: nf_tables: Speed up selective rule dumps") Signed-off-by: Phil Sutter <phil@nwl.cc> --- net/netfilter/nf_tables_api.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-)