Re: [PATCH] netfilter: nfnetlink: always ACK batch end if requested

[Florian Westphal <fw@strlen.de>]

Donald Hunter <donald.hunter@gmail.com> wrote:
> Thanks for the cc. I'm having difficulty catching up with the thread
> of conversation, so forgive me if I miss something.
> 
> > Yes, I am inclined not to add more features to bf2ac490d28c (and
> > follow up fixes patches that came with it).
> 
> Is there a problem with bf2ac490d28c or just with things that have
> come after it?

it broke golang library for nftables:
https://github.com/google/nftables/issues/329

Fix is:
https://git.kernel.org/pub/scm/linux/kernel/git/netfilter/nf-next.git/commit/?id=09efbac953f6f076a07735f9ba885148d4796235

Current discussion centers more on what bf2ac490d28c wants,
what it does, what netlink can provide in practice and what
userspace would expect/need.

> > > > I suspect the author of bf2ac490d28c is making wrong assumptions on
> > > > the number of acknowledgements that are going to be received by
> > > > userspace.
> 
> Hopefully not. In the success case, ask for an ack, get an ack.
> Without that guarantee to userspace, we'd need to extend the YNL spec
> to say which messages don't honour acks.

Its not about the types, its more about available buffer space for acks.

> > > > #2 If you set NLM_F_ACK in your netlink messages in the batch:
> > > >    You get one acknowledgement for each message in the batch, with a
> > > >    sufficiently large batch, this may overrun the userspace socket
> > > >    buffer (ENOBUFS), then maybe the kernel was successful to fully
> > > >    process the transaction but some of those acks get lost.
> 
> Are people reporting ENOBUFS because of ACKs in practice or is this theoretical?

It happens in practice, e.g.:
https://lore.kernel.org/netfilter/aKrK6h2zYdqj2unR@lilyboudoir.localdomain/

(every set element triggers another error message).
Note, this is without NLM_F_ACK, just due to error messages
triggered during batch processing.

> I think we should try to specify the behaviour and see if it makes
> sense before layering more functionality onto what is there.

I think we can all agree on that :-)

> I can describe how the YNL python code sees the world, when it asks for ACKs:
> 
> 1. An ack for BEGIN, cmd, cmd, ... , END in the success scenario.
> 2. An ack for BEGIN, cmd, cmd, ... up to the first ERROR in a failure scenario.

Thanks for clarifying.

This should work, IFF the batch is small enough so receive buffer can
hold all acks.

nftables doesn't bail out on first error (transaction will fail and
no permanent changes are made, but it will move to next contained
message.