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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Thu, 18 Apr 2024 18:30:55 +0200
From: Pablo Neira Ayuso <pablo@...filter.org>
To: Donald Hunter <donald.hunter@...il.com>
Cc: netdev@...r.kernel.org, Jakub Kicinski <kuba@...nel.org>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>, Paolo Abeni <pabeni@...hat.com>,
	Jiri Pirko <jiri@...nulli.us>,
	Jacob Keller <jacob.e.keller@...el.com>,
	Jozsef Kadlecsik <kadlec@...filter.org>,
	netfilter-devel@...r.kernel.org, coreteam@...filter.org,
	donald.hunter@...hat.com
Subject: Re: [PATCH net-next v4 4/4] netfilter: nfnetlink: Handle ACK flags
 for batch messages

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@...il.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
>