| Message ID | 20230908002229.1409-2-phil@nwl.cc (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Paul Moore |
| Headers | show |
| Series | nf_tables: follow-up on audit fix, propose kselftest | expand |
Hi Phil, kernel test robot noticed the following build warnings: [auto build test WARNING on netfilter-nf/main] url: https://github.com/intel-lab-lkp/linux/commits/Phil-Sutter/netfilter-nf_tables-Fix-entries-val-in-rule-reset-audit-log/20230908-082530 base: git://git.kernel.org/pub/scm/linux/kernel/git/netfilter/nf.git main patch link: https://lore.kernel.org/r/20230908002229.1409-2-phil%40nwl.cc patch subject: [nf PATCH 1/2] netfilter: nf_tables: Fix entries val in rule reset audit log config: mips-randconfig-r002-20230908 (https://download.01.org/0day-ci/archive/20230908/202309081138.IpMoJwFy-lkp@intel.com/config) compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project.git 4a5ac14ee968ff0ad5d2cc1ffa0299048db4c88a) reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20230908/202309081138.IpMoJwFy-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202309081138.IpMoJwFy-lkp@intel.com/ All warnings (new ones prefixed by >>): >> net/netfilter/nf_tables_api.c:3536:11: warning: comparison of distinct pointer types ('typeof (idx) *' (aka 'unsigned int *') and 'typeof (cb->args[0]) *' (aka 'long *')) [-Wcompare-distinct-pointer-types] 3536 | s_idx = max(idx, cb->args[0]); | ^~~~~~~~~~~~~~~~~~~~~ include/linux/minmax.h:74:19: note: expanded from macro 'max' 74 | #define max(x, y) __careful_cmp(x, y, >) | ^~~~~~~~~~~~~~~~~~~~~~ include/linux/minmax.h:36:24: note: expanded from macro '__careful_cmp' 36 | __builtin_choose_expr(__safe_cmp(x, y), \ | ^~~~~~~~~~~~~~~~ include/linux/minmax.h:26:4: note: expanded from macro '__safe_cmp' 26 | (__typecheck(x, y) && __no_side_effects(x, y)) | ^~~~~~~~~~~~~~~~~ include/linux/minmax.h:20:28: note: expanded from macro '__typecheck' 20 | (!!(sizeof((typeof(x) *)1 == (typeof(y) *)1))) | ~~~~~~~~~~~~~~ ^ ~~~~~~~~~~~~~~ 1 warning generated. vim +3536 net/netfilter/nf_tables_api.c 3486 3487 static int nf_tables_dump_rules(struct sk_buff *skb, 3488 struct netlink_callback *cb) 3489 { 3490 const struct nfgenmsg *nfmsg = nlmsg_data(cb->nlh); 3491 const struct nft_rule_dump_ctx *ctx = cb->data; 3492 struct nft_table *table; 3493 const struct nft_chain *chain; 3494 unsigned int idx = 0, s_idx; 3495 struct net *net = sock_net(skb->sk); 3496 int family = nfmsg->nfgen_family; 3497 struct nftables_pernet *nft_net; 3498 bool reset = false; 3499 int ret; 3500 3501 if (NFNL_MSG_TYPE(cb->nlh->nlmsg_type) == NFT_MSG_GETRULE_RESET) 3502 reset = true; 3503 3504 rcu_read_lock(); 3505 nft_net = nft_pernet(net); 3506 cb->seq = READ_ONCE(nft_net->base_seq); 3507 3508 list_for_each_entry_rcu(table, &nft_net->tables, list) { 3509 if (family != NFPROTO_UNSPEC && family != table->family) 3510 continue; 3511 3512 if (ctx && ctx->table && strcmp(ctx->table, table->name) != 0) 3513 continue; 3514 3515 if (ctx && ctx->table && ctx->chain) { 3516 struct rhlist_head *list, *tmp; 3517 3518 list = rhltable_lookup(&table->chains_ht, ctx->chain, 3519 nft_chain_ht_params); 3520 if (!list) 3521 goto done; 3522 3523 rhl_for_each_entry_rcu(chain, tmp, list, rhlhead) { 3524 if (!nft_is_active(net, chain)) 3525 continue; 3526 __nf_tables_dump_rules(skb, &idx, 3527 cb, table, chain, reset); 3528 break; 3529 } 3530 if (reset && idx > cb->args[0]) 3531 audit_log_rule_reset(table, cb->seq, 3532 idx - cb->args[0]); 3533 goto done; 3534 } 3535 > 3536 s_idx = max(idx, cb->args[0]); 3537 list_for_each_entry_rcu(chain, &table->chains, list) { 3538 ret = __nf_tables_dump_rules(skb, &idx, 3539 cb, table, chain, reset); 3540 if (ret) 3541 break; 3542 } 3543 if (reset && idx > s_idx) 3544 audit_log_rule_reset(table, cb->seq, idx - s_idx); 3545 3546 if ((ctx && ctx->table) || ret) 3547 break; 3548 } 3549 done: 3550 rcu_read_unlock(); 3551 3552 cb->args[0] = idx; 3553 return skb->len; 3554 } 3555
diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c index e429ebba74b3d..d429270676131 100644 --- a/net/netfilter/nf_tables_api.c +++ b/net/netfilter/nf_tables_api.c @@ -3481,9 +3481,6 @@ static int __nf_tables_dump_rules(struct sk_buff *skb, (*idx)++; } - if (reset && *idx) - audit_log_rule_reset(table, cb->seq, *idx); - return 0; } @@ -3494,11 +3491,12 @@ static int nf_tables_dump_rules(struct sk_buff *skb, const struct nft_rule_dump_ctx *ctx = cb->data; struct nft_table *table; const struct nft_chain *chain; - unsigned int idx = 0; + unsigned int idx = 0, s_idx; struct net *net = sock_net(skb->sk); int family = nfmsg->nfgen_family; struct nftables_pernet *nft_net; bool reset = false; + int ret; if (NFNL_MSG_TYPE(cb->nlh->nlmsg_type) == NFT_MSG_GETRULE_RESET) reset = true; @@ -3529,16 +3527,23 @@ static int nf_tables_dump_rules(struct sk_buff *skb, cb, table, chain, reset); break; } + if (reset && idx > cb->args[0]) + audit_log_rule_reset(table, cb->seq, + idx - cb->args[0]); goto done; } + s_idx = max(idx, cb->args[0]); list_for_each_entry_rcu(chain, &table->chains, list) { - if (__nf_tables_dump_rules(skb, &idx, - cb, table, chain, reset)) - goto done; + ret = __nf_tables_dump_rules(skb, &idx, + cb, table, chain, reset); + if (ret) + break; } + if (reset && idx > s_idx) + audit_log_rule_reset(table, cb->seq, idx - s_idx); - if (ctx && ctx->table) + if ((ctx && ctx->table) || ret) break; } done:
The value in idx and the number of rules handled in that particular __nf_tables_dump_rules() call is not identical. The former is a cursor to pick up from if multiple netlink messages are needed, so its value is ever increasing. Fixing this is not just a matter of subtracting s_idx from it, though: When resetting rules in multiple chains, __nf_tables_dump_rules() is called for each and cb->args[0] is not adjusted in between. The audit notification in __nf_tables_dump_rules() had another problem: If nf_tables_fill_rule_info() failed (e.g. due to buffer exhaustion), no notification was sent - despite the rules having been reset already. To catch all the above and return to a single (if possible) notification per table again, move audit logging back into the caller but into the table loop instead of past it to avoid the potential null-pointer deref. This requires to trigger the notification in two spots. Care has to be taken in the second case as cb->args[0] is also not updated in between tables. This requires a helper variable as either it is the first table (with potential non-zero cb->args[0] cursor) or a consecutive one (with idx holding the current cursor already). Fixes: 9b5ba5c9c5109 ("netfilter: nf_tables: Unbreak audit log reset") Signed-off-by: Phil Sutter <phil@nwl.cc> --- net/netfilter/nf_tables_api.c | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-)