diff mbox series

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

Message ID 20240410221108.37414-3-donald.hunter@gmail.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series netlink: Add nftables spec w/ multi messages | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for net-next
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 942 this patch: 942
netdev/build_tools success No tools touched, skip
netdev/cc_maintainers success CCed 8 of 8 maintainers
netdev/build_clang success Errors and warnings before: 953 this patch: 953
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 953 this patch: 953
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 17 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
netdev/contest fail net-next-2024-04-11--09-00 (tests: 961)

Commit Message

Donald Hunter April 10, 2024, 10:11 p.m. UTC
The NLM_F_ACK flag is not processed for nfnetlink batch messages. 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.

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(+)

Comments

Pablo Neira Ayuso April 10, 2024, 11:13 p.m. UTC | #1
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.

> 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?

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

Thanks.

>  			goto done;
>  		} else if (type < NLMSG_MIN_TYPE) {
>  			err = -EINVAL;
> -- 
> 2.43.0
>
Donald Hunter April 11, 2024, 10:03 a.m. UTC | #2
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!
diff mbox series

Patch

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);
 			goto done;
 		} else if (type < NLMSG_MIN_TYPE) {
 			err = -EINVAL;