[AI] fix: stop Execute after ACK or Done

[mrIncompetent]

Execute could wait forever when conntrack sent a Multi reply followed by an ACK without Done.

For requests with Acknowledge, keep receiving until ACK or Done, following kernel YNL behavior.

Notes:

• AI wrote the commit with my steering. Assume the absolute worst.
• Some tests in github.com/mdlayher/ethtool will fail with this commit as those tests don't mock Error (0) .
• I ran this commit(all tests) against all kernel versions since v5.0, all pass except the cleanup fails on v5.0 due to a missing bugfix in the kernel release

[nickgarlis]

Hi @mrIncompetent

Thanks for the PR! I will try to review as soon as I can. Some initial questions after looking at it...

Execute could wait forever when conntrack sent a Multi reply followed by an ACK without Done.

Judging from your tests, you get a multi-part reply even though you do not set a Dump flag ? I wonder whether that's expected. What's also surprising is that the ACK terminates the multi-part reply. Have you verified that this is the behavior in other kernel versions ? Could it be a bug with the version that you're running ?

For requests with Acknowledge, keep receiving until ACK or Done, following kernel YNL behavior.

Indeed, they do seem to exit when an ACK is received, I wonder whether that's the reason for it.

Could you provide some more details about the issue ? Are you replicating the exact same requests that conntrack makes ?

Assuming the fix is correct, we should consider having it in the plain receive path as well.

[mrIncompetent]

Judging from your tests, you get a multi-part reply even though you do not set a Dump flag ?

Yes, when getting a single conntrack entry via github.com/mdlayher/netlink:

	conn, err := ct.Dial(&mdnetlink.Config{NetNS: int(routerNS)})
	if err != nil {
		t.Fatal(err)
	}
	defer conn.Close()

	var f ct.Flow
	f.TupleReply.IP.SourceAddress = netip.MustParseAddr("10.0.4.1")
	f.TupleReply.Proto.SourcePort = 9090
	f.TupleReply.IP.DestinationAddress = netip.MustParseAddr("10.0.4.2")
	f.TupleReply.Proto.DestinationPort = pair.clientPort
	f.TupleReply.Proto.Protocol = unix.IPPROTO_TCP

	got, err := conn.Get(f)
	if err != nil {
		t.Fatal(err)
	}

conn.Get only sets Request and Acknowledge:

	req, err := netfilter.MarshalNetlink(
		netfilter.Header{
			SubsystemID: netfilter.NFSubsysCTNetlink,
			MessageType: netfilter.MessageType(ctGet),
			Family:      pf,
			Flags:       netlink.Request | netlink.Acknowledge,
		}, attrs)

Have you verified that this is the behavior in other kernel versions ? Could it be a bug with the version that you're running ?

The added integration test does exactly that - admittingly the assertion code isn't the most obvious.
The test got executed on all kernels since v5.0 (Only the last patch release).
I can add the AI slop that runs the tests against a range of kernels tomorrow if desired.

Could you provide some more details about the issue ? Are you replicating the exact same requests that conntrack makes ?

Yup, see above snippet.

Assuming the fix is correct, we should consider having it in the plain receive path as well.

I can have a look!

[nickgarlis]

Seems like the kernel sets the NLM_F_MULTI when port_id != 0 for conntrack. It seems like NLM_F_MULTI may mean something else there.

From our side this might be a regression introduced in 5af0e4f. I'll try to understand more about this tomorrow. There might more netfilter subsystems that are affected.

The test got executed on all kernels since v5.0 (Only the last patch release).

👍

I can have a look!

I believe the fix would be to reset more in receiveLockedIter and discard any waitForAck logic.

for {
  more = false
  for m, err := range c.sock.ReceiveIter() {
  ...

Thanks for the detailed explanation!

[mrIncompetent]

@nickgarlis I'm very sorry for not getting back here!

I've tested the proposed minimal fix (more reset at loop begin) across the entire range of kernels locally & it passes.
It's pushed in 8d848ae

[nickgarlis]

Thanks for changing your approach. The tests look good!