| Message ID | 20230913135137.15154-2-phil@nwl.cc (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | nf_tables: follow-up on audit fix, add selftest | expand |
Phil Sutter <phil@nwl.cc> wrote: > 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. Introduce a dedicated counter to record the number > of rules reset in this call in a less confusing way. > > While being at it, prevent the direct return upon buffer exhaustion: Any > rules previously dumped into that skb would evade audit logging > otherwise. Reviewed-by: Florian Westphal <fw@strlen.de> We can investigate ways to compress/coalesce (read: make this more complicated) in case somebody complains about too many audit messages. But I would not go ahead and keep it simple for now.
On Wed, Sep 13, 2023 at 09:31:46PM +0200, Florian Westphal wrote: > Phil Sutter <phil@nwl.cc> wrote: > > 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. Introduce a dedicated counter to record the number > > of rules reset in this call in a less confusing way. > > > > While being at it, prevent the direct return upon buffer exhaustion: Any > > rules previously dumped into that skb would evade audit logging > > otherwise. > > Reviewed-by: Florian Westphal <fw@strlen.de> > > We can investigate ways to compress/coalesce (read: make this more > complicated) in case somebody complains about too many audit messages. It is only about reset command. Anything following the transaction path is coalesced already (on a per-table basis, so there's more work needed if consistent per-chain logging is desired). > But I would not go ahead and keep it simple for now. I just want to avoid a second rhbz#2001815[1]. As we both know, OpenShift likes to have both excessively big chains and excessively many small ones. %) Cheers, Phil [1] https://bugzilla.redhat.com/show_bug.cgi?id=2001815
diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c index e429ebba74b3d..446e1882428e6 100644 --- a/net/netfilter/nf_tables_api.c +++ b/net/netfilter/nf_tables_api.c @@ -3449,6 +3449,8 @@ static int __nf_tables_dump_rules(struct sk_buff *skb, struct net *net = sock_net(skb->sk); const struct nft_rule *rule, *prule; unsigned int s_idx = cb->args[0]; + unsigned int entries = 0; + int ret = 0; u64 handle; prule = NULL; @@ -3471,9 +3473,11 @@ static int __nf_tables_dump_rules(struct sk_buff *skb, NFT_MSG_NEWRULE, NLM_F_MULTI | NLM_F_APPEND, table->family, - table, chain, rule, handle, reset) < 0) - return 1; - + table, chain, rule, handle, reset) < 0) { + ret = 1; + break; + } + entries++; nl_dump_check_consistent(cb, nlmsg_hdr(skb)); cont: prule = rule; @@ -3481,10 +3485,10 @@ static int __nf_tables_dump_rules(struct sk_buff *skb, (*idx)++; } - if (reset && *idx) - audit_log_rule_reset(table, cb->seq, *idx); + if (reset && entries) + audit_log_rule_reset(table, cb->seq, entries); - return 0; + return ret; } static int nf_tables_dump_rules(struct sk_buff *skb,
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. Introduce a dedicated counter to record the number of rules reset in this call in a less confusing way. While being at it, prevent the direct return upon buffer exhaustion: Any rules previously dumped into that skb would evade audit logging otherwise. Fixes: 9b5ba5c9c5109 ("netfilter: nf_tables: Unbreak audit log reset") Signed-off-by: Phil Sutter <phil@nwl.cc> --- Changes since v2: - Restore per-chain logging as requested. Changes since v1: - Use max_t() to eliminate the kernel warning --- net/netfilter/nf_tables_api.c | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-)