We have spent a fair bit of effort trying to remove these calls when possible. They are expensive and add a new failure mode. For instance, it looks like there's a bug here: we're dropping a packet but not incrementing any error counters.

Can we avoid the problem by only pulling up sizeof(struct ether_header) bytes at first, and then sizeof(struct llc) bytes later if needed?

Other areas in the bridge code seem to drop packets without increasing counters too.

I think this could be modeled here in the bridge code in a way like pf does it already in another place. But this then opens up the possibility that other places run into that same panic (see in my previous comments).

I can test easily on the system where I got the panic with pf. I have a look at coming up with something less expensive.

I don't think this comment is useful. It's pretty common in the network stack to only pull up the exact number of bytes you need.

Removed locally.

Why check the full packet length? The old code doesn't do that, m_pullup() would have failed in this case. So you're changing the behaviour here, but it doesn't seem intentional.

I *think* the old code never reached that failure (min(m_pkthdr.len, max_protohdr)). For a small enough size we would get a m_pullup failed now.
A 802.1D topology-change BPDU is 14 + 3 + 4 = 21 bytes, less than ETHER_HDR_LEN + sizeof(struct llc).

I've just let Claude run some tests, not the panic trigger case, but test cases to exercise this code path (the highlight for C1 and C2 is automatic, not by me, no markup for them).
C1 is the control that proves bridge_pfil() is actually on the path (flipping pfil_onlyip alone changes the verdict),
C2 is a frame of the same 802.3 shape when it is long enough.
T is the datum: a 21-byte 802.3/LLC frame.

main without patches:

kernel  GENERIC, main-n288870-d1c07141dccf (golden image, unpatched)
C1  onlyip=1 len=21  -> DROP (expected DROP)   frames=0   "m_pullup failed"=0   OK
C2  onlyip=0 len=60  -> PASS (expected PASS)   frames=1   "m_pullup failed"=0   OK
T   onlyip=0 len=21  -> PASS (expected PASS)   frames=1   "m_pullup failed"=0   OK

This patch:

kernel  BRIDGEA, bridge-pfil-extpg-n288731-1e1f7bd9bb99 (staged 0041, guard present)
C1  onlyip=1 len=21  -> DROP (expected DROP)   frames=0   "m_pullup failed"=0   OK
C2  onlyip=0 len=60  -> PASS (expected PASS)   frames=1   "m_pullup failed"=0   OK
T   onlyip=0 len=21  -> PASS (expected PASS)   frames=1   "m_pullup failed"=0   OK

Changed patch in the sense of:

-		if ((*mp)->m_len < ETHER_HDR_LEN + sizeof(struct llc) &&
-		    (*mp)->m_pkthdr.len >= ETHER_HDR_LEN + sizeof(struct llc)) {
+		if ((*mp)->m_len < ETHER_HDR_LEN + sizeof(struct llc)) {

Result:

kernel  BRIDGEB, bridge-pfil-extpg-n288731-1e1f7bd9bb99-dirty (staged 0041, guard present)
C1  onlyip=1 len=21  -> DROP (expected DROP)   frames=0   "m_pullup failed"=1   OK
C2  onlyip=0 len=60  -> PASS (expected PASS)   frames=1   "m_pullup failed"=0   OK
T   onlyip=0 len=21  -> DROP (expected PASS)   frames=0   "m_pullup failed"=1   OK

So the code in question makes sure the behaviour does not change.

The test packet is handcrafted to test the behavior, it may not show up like that in a real system. It is supposed to test the behavior of the bridge code.

The test packet:

ff ff ff ff ff ff  02 00 00 00 00 01  00 07     dst, src, 802.3 length = 7
42 42 03                                        LLC DSAP/SSAP 0x42, control UI
00 00 00 80                                     802.1D TCN BPDU

What Claude tells about it: 14 + 3 + 4 = 21 bytes is exactly an 802.1D topology-change BPDU; an LLC XID or TEST command is shorter still. Two conditions gate reachability, both worth stating in the reply: the frame must be unpadded, which rules out anything off a real NIC (padded to 60) and rules in epair/tap/vxlan/bpf writes — what a bridge is made of; and it must not carry the 802.1D group address, which bridge_forward() drops before the filter, so the test uses broadcast.

Two by-products: on B even the C1 drop now arrives through a freed chain rather than the pfil_onlyip policy check, because the pullup runs first, and bridge_pfil()'s printf is unconditional, so the drop is also a write to the console, triggered by remote input.

And about the "unpadded" part above, the real world (partly unmapped) chain which triggers the kernel panic which triggered me into looking into this is about a 54 bytes small part of the chain, from nginx with KTLS send into the bridge:

m_pullup (n=0xfffff8054e572b00, n@entry=0xfffff80ee5695500, len=6)
        m = 0xfffff80ee5695500, space = 104, count = 60
bridge_pfil (bifp=vswitch0, ifp=j_rp_hif, dir=PFIL_IN)

kgdb printed the two mbufs. Head: m_len 54, m_pkthdr.len 925, mapped. Next: m_data NULL, m_len 871, m_flags 265 = M_EXTPG|M_RDONLY|M_EXT, with m_epg_record_type 23 and extpg_hdr 17 03 03 03 62 — a TLS 1.2 application-data record of length 0x362, i.e. KTLS. 5 + 849 + 17 = 871, and 54 + 871 = 925.

I don't know anything about llc. But remember that m_pullup will fail if you ask it to pull up more bytes than are in the mbuf chain. So it seems to be the case that a valid llc can be less than the size of the struct, in which case, m_pullup(m, 22) is naturally going to fail on a 21 byte chain, and you need to parse the size and run the pullup on the actual size. Your check seems to somehow be accidentally making the tests pass by avoiding the pullup.

The old code didn't ask in that situation too.

Old: "i = min((*mp)->m_pkthdr.len, max_protohdr)"

To me it looks like the m_pullup is not needed in the particular case. And removing that check would drop the packet instead of letting it pass.
This patch, test in previous comment:
T onlyip=0 len=21 -> PASS (expected PASS) frames=1 "m_pullup failed"=0 OK
Without the check, test in previous comment:
T onlyip=0 len=21 -> DROP (expected PASS) frames=0 "m_pullup failed"=1 OK
Vs the current main code:
T onlyip=0 len=21 -> PASS (expected PASS) frames=1 "m_pullup failed"=0 OK

I may misunderstand something here. Feel free to point to something bluntly, I'm willing to learn.

I also asked Claude some questions (and gave my and your reply),to get a better understanding of what I may be overlooking... the answer supports the need of that check:
---snip---
The code he is asking about is the second pullup the narrowed fix adds:

	if ((*mp)->m_len < ETHER_HDR_LEN + sizeof(struct llc) &&
	    (*mp)->m_pkthdr.len >= ETHER_HDR_LEN + sizeof(struct llc)) {
		*mp = m_pullup(*mp, ETHER_HDR_LEN + sizeof(struct llc));

He is right that m_pullup() would fail. The question this answers is
what "fail" costs: a crash, or a dropped frame — and whether any real
frame is in the window at all.

It drops the frame. It does not crash. m_pullup() frees the chain
and returns NULL, bridge_pfil() prints m_pullup failed and returns
-1, and all six call sites return/continue without touching *mp,
so there is no double free. Measured on an INVARIANTS + WITNESS
kernel, which would have caught one.

And the drop is a behaviour change, not a no-op. Old code passed the
frame; the guard is what keeps that true. Removing it makes a filtering
bridge start dropping frames it used to forward.
---snip---

Another answer regarding the "accidental":
---snip---
sizeof(struct llc) is 8 — if_llc.h says so itself: CTASSERT(sizeof (struct llc) == LLC_SNAPFRAMELEN), sitting next to LLC_UFRAMELEN 3, LLC_ISFRAMELEN 4, LLC_FRMRLEN 7 and the comment "Don't use sizeof(struct llc_un) for LLC header sizes". So yes, a valid LLC header can be 3 bytes and a valid frame shorter than the struct.

But there is no size to parse. LLC carries no header-length field; the only length available is the frame length, which in a chain is m_pkthdr.len — and that is what the check reads. The pullup exists solely to read llc_un.type_snap.ether_type, 8 bytes in; a frame with fewer than 8 LLC bytes cannot be SNAP, so there is nothing to make contiguous. The code doesn't look at it either: the read stays gated by m_len >= ETHER_HDR_LEN + sizeof(struct llc), which is the original test, unchanged — it was spelled with a literal 8.

It isn't accidental. The old i = min((*mp)->m_pkthdr.len, max_protohdr) is this check. The min() is exactly why the old code could never ask m_pullup() for more than the chain holds, and so never reached the free-on-short-chain path; once the requested length becomes a constant, a clamp has no expression left except a precondition. The equivalence is exact: a 21-byte frame split 14 + 7 gets pulled up to 21 by the old code, then fails m_len >= 22 and skips the SNAP read; the patch skips the pullup and fails the same test. Same verdict, one copy fewer. And that is what the T row measures — stock passes the 21-byte frame, the patch passes it, the patch without the term drops it.

If the old shape reads better, m_pullup(*mp, MIN((*mp)->m_pkthdr.len, ETHER_HDR_LEN + sizeof(struct llc))) is behaviourally identical and makes the heritage visible — it just copies a frame the following test discards anyway.
---snip---

So all in all, I do not understand how this would change behaviour or is accidental. Please be direct/blunt/... to point out what is not ok, so this crash which is currently possible in -current gets fixed the right way (you have way more experience with the network code than I do... I have just a little toe in that right now).

I still think the current code is broken due to the (*mp)->m_pkthdr.len >= ETHER_HDR_LEN + sizeof(struct llc)) check.
You could fail to pullup a hand-crafted packet that is ETHER_HDR_LEN + sizeof(struct llc)) -1 bytes long if you have, say, ETHER_HDR_LEN bytes in the first mbuf.

I think the min is a good idea.. eg, m_pullup(min(m->pkthdr.len bytes, sizeof (struct llc))