Re: [PATCH net-next v4 4/4] netfilter: nfnetlink: Handle ACK flags for batch messages

[Pablo Neira Ayuso <pablo@netfilter.org>]

Hi Donald,

Apologies for a bit late jumping back on this, it took me a while.

On Thu, Apr 18, 2024 at 11:47:37AM +0100, Donald Hunter wrote:
> The NLM_F_ACK flag is ignored for nfnetlink batch begin and end
> messages. This is a problem for ynl which wants to receive an ack for
> every message it sends, not just the commands in between the begin/end
> messages.

Just a side note: Turning on NLM_F_ACK for every message fills up the
receiver buffer very quickly, leading to ENOBUFS. Netlink, in general,
supports batching (with non-atomic semantics), I did not look at ynl
in detail, if it does send() + recv() for each message for other
subsystem then fine, but if it uses batching to amortize the cost of
the syscall then this explicit ACK could be an issue with very large
batches.

Out of curiosity: Why does the tool need an explicit ack for each
command? As mentioned above, this consumes a lot netlink bandwidth.

> Add processing for ACKs for begin/end messages and provide responses
> when requested.
> 
> I have checked that iproute2, pyroute2 and systemd are unaffected by
> this change since none of them use NLM_F_ACK for batch begin/end.

nitpick: Quick grep shows me iproute2 does not use the nfnetlink
subsystem? If I am correct, maybe remove this.

Thanks!

One more comment below.

> Signed-off-by: Donald Hunter <donald.hunter@gmail.com>
> ---
>  net/netfilter/nfnetlink.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/net/netfilter/nfnetlink.c b/net/netfilter/nfnetlink.c
> index c9fbe0f707b5..4abf660c7baf 100644
> --- a/net/netfilter/nfnetlink.c
> +++ b/net/netfilter/nfnetlink.c
> @@ -427,6 +427,9 @@ static void nfnetlink_rcv_batch(struct sk_buff *skb, struct nlmsghdr *nlh,
>  
>  	nfnl_unlock(subsys_id);
>  
> +	if (nlh->nlmsg_flags & NLM_F_ACK)
> +		nfnl_err_add(&err_list, nlh, 0, &extack);
> +
>  	while (skb->len >= nlmsg_total_size(0)) {
>  		int msglen, type;
>  
> @@ -573,6 +576,8 @@ static void nfnetlink_rcv_batch(struct sk_buff *skb, struct nlmsghdr *nlh,
>  		} else if (err) {
>  			ss->abort(net, oskb, NFNL_ABORT_NONE);
>  			netlink_ack(oskb, nlmsg_hdr(oskb), err, NULL);
> +		} else if (nlh->nlmsg_flags & NLM_F_ACK) {
> +			nfnl_err_add(&err_list, nlh, 0, &extack);
>  		}
>  	} else {
>  		enum nfnl_abort_action abort_action;
> -- 
> 2.44.0
>