Re: [PATCH net-next v2 2/3] netfilter: nfnetlink: Handle ACK flags for batch messages

[Donald Hunter <donald.hunter@gmail.com>]

Pablo Neira Ayuso <pablo@netfilter.org> writes:

> On Wed, Apr 10, 2024 at 11:11:07PM +0100, Donald Hunter wrote:
>> The NLM_F_ACK flag is not processed for nfnetlink batch messages.
>
> Let me clarify: It is not processed for the begin and end marker
> netlink message, but it is processed for command messages.

That's a good point - my apologies for not making it clear in my
description that it is only the batch begin and end messages where
NLM_F_ACK is ignored. All the command messages between the begin and end
messages do indeed give ack responses when requested. I will reword the
commit message to make this clear.

>> This is a problem for ynl which wants to receive an ack for every
>> message it sends. Add processing for ACK and provide responses when
>> requested.
>
> NLM_F_ACK is regarded for the specific command messages that are
> contained in the batch, that is:
>
> batch begin
> command
> command
> ...
> command
> batch end
>
> Thus, NLM_F_ACK can be set on for the command messages and it is not
> ignore in that case.
>
> May I ask why do you need this? Is it to make your userspace tool happy?

Yes, as I mentioned this is a problem for ynl and it would also be a
problem for any user space tool that is ynl spec driven, i.e. not
hard-coded with special cases for a given netlink family.

Previous conversation:

https://lore.kernel.org/netdev/20240329144639.0b42dc19@kernel.org/

>> 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.
>> I also ran a search on github and did not spot any usage that would
>> break.
>> 
>> 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..37762941c288 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;
>>  
>> @@ -463,6 +466,8 @@ static void nfnetlink_rcv_batch(struct sk_buff *skb, struct nlmsghdr *nlh,
>>  			goto done;
>>  		} else if (type == NFNL_MSG_BATCH_END) {
>>  			status |= NFNL_BATCH_DONE;
>> +			if (nlh->nlmsg_flags & NLM_F_ACK)
>> +				nfnl_err_add(&err_list, nlh, 0, &extack);
>
> if (status == NFNL_BATCH_DONE) should probably be a better place for
> this. I would like to have userspace that uses this, I don't have a
> usecase at this moment for this new code.

I looked at putting it there but when the code reaches the 'done'
processing, it is not obvious that nlh still refers to the correct
message header in the skb. It seemed more natural to process all acks
with nfnl_err_add() at the point where each message gets processed. I
can take another look at moving it there if you prefer.

The userspace that uses this is the ynl tool with the nftables spec and
--multi patch that is included in this patchset. I included an example
of how to use it in the cover letter:

https://lore.kernel.org/netdev/20240410221108.37414-1-donald.hunter@gmail.com/T/

Here's the example:

./tools/net/ynl/cli.py \
 --spec Documentation/netlink/specs/nftables.yaml \
 --multi batch-begin '{"res-id": 10}' \
 --multi newtable '{"name": "test", "nfgen-family": 1}' \
 --multi newchain '{"name": "chain", "table": "test", "nfgen-family": 1}' \
 --multi batch-end '{"res-id": 10}'

Thanks!