Hi Pablo,
On Wed, Sep 27, 2023 at 09:41:53PM +0200, Pablo Neira Ayuso wrote:
> 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.
Exactly. I needed an internal __nft_obj_notify() which just serves
notify_list but not audit.
> > 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?
It's easier to to make nf_tables_obj_notify() skip the audit log and
not introduce special casing in nf_tables_commit_audit_log() or call
nf_tables_commit_audit_collect() for all transaction entries but NEWOBJ
and DELOBJ.
Cheers, Phil
