Hi Phil,
On Sat, Sep 23, 2023 at 03:53:50AM +0200, Phil Sutter wrote:
> When adding/updating an object, the transaction handler emits suitable
> audit log entries already, the one in nft_obj_notify() is redundant. To
> fix that (and retain the audit logging from objects' 'update' callback),
> Introduce an "audit log free" variant for internal use.
>
> Fixes: c520292f29b80 ("audit: log nftables configuration change events once
> per table")
> Signed-off-by: Phil Sutter <[email protected]>
> ---
> net/netfilter/nf_tables_api.c | 44 ++++++++++++-------
> .../testing/selftests/netfilter/nft_audit.sh | 20 +++++++++
> 2 files changed, 48 insertions(+), 16 deletions(-)
>
> diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
> index 0e5d9bdba82b8..48d50df950a18 100644
> --- a/net/netfilter/nf_tables_api.c
> +++ b/net/netfilter/nf_tables_api.c
> @@ -8046,24 +8046,14 @@ static int nf_tables_delobj(struct sk_buff *skb,
> const struct nfnl_info *info,
> return nft_delobj(&ctx, obj);
> }
>
> -void nft_obj_notify(struct net *net, const struct nft_table *table,
> - struct nft_object *obj, u32 portid, u32 seq, int event,
> - u16 flags, int family, int report, gfp_t gfp)
> +static void
> +__nft_obj_notify(struct net *net, const struct nft_table *table,
> + struct nft_object *obj, u32 portid, u32 seq, int event,
> + u16 flags, int family, int report, gfp_t gfp)
> {
> struct nftables_pernet *nft_net = nft_pernet(net);
> struct sk_buff *skb;
> int err;
> - char *buf = kasprintf(gfp, "%s:%u",
> - table->name, nft_net->base_seq);
> -
> - audit_log_nfcfg(buf,
> - family,
> - obj->handle,
> - event == NFT_MSG_NEWOBJ ?
> - AUDIT_NFT_OP_OBJ_REGISTER :
> - AUDIT_NFT_OP_OBJ_UNREGISTER,
> - gfp);
> - kfree(buf);
>
> if (!report &&
> !nfnetlink_has_listeners(net, NFNLGRP_NFTABLES))
> @@ -8086,13 +8076,35 @@ void nft_obj_notify(struct net *net, const struct
> nft_table *table,
> err:
> nfnetlink_set_err(net, portid, NFNLGRP_NFTABLES, -ENOBUFS);
> }
> +
> +void nft_obj_notify(struct net *net, const struct nft_table *table,
> + struct nft_object *obj, u32 portid, u32 seq, int event,
> + u16 flags, int family, int report, gfp_t gfp)
> +{
> + struct nftables_pernet *nft_net = nft_pernet(net);
> + char *buf = kasprintf(gfp, "%s:%u",
> + table->name, nft_net->base_seq);
> +
> + audit_log_nfcfg(buf,
> + family,
> + obj->handle,
> + event == NFT_MSG_NEWOBJ ?
> + AUDIT_NFT_OP_OBJ_REGISTER :
> + AUDIT_NFT_OP_OBJ_UNREGISTER,
> + gfp);
> + kfree(buf);
> +
> + __nft_obj_notify(net, table, obj, portid, seq, event,
> + flags, family, report, gfp);
> +}
> EXPORT_SYMBOL_GPL(nft_obj_notify);
OK, so nft_obj_notify() is called from nft_quota to notify that the
quota is depleted and the audit log is still there in this case.
> static void nf_tables_obj_notify(const struct nft_ctx *ctx,
> struct nft_object *obj, int event)
> {
> - nft_obj_notify(ctx->net, ctx->table, obj, ctx->portid, ctx->seq, event,
> - ctx->flags, ctx->family, ctx->report, GFP_KERNEL);
> + __nft_obj_notify(ctx->net, ctx->table, obj, ctx->portid,
> + ctx->seq, event, ctx->flags, ctx->family,
> + ctx->report, GFP_KERNEL);
> }
This function is called from the commit path to send the event
notification, and it should send the audit log?
Is this nf_tables_commit_audit_log() that provides the redundant log,
right?
> /*
> diff --git a/tools/testing/selftests/netfilter/nft_audit.sh
> b/tools/testing/selftests/netfilter/nft_audit.sh
> index 0b3255e7b3538..bb34329e02a7f 100755
> --- a/tools/testing/selftests/netfilter/nft_audit.sh
> +++ b/tools/testing/selftests/netfilter/nft_audit.sh
> @@ -85,6 +85,26 @@ do_test "nft add set t1 s2 $setblock; add set t1 s3 {
> $settype; }" \
> do_test "nft add element t1 s3 $setelem" \
> "table=t1 family=2 entries=3 op=nft_register_setelem"
>
> +# adding counters
> +
> +do_test 'nft add counter t1 c1' \
> +'table=t1 family=2 entries=1 op=nft_register_obj'
> +
> +do_test 'nft add counter t2 c1; add counter t2 c2' \
> +'table=t2 family=2 entries=2 op=nft_register_obj'
> +
> +# adding/updating quotas
> +
> +do_test 'nft add quota t1 q1 { 10 bytes }' \
> +'table=t1 family=2 entries=1 op=nft_register_obj'
> +
> +do_test 'nft add quota t2 q1 { 10 bytes }; add quota t2 q2 { 10 bytes }' \
> +'table=t2 family=2 entries=2 op=nft_register_obj'
> +
> +# changing the quota value triggers obj update path
> +do_test 'nft add quota t1 q1 { 20 bytes }' \
> +'table=t1 family=2 entries=1 op=nft_register_obj'
> +
> # resetting rules
>
> do_test 'nft reset rules t1 c2' \
> --
> 2.41.0
>
