Page MenuHomeFreeBSD

pf tests: Add 'mbuf' test for (*m0)->m_len < sizeof(struct ip) cases
ClosedPublic

Authored by igoro on Jul 9 2024, 10:39 AM.
Tags
None
Referenced Files
Unknown Object (File)
Sun, Oct 27, 7:51 PM
Unknown Object (File)
Sat, Oct 19, 11:55 AM
Unknown Object (File)
Wed, Oct 16, 4:29 PM
Unknown Object (File)
Oct 5 2024, 5:46 PM
Unknown Object (File)
Oct 4 2024, 9:08 PM
Unknown Object (File)
Oct 2 2024, 10:52 AM
Unknown Object (File)
Oct 1 2024, 11:19 PM
Unknown Object (File)
Oct 1 2024, 3:19 PM

Details

Test Plan
# kldload pf dummymbuf
# kyua test -k /usr/tests/sys/netpfil/pf/Kyuafile mbuf

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

igoro requested review of this revision.Jul 9 2024, 10:39 AM

The context:

I still need to go through the preceding patch, but I'm becoming more enthusiastic about this.

tests/sys/netpfil/pf/mbuf.sh
57

I'm not going to stop you doing this, but I wouldn't bother having separate tests for ipfw is or is not loaded. The other pf tests just test pf and ignore ipfw entirely.

79

That's probably better done through pft_set_rules.

91

That assumes we're always going to get epair0. Usually true, but not always, so that should use ${epair}b.

99

It might be good to have a reset counters command so we don't need to manually keep track of the number of packets between different test packets.

101

Is there a specific reason to use 19 here? If so it'd be nice to explain that in the comment.

This update covers the points raised so far.

igoro added inline comments.
tests/sys/netpfil/pf/mbuf.sh
57

Thanks for raising this and confirming the way. After that divert-to case someway I've made it like my personal burden. During work on this test case I had serious doubts, but it did not cost much for me to keep it. Now I think if there is a solid support of getting rid of it then it should simplify the test a lot for non-original-author maintainers.

79

Definitely. It's better to stick to the same subr interface in case of future changes, which could be applied at a single location.

91

Good catch.

99

It might be good to have a reset counters command so we don't need to manually keep track of the number of packets between different test packets.

Yes, sure, the sysctl is based on a usual counter handler with reset logic built-in.

101

Is there a specific reason to use 19 here? If so it'd be nice to explain that in the comment.

That's simply a corner case black box based testing approach. At the same time, from a white box perspective, the idea is that a missing last byte should distort the destination field of the header. I've added a respective comment. It does not prolong much the testing to have such sub-case covered, but let me know if it feels better to remove it.

igoro marked 5 inline comments as done.

A couple of improvements:

  • Fix the ubiquitous language used in this scope, i.e. stick to the inet term. The upcoming test cases will use inet6 and ethernet pfil naming.
  • Speed up the test (kind of x5 in my env) with a stricter timeout for the expected ping failure.
This revision was not accepted when it landed; it landed in state Needs Review.Aug 15 2024, 7:35 AM
This revision was automatically updated to reflect the committed changes.