| Message ID | 20240410221108.37414-3-donald.hunter@gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | netlink: Add nftables spec w/ multi messages | expand |
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 >
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 --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;
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(+)