unmapped_block now denies the data segments instead of the handshake (tcpdatalen 1000-65535): 71 s -> 1.5 s, and an unfixed kernel panics on it

Superseeded by D59426

Makefiles:

• tests/sys/Makefile descends into tests/sys/amd, which this diff no longer creates.
• tests/sys/pmc/Makefile says SUBDIR+= amd. bsd.test.mk reserves SUBDIR for helper/data dirs; only TESTS_SUBDIRS gets include("amd/Kyuafile") in the parent Kyuafile, so as posted kyua test from /usr/tests are not run. And it's ungated: MK_PMC is default-on everywhere, the C tests need do_cpuid() from <machine/cpufunc.h> (amd64/i386 only) → aarch64/riscv/powerpc break. The architecture guard needs to be here.
• pmc_pidtrack_test is a .c file listed under ATF_TESTS_SH. atf.test.mk makes it depend on pmc_pidtrack_test.sh, which doesn't exist. ATF_TESTS_C is the correct one.

Please update to a recent version of the main branch. You created a new pmc/Makefile, but the tip of main has one already. As-is the patch does not apply, and is not self-sufficient. It also contains a build-host hack (include dir and lib dir pointing hardcoded to /usr/local, whereas ATF should be in the base and doesn't need that. I have seen several internal Jira IDs. Not sure if you want to have that in public. And tests are skipped unless amd.pmc.grouping.runtime is set (which nothing does in this patch). umcdf/amd_umcdf_common.h still uses /dev/cpuctl0 (do_cpuid() went into amd_pmc_test_common.h only). The family/model messages and the AuthenticAMD-25-00-1 fixture names still print hex where the map now has the names.

• End of last month I added some tests/sys/pmc tests. While the items in this review seem to be AMD specific, they are testing parts of the PMC subsystem. I would expect the tests in the pmc directory (if you want in an amd subdirectory, but I have already some amd specific tests in the main OMC directory), than as it is vice versa.
• Is it feasible to use speaking names for model in amd_umcdf_map_zen()?
• hwpmc_exterr_test.c is a near-copy of the in-tree tests/sys/pmc/pmc_exterr_test.c. 13 of 14 case names identical; adds amd_missing_pmu_flag, drops amd_invalid_df_config_bits. Copyright/author line changed from the original.
• The directory never builds. tests/sys/Makefile gains TESTS_SUBDIRS+= ${_amd} but _amd is defined nowhere in the diff (the _pmc/_audit pattern needs the .if … _amd= amd .endif block, and _pmc is also gated on MK_PMC, which this isn't).
• tests/sys/amd/pmc/Makefile starts with a UTF-8 BOM and has no trailing newline. make may choke on the BOM.
• Content for other diffs shipped here. msr_snapshot.[ch] is not compiled ("see follow-on diff").
• The two umcdf/ headers seem to belong to to something that isn't in this diff, yet five pmc tests #include "../umcdf/amd_umcdf_common.h" for Zen-generation decoding. Not sure if the decde should belong together with amd_pmc_test_common.h or with this something else not included here.
• hwpmc_rdpmc_test is in the Makefile and the diff but not in the summary.
• Roughly half the suite is not AMD-specific. The callchain-UR regression (af3929c5152b, 66118c3f1011 — both vendor-neutral hwpmc_mod.c fixes), pmcstat TSC column, group-alloc atomicity, pidtrack, pmc(8) CLI smoke: they only use AMD event names. Under tests/sys/pmc/ with a per-vendor event table they'd cover Intel too. Having this in a vendor-specific subdirectory would need a reorg to add intel support. Better to not use a vendor specific directory (I do not ask someone from AMD to provide the Intel parts, but not making it harder to add them would be polite).
• Vendor detection via /dev/cpuctl0 (needs cpuctl.ko + root) in the C tests vs. kern.hwpmc.cpuid in the sh tests. <machine/cpufunc.h> do_cpuid() needs no device.
• AMDID2_* redefined instead of <machine/specialreg.h>.
• "../amd_pmc_test_common.h" relative includes and -I${SRCTOP}/tests/sys/amd — one is redundant.

I forgot to add the revision in the commit...
https://cgit.freebsd.org/src/commit/?id=26248c370fade359e5303094fb57102d7fcc9c78

Limit the SNAP pullup length via min(m_pkthdr.len, ETHER_HDR_LEN + sizeof(struct llc)), per gallatin@.

The bad: label in the previous revision gets called for ether types 0x88e1 (HomePlug AV management frames) and 0x8912 (not named in sys/net/ethernet.h) via the default: case and with net.link.bridge.pfil_onlyip=1. This looks to me like a policy drop and not an error path. As such I removed that part. Correct me if I get this wrong... (447 such packets in 300s doesn't not sound like an error to me).

rework: the bad: counter is removed; only the fragmentation path is counted

The following part may need to get a rework for this change... investigating...

In D59332#1362525, @markj wrote:

There are a couple of other places in if_bridge where we m_copyup() up to max_protohdr bytes. Do we need to fix those as well? m_copyup() doesn't handle extpg mbufs, though it wouldn't be hard to just fix that directly.

In D59389#1362863, @glebius wrote:

Ok, this mess mostly comes from me. While reviewing this patch I came to alternative that I will post soon.

D59389 has a similar fix for ipfw.

Unit test for the issue in D59389.

Narrowed per markj@: pull up ETHER_HDR_LEN first, the SNAP/LLC header only when the frame is long enough to carry one. No mb_unmapped_to_ext() call. Rebased on current main.

In D59332#1361528, @netchild wrote:

After a bit more digging around. This does not seem only to be a particular problem for the bridge, this is a problem for everything pfil related...
ipfw_check_frame_mbuf() seems to have the same bug (depends on net.link.ether.ipfw=1).
pf's ethernet hook seems to be safe at first look (m_copydata()).
if_enc passes the chain without touching, safe.
dummynet: pulls 14 bytes, so probably safe.
ipfilter: no idea, maybe.

This is a test for the fix in D59332

Unit tests in D59333

In D59204#1360863, @gallatin wrote:

In D59204#1360536, @netchild wrote:

The patchset is now running on the host which needs the watchdog to trigger from time to time. It needs a while to run into the issue.

Any idea how long this will take to trigger? I'd really like this patch to be pushed, as my test machines are all delivering spurious WD resets and need this patch merged (and a follow-on of my own to fix the world for ktls as well)

This fixes the hang at boot I've seen.

But don't we want the second one to fail?
Why do I want the second one to succeed?
When I run a non-svc start in parllel, I would want only one to succeed (if we think about nginx, or mysql or such).
What if someone hits ctrl-c after taking the lock?

The patchset is now running on the host which needs the watchdog to trigger from time to time. It needs a while to run into the issue.

My build is still running on the old hardware where I can test this.
Quick check, if I get it right:

• this solves the issue where demand is false on the next period.
• this does not solve an issue where ~1 packet per period is send while more than one is in the queue (no idea if this is behavior we can see in the real world): growth every period → demand true every period → armed reaches 4 → still resets.

In D59102#1356034, @markj wrote:

In D59102#1355583, @netchild wrote:

	if (PMC_SYSTEM_SAMPLING_ACTIVE() || PMC_PROC_IS_USING_PMCS(p)) {
		VOP_UNLOCK(imgp->vp);
-		pe.pm_credentialschanged = credential_changing;
+		pe.pm_credentialschanged = imgp->credential_setid;

filemon already does it this way — filemon_wrapper.c:127-129 pairs the same priv_check_cred(cred, PRIV_DEBUG_DIFFCRED) with imgp->newcred != NULL && imgp->credential_setid.
With this change, my 32 test cases pass (= no regression to what we have right now, except the security issue gone).

Yes, thanks. Probably exec should just be passing imgp directly to these hooks. (HWT has a nearly duplicate hook, but it doesn't seem to check for setuid/setgid switches. OTOH it is currently only usable by privileged users, at least today. Something to fix, but not in this patch.)

My tests detect a regression:

Comments reworded, a part moved into the commit log. The single (after f6afed726b00 ) goto moved into the one place where it matters.

Make the padding explicit.

Move the variables instruct iflib_txq to a place which causes less padding. Make the existing padding in the struct visible. Checked for amd64, riscv64, arm64, and i386 (only arch where the sizeof() changes, follow-up commit to remove the "old" watchdog parts changes the size back.