Page MenuHomeFreeBSD

pf tests: make mbuf:inet6_in_mbuf_len more robust
ClosedPublic

Authored by kp on Fri, Jul 18, 12:37 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Jul 19, 4:02 PM
Unknown Object (File)
Sat, Jul 19, 3:51 PM
Unknown Object (File)
Sat, Jul 19, 11:57 AM
Unknown Object (File)
Fri, Jul 18, 7:13 PM

Details

Summary

The mbuf:inet6_in_mbuf_len test sometimes fails because it encountered
unexpected extra packets. These turn out to be MLD packets, so block these
packets on the host with pf so they don't disturb what we're actually trying
to test.

Sponsored by: Rubicon Communications, LLC ("Netgate")

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

kp requested review of this revision.Fri, Jul 18, 12:37 PM

Thank you very much for identifying the root cause. I've been monitoring the false positives on the CI since Oct-2024 -- 25 of them so far. And my TODO list is too deep.

Just thinking out loud. I'm thinking that we change pf rules outside, but for now we are covered with being jailed, I assume the test is not going to be used w/o execenv=jail. Or we could put it under "if execenv=jail" condition, just in case, not to leave the host with unexpected change. It seems checking sysctl security.jail.jailed is tricky as it does not tell us whether it is a temporary one to be removed by Kyua or something the kyua is actually running within. I need to check if execenv value is readable.
Probably, I was too strict to expect exactly one packet (it was relatively easy to expect that from IPv4). Maybe it is better to expect 1+ instead, in theory it could provide false negative, but it could be less cost and better balance of complexity.

But now I see that we have this interesting improvement instead: https://reviews.freebsd.org/D51409.

This revision is now accepted and ready to land.Fri, Jul 18, 1:45 PM

Thank you very much for identifying the root cause. I've been monitoring the false positives on the CI since Oct-2024 -- 25 of them so far. And my TODO list is too deep.

Very relatable :)

Just thinking out loud. I'm thinking that we change pf rules outside, but for now we are covered with being jailed, I assume the test is not going to be used w/o execenv=jail. Or we could put it under "if execenv=jail" condition, just in case, not to leave the host with unexpected change. It seems checking sysctl security.jail.jailed is tricky as it does not tell us whether it is a temporary one to be removed by Kyua or something the kyua is actually running within. I need to check if execenv value is readable.

I wouldn't worry about that. We do set 'execenv=jail', and we do a lot of this sort of thing in other pf tests.

Probably, I was too strict to expect exactly one packet (it was relatively easy to expect that from IPv4). Maybe it is better to expect 1+ instead, in theory it could provide false negative, but it could be less cost and better balance of complexity.

Yeah, it's a common gotcha. I wouldn't actually make the test less strict though. I like that we're being specific here. If we ever end up incorrectly duplicating the packet or something we should detect that too.

But now I see that we have this interesting improvement instead: https://reviews.freebsd.org/D51409.

To be clear, that doesn't actually fix this problem. I attempted to fix the problem by disabling MLD, but the stack still emits these packets sometimes, even with those sysctls set.
I posted that change because we may as well have it, but it's not actually enough by itself (or with an associated change to set the sysctls in this test case) to fix this intermittent failure.

This revision was automatically updated to reflect the committed changes.