On Wed, Oct 25, 2023 at 10:46:08PM +0200, Pablo Neira Ayuso wrote:
> On Wed, Oct 25, 2023 at 10:08:26PM +0200, Phil Sutter wrote:
> > In theory, dumpreset may fail and invalidate the preceeding log message.
> > Fix this and use the occasion to prepare for object reset locking, which
> > benefits from a few unrelated changes:
> >
> > * Add an early call to nfnetlink_unicast if not resetting which
> > effectively skips the audit logging but also unindents it.
> > * Extract the table's name from the netlink attribute (which is verified
> > via earlier table lookup) to not rely upon validity of the looked up
> > table pointer.
> > * Do not use local variable family, it will vanish.
> >
> > Signed-off-by: Phil Sutter <phil@xxxxxx>
> > ---
> > net/netfilter/nf_tables_api.c | 28 +++++++++++++---------------
> > 1 file changed, 13 insertions(+), 15 deletions(-)
> >
> > diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
> > index 3c1fd8283bf4..d0f7274b7ffe 100644
> > --- a/net/netfilter/nf_tables_api.c
> > +++ b/net/netfilter/nf_tables_api.c
> > @@ -7767,6 +7767,7 @@ static int nf_tables_dump_obj_done(struct netlink_callback *cb)
> > static int nf_tables_getobj(struct sk_buff *skb, const struct nfnl_info *info,
> > const struct nlattr * const nla[])
> > {
> > + const struct nftables_pernet *nft_net = nft_pernet(info->net);
> > struct netlink_ext_ack *extack = info->extack;
> > u8 genmask = nft_genmask_cur(info->net);
> > u8 family = info->nfmsg->nfgen_family;
> > @@ -7776,6 +7777,7 @@ static int nf_tables_getobj(struct sk_buff *skb, const struct nfnl_info *info,
> > struct sk_buff *skb2;
> > bool reset = false;
> > u32 objtype;
> > + char *buf;
> > int err;
> >
> > if (info->nlh->nlmsg_flags & NLM_F_DUMP) {
> > @@ -7814,27 +7816,23 @@ static int nf_tables_getobj(struct sk_buff *skb, const struct nfnl_info *info,
> > if (NFNL_MSG_TYPE(info->nlh->nlmsg_type) == NFT_MSG_GETOBJ_RESET)
> > reset = true;
> >
> > - if (reset) {
> > - const struct nftables_pernet *nft_net;
> > - char *buf;
> > -
> > - nft_net = nft_pernet(net);
> > - buf = kasprintf(GFP_ATOMIC, "%s:%u", table->name, nft_net->base_seq);
> > -
> > - audit_log_nfcfg(buf,
> > - family,
> > - 1,
> > - AUDIT_NFT_OP_OBJ_RESET,
> > - GFP_ATOMIC);
> > - kfree(buf);
> > - }
> > -
> > err = nf_tables_fill_obj_info(skb2, net, NETLINK_CB(skb).portid,
> > info->nlh->nlmsg_seq, NFT_MSG_NEWOBJ, 0,
> > family, table, obj, reset);
> > if (err < 0)
> > goto err_fill_obj_info;
> >
> > + if (!reset)
> > + return nfnetlink_unicast(skb2, net, NETLINK_CB(skb).portid);
>
> More simple with?
>
> if (reset) {
> buf = kasprintf(GFP_ATOMIC, "%.*s:%u",
> nla_len(nla[NFTA_OBJ_TABLE]),
> (char *)nla_data(nla[NFTA_OBJ_TABLE]),
> nft_net->base_seq);
> audit_log_nfcfg(buf, info->nfmsg->nfgen_family,
> 1, AUDIT_NFT_OP_OBJ_RESET, GFP_ATOMIC);
> kfree(buf);
> }
>
> return nfnetlink_unicast(skb2, net, NETLINK_CB(skb).portid);
>
> single call to nfnetlink_unicast().
Oh I see. It goes away in patch 3/3.
- if (!reset)
- return nfnetlink_unicast(skb2, net, NETLINK_CB(skb).portid);
