[PATCH net] can: j1939: validate the EDPO packet number

[PATCH net] can: j1939: validate the EDPO packet number

[Quchaosheng]

ETP.CM_DPO is peer controlled and its packet number is accepted without
any validation.  It names the packet a transmission window starts at, and
j1939_xtp_rx_dat_one() places incoming data at "dat[0] - 1 + pkt.dpo", so
a value past the end of the message moves pkt.dpo beyond the reassembled
buffer.

The transfer still completes normally in that case.  With
pkt.dpo == pkt.total, a closing ETP.DT frame with dat[0] == 0 evaluates to
packet pkt.total - 1, which is the last in-order packet, so pkt.rx reaches
pkt.total and the receiver emits its own EOMA.  The local loopback of that
EOMA runs j1939_session_completed(), which asks
j1939_session_skb_get() for offset "pkt.dpo * 7" -- one full packet past
the end of the queued skb -- gets NULL, and passes it straight to
j1939_sk_recv(), which dereferences oskb->sk.

The trigger is deterministic and needs no hardware, no race and no memory
pressure.  All of it is reachable by an unprivileged user through a
virtual CAN interface:

  ETP.CM_RTS (size 1786)  -> total = 256 packets
  ETP.CM_DPO (packet 0)
  ETP.DT seq 1..255       -> packets 0..254, pkt.rx = 255
  ETP.CM_DPO (packet 256) -> pkt.dpo = 256, unvalidated
  ETP.DT seq 0            -> packet 255 == pkt.rx, completes the transfer

resulting in:

  BUG: kernel NULL pointer dereference, address: 0000000000000018
  RIP: 0010:j1939_sk_recv+0xaf/0x150
  Call Trace:
   <IRQ>
   j1939_session_completed+0x69/0x80
   j1939_xtp_rx_eoma+0xc1/0x170
   j1939_tp_recv+0x41b/0x4c0
   j1939_can_recv+0x1ad/0x290
  Kernel panic - not syncing: Fatal exception in interrupt

Reject an EDPO that names a packet at or past pkt.total and abort the
session with J1939_XTP_ABORT_BAD_EDPO_OFFSET, the SAE J1939-21 abort code
for exactly this case.  The check has to be ">=" and not ">": pkt.total is
a packet count, so the last valid packet is pkt.total - 1 and
"dpo == pkt.total" is already out of range -- it is the value used by the
trigger above.

Both roles tolerate the check because a transmitter derives the offset
from pkt.tx_acked, which is "cts_packet - 1" with cts_packet <= pkt.total,
so the largest value it ever announces is pkt.total - 1.

Also guard j1939_session_completed() against a NULL skb as defense in
depth, so that any future regression in the offset arithmetic degrades to
a dropped message instead of a kernel panic.  The only other caller of
j1939_session_skb_get(), j1939_simple_txnext(), already does this.

Tested on v7.3.0-rc5 under QEMU:

  - the frame sequence above panics an unpatched kernel in
    j1939_sk_recv() and is cleanly aborted by this patch
  - a full 1786 byte ETP transfer between two J1939 sockets still
    completes with the payload intact.  That transfer announces
    pkt.total - 1 in its last EDPO, so it exercises the boundary value
    one below the rejected one, and it runs the transmitter's own EDPO
    through the vcan loopback as well.

Fixes: 9d71dd0c7009 ("can: add support of SAE J1939 protocol")
Assisted-by: LLM
Signed-off-by: Quchaosheng <quchaosheng000406@163.com>
---
 net/can/j1939/transport.c | 29 +++++++++++++++++++++++++----
 1 file changed, 25 insertions(+), 4 deletions(-)

diff --git a/net/can/j1939/transport.c b/net/can/j1939/transport.c
index 8fcfd13e5..065b4f71e 100644
--- a/net/can/j1939/transport.c
+++ b/net/can/j1939/transport.c
@@ -1243,9 +1243,18 @@ static void j1939_session_completed(struct j1939_session *session)
 
 	if (!session->transmission) {
 		se_skb = j1939_session_skb_get(session);
-		/* distribute among j1939 receivers */
-		j1939_sk_recv(session->priv, se_skb);
-		consume_skb(se_skb);
+
+		/* The offset "pkt.dpo * 7" is not necessarily covered by a
+		 * queued skb, e.g. when a peer names a packet past the end of
+		 * the message in an EDPO.  j1939_simple_txnext() already
+		 * guards its call the same way; dropping the message is
+		 * better than handing a NULL skb to j1939_sk_recv().
+		 */
+		if (se_skb) {
+			/* distribute among j1939 receivers */
+			j1939_sk_recv(session->priv, se_skb);
+			consume_skb(se_skb);
+		}
 	}
 
 	j1939_session_deactivate_activate_next(session);
@@ -1823,14 +1832,34 @@ static void j1939_xtp_rx_dpo_one(struct j1939_session *session,
 				 struct sk_buff *skb)
 {
 	const u8 *dat = skb->data;
+	unsigned int dpo;
 
 	if (j1939_xtp_rx_cmd_bad_pgn(session, skb))
 		return;
 
 	netdev_dbg(session->priv->ndev, "%s: 0x%p\n", __func__, session);
 
+	dpo = j1939_etp_ctl_to_packet(dat);
+
+	/*
+	 * EDPO is peer controlled.  It names the packet the announced window
+	 * starts at, and j1939_xtp_rx_dat_one() places data at
+	 * "dat[0] - 1 + pkt.dpo", so it has to stay inside the message.  A
+	 * value past the end moves pkt.dpo beyond the reassembled buffer and
+	 * makes the ETP completion path look up offset "pkt.dpo * 7", which is
+	 * no longer covered by any queued skb.
+	 */
+	if (dpo >= session->pkt.total) {
+		netdev_warn(session->priv->ndev,
+			    "%s: 0x%p: EDPO %u out of range (total %u), aborting\n",
+			    __func__, session, dpo, session->pkt.total);
+		j1939_session_timers_cancel(session);
+		j1939_session_cancel(session, J1939_XTP_ABORT_BAD_EDPO_OFFSET);
+		return;
+	}
+
 	/* transmitted without problems */
-	session->pkt.dpo = j1939_etp_ctl_to_packet(skb->data);
+	session->pkt.dpo = dpo;
 	session->last_cmd = dat[0];
 	j1939_tp_set_rxtimeout(session, 750);
 

[PATCH net] net/packet: guard the ll header push in packet_rcv_spkt()

[Quchaosheng]

packet_rcv_spkt() restores the link layer header with

	skb_push(skb, skb->data - skb_mac_header(skb));

That subtraction is only meaningful when the device actually has a link
layer header and the producer initialised skb->mac_header.  A CAN skb
does not: init_can_skb() sets pkt_type and ip_summed but leaves
skb->mac_header at the 0xFFFF sentinel, because 9f10374bb024 ("can:
remove private CAN skb headroom infrastructure") dropped the
skb_reset_*_header() calls that used to be there.

skb_mac_header() is then 0xFFFF, the length becomes a large negative
number and skb_push() reports it through skb_under_panic() -- from
softirq context, so it is a full system panic even with panic_on_oops=0:

  skbuff: skb_under_panic: text:ffffffffadd28261 len:-65455 put:-65471 head:... data:... tail:0x50 end:0x180 dev:can0
  kernel BUG at net/core/skbuff.c:214!
  RIP: 0010:skb_panic+0x50/0x60
  Call Trace:
   <TASK>
   skb_push+0x4d/0x60
   packet_rcv_spkt+0xe1/0x170
   ...
  Kernel panic - not syncing: Fatal exception in interrupt

packet_rcv() and tpacket_rcv() already handle this: both wrap their
skb_push() in dev_has_header(dev).  packet_rcv_spkt() is the only
remaining receive path that does the subtraction unconditionally, and it
is reachable with SOCK_PACKET -- a socket type that still works and that
no length or capability check keeps away from a CAN interface.

The missing skb_reset_*_header() calls in init_can_skb() are a regression
in their own right and are being fixed separately, but a packet socket
should not turn a link layer that forgot to initialise its mac header into
a kernel panic.  Guard the push the same way the other two paths do.

Tested on v7.3.0-rc5 under QEMU with a slcan device on a pty (the driver
RX path is required; vcan resets the headers on the way out and does not
reproduce it).  A SOCK_PACKET socket on can0 panics an unpatched kernel
with the trace above and is silent with this patch applied.

Fixes: 9f10374bb024 ("can: remove private CAN skb headroom infrastructure")
Assisted-by: LLM
Cc: stable@vger.kernel.org
Signed-off-by: Quchaosheng <quchaosheng000406@163.com>
---
 net/packet/af_packet.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
index 7c83e0152..ab8e0309e 100644
--- a/net/packet/af_packet.c
+++ b/net/packet/af_packet.c
@@ -1911,7 +1911,17 @@ static int packet_rcv_spkt(struct sk_buff *skb, struct net_device *dev,
 
 	spkt = &PACKET_SKB_CB(skb)->sa.pkt;
 
-	skb_push(skb, skb->data - skb_mac_header(skb));
+	/* The push restores the link layer header, which only exists for
+	 * devices that have one.  A device without a visible ll header
+	 * (ARPHRD_CAN and other L3 types) must not be pushed, and the
+	 * subtraction is meaningless when the producer never initialised
+	 * skb->mac_header -- it then holds the 0xFFFF sentinel and the
+	 * result is a huge negative length that trips skb_under_panic() in
+	 * softirq context.  packet_rcv() and tpacket_rcv() already guard
+	 * this with dev_has_header(); do the same here.
+	 */
+	if (dev_has_header(dev) && skb_mac_header_was_set(skb))
+		skb_push(skb, skb->data - skb_mac_header(skb));
 
 	/*
 	 *	The SOCK_PACKET socket receives _all_ frames.

Re: [PATCH net] net/packet: guard the ll header push in packet_rcv_spkt()

[Oliver Hartkopp]

On 28.09.26 08:50, Quchaosheng wrote:
> 
> packet_rcv_spkt() restores the link layer header with
> 
> 	skb_push(skb, skb->data - skb_mac_header(skb));
> 
> That subtraction is only meaningful when the device actually has a link
> layer header and the producer initialised skb->mac_header.  A CAN skb
> does not: init_can_skb() sets pkt_type and ip_summed but leaves
> skb->mac_header at the 0xFFFF sentinel, because 9f10374bb024 ("can:
> remove private CAN skb headroom infrastructure") dropped the
> skb_reset_*_header() calls that used to be there.
> 
> skb_mac_header() is then 0xFFFF, the length becomes a large negative
> number and skb_push() reports it through skb_under_panic() -- from
> softirq context, so it is a full system panic even with panic_on_oops=0:
> 
>    skbuff: skb_under_panic: text:ffffffffadd28261 len:-65455 put:-65471 head:... data:... tail:0x50 end:0x180 dev:can0
>    kernel BUG at net/core/skbuff.c:214!
>    RIP: 0010:skb_panic+0x50/0x60
>    Call Trace:
>     <TASK>
>     skb_push+0x4d/0x60
>     packet_rcv_spkt+0xe1/0x170
>     ...
>    Kernel panic - not syncing: Fatal exception in interrupt
> 
> packet_rcv() and tpacket_rcv() already handle this: both wrap their
> skb_push() in dev_has_header(dev).  packet_rcv_spkt() is the only
> remaining receive path that does the subtraction unconditionally, and it
> is reachable with SOCK_PACKET -- a socket type that still works and that
> no length or capability check keeps away from a CAN interface.
> 
> The missing skb_reset_*_header() calls in init_can_skb() are a regression
> in their own right and are being fixed separately, but a packet socket
> should not turn a link layer that forgot to initialise its mac header into
> a kernel panic.  Guard the push the same way the other two paths do.
> 
> Tested on v7.3.0-rc5 under QEMU with a slcan device on a pty (the driver
> RX path is required; vcan resets the headers on the way out and does not
> reproduce it).  A SOCK_PACKET socket on can0 panics an unpatched kernel
> with the trace above and is silent with this patch applied.
> 
> Fixes: 9f10374bb024 ("can: remove private CAN skb headroom infrastructure")

I wonder if we should fix this in af_packet.c ?

There's already a patch waiting for upstream in the CAN subsystem, where 
the problem has originally been introduced:

[PATCH v2] can: restore skb header initialisations in init_can_skb()
https://lore.kernel.org/linux-can/20260917123716.63116-1-ndaugoing@gmail.com/

Best regards,
Oliver

> Assisted-by: LLM
> Cc: stable@vger.kernel.org
> Signed-off-by: Quchaosheng <quchaosheng000406@163.com>
> ---
>   net/packet/af_packet.c | 12 ++++++++++--
>   1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
> index 7c83e0152..ab8e0309e 100644
> --- a/net/packet/af_packet.c
> +++ b/net/packet/af_packet.c
> @@ -1911,7 +1911,17 @@ static int packet_rcv_spkt(struct sk_buff *skb, struct net_device *dev,
>   
>   	spkt = &PACKET_SKB_CB(skb)->sa.pkt;
>   
> -	skb_push(skb, skb->data - skb_mac_header(skb));
> +	/* The push restores the link layer header, which only exists for
> +	 * devices that have one.  A device without a visible ll header
> +	 * (ARPHRD_CAN and other L3 types) must not be pushed, and the
> +	 * subtraction is meaningless when the producer never initialised
> +	 * skb->mac_header -- it then holds the 0xFFFF sentinel and the
> +	 * result is a huge negative length that trips skb_under_panic() in
> +	 * softirq context.  packet_rcv() and tpacket_rcv() already guard
> +	 * this with dev_has_header(); do the same here.
> +	 */
> +	if (dev_has_header(dev) && skb_mac_header_was_set(skb))
> +		skb_push(skb, skb->data - skb_mac_header(skb));
>   
>   	/*
>   	 *	The SOCK_PACKET socket receives _all_ frames.
> 

[PATCH v2 net] net/packet: guard the ll header push in packet_rcv_spkt()

[Quchaosheng]

packet_rcv_spkt() restores the link layer header with

	skb_push(skb, skb->data - skb_mac_header(skb));

That subtraction is only meaningful when the device actually has a link
layer header.  packet_rcv() and tpacket_rcv() both wrap it in
dev_has_header(), which is also the predicate the block comment at the
top of the file states the restore in terms of; packet_rcv_spkt() does
not.  commit d549699048b4 ("net/packet: fix packet receive on L3
devices without visible hard header") introduced the helper and changed
the two call sites, and this one stayed behind.

A device without a visible ll header can leave skb->mac_header at the
0xFFFF sentinel that __alloc_skb() initialises it to.  A CAN skb does:
init_can_skb() sets pkt_type and ip_summed but does not reset the
headers, and commit 9f10374bb024 ("can: remove private CAN skb
headroom infrastructure") dropped the skb_reset_*_header() calls that
used to be there.  skb_mac_header() is then 0xFFFF, the length becomes
a large negative number and skb_push() reports it through
skb_under_panic() -- from softirq context, so it is a full system panic
even with panic_on_oops=0:

  skbuff: skb_under_panic: text:ffffffff8bd21bc1 len:-65455 put:-65471 head:... data:... tail:0x50 end:0x180 dev:can0
  kernel BUG at net/core/skbuff.c:214!
  RIP: 0010:skb_panic+0x50/0x60
  Call Trace:
   <IRQ>
   skb_push+0x38/0x40
   packet_rcv_spkt+0xe1/0x170
   __netif_receive_skb_core.constprop.0+0x7e8/0xd30
   ...
  Kernel panic - not syncing: Fatal exception in interrupt

The socket type is reachable: packet_create() accepts SOCK_PACKET
alongside SOCK_RAW and SOCK_DGRAM behind the same CAP_NET_RAW check,
and neither the socket length nor a capability check keeps it away
from a CAN interface.

The missing skb_reset_*_header() calls in init_can_skb() are a
regression in their own right and are being fixed separately, but a
packet socket should not turn a link layer that did not initialise its
mac header into a kernel panic.  Guard the push the way the other two
receive paths do.

Tested on v7.3-rc5 under QEMU with a slcan device on a pty, which is
the driver RX path: vcan does not reproduce it, because can_send()
resets the headers on the way out.  One SOCK_PACKET socket bound to
can0 and one frame written into the line discipline panics an
unpatched kernel with the trace above; the same image with this patch
prints no panic and powers off normally.  Both kernels are this tree,
defconfig plus CONFIG_CAN_SLCAN=y, differing only in this hunk.

Fixes: d549699048b4 ("net/packet: fix packet receive on L3 devices without visible hard header")
Assisted-by: LLM
Cc: stable@vger.kernel.org
Signed-off-by: Quchaosheng <quchaosheng000406@163.com>
---
Hello Oliver,

Yes -- af_packet.c is the right place, and your question made me cut the
patch back, so this is a v2.

You are right that the CAN side is already being fixed: I ran zjamg's v2
earlier today and verified it holds.  The two are not alternatives to each
other.  That patch restores the header initialisations the CAN stack lost,
which is the actual regression; this one is the packet socket that should
not panic when a link layer hands it an skb whose mac_header was never set.
Either one stops the crash on this path, but only the pair leaves the
producer correct and the receiver safe.

On your question about where it belongs: packet_rcv_spkt() is the third
call site of the same subtraction, and commit d549699048b4 changed the
other two to dev_has_header() and left this one alone.  It is still the
only one that does the subtraction unconditionally, and it is reachable
with SOCK_PACKET.  So this is that commit's missing hunk rather than a
second opinion on the CAN fix.

The v1 had an extra skb_mac_header_was_set(skb) conjunct and this version
drops it.  dev_has_header() alone is what the block comment at the top of
the file states the restore in terms of, and what packet_rcv() and
tpacket_rcv() test; for a device without a visible ll header the documented
invariant is that mac_header points at data, so the subtraction is a no-op
push and skipping it outright is the same thing.  CAN is the case where
that invariant does not hold, which is the panic; the extra conjunct only
would have masked it.  I have re-run both kernels with this version.

Details of the re-run, since the earlier numbers were from the v1: v7.3-rc5
under QEMU with slcan on a pty, defconfig plus CONFIG_CAN_SLCAN=y, two
images from one tree differing only in this hunk.  Unpatched: skb_under_panic
len:-65455, packet_rcv_spkt+0xe1, "Kernel panic - not syncing: Fatal
exception in interrupt".  With this patch: no panic, powers off normally.
checkpatch is clean apart from the unavoidable long line in the quoted trace.

Thanks,
Quchaosheng

 net/packet/af_packet.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
index 7c83e01526edc..40c67b86a1735 100644
--- a/net/packet/af_packet.c
+++ b/net/packet/af_packet.c
@@ -1911,7 +1911,14 @@ static int packet_rcv_spkt(struct sk_buff *skb, struct net_device *dev,
 
 	spkt = &PACKET_SKB_CB(skb)->sa.pkt;
 
-	skb_push(skb, skb->data - skb_mac_header(skb));
+	/* Only a device with a visible ll header can have it restored, which is
+	 * the test packet_rcv() and tpacket_rcv() already make.  For the others
+	 * the header is invisible and the subtraction has no meaning: a producer
+	 * that left skb->mac_header at its 0xFFFF sentinel turns it into a huge
+	 * negative length that trips skb_under_panic() in softirq context.
+	 */
+	if (dev_has_header(dev))
+		skb_push(skb, skb->data - skb_mac_header(skb));
 
 	/*
 	 *	The SOCK_PACKET socket receives _all_ frames.
-- 
2.43.0

Re: [PATCH v2 net] net/packet: guard the ll header push in packet_rcv_spkt()

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - How the issue was discovered, e.g. hit in production, hit during
   development, syzbot report, manual code inspection, LLM or static
   analysis tool scan.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.

Re: [PATCH v2 net] net/packet: guard the ll header push in packet_rcv_spkt()

[Quchaosheng]

Manual code inspection, and nothing reported it before this.

I was working out what else the CAN skb header regression (9f10374bb024)
reaches, and the packet socket turned out not to be covered by the CAN
patch: packet_rcv_spkt() is the one call site of the ll header restore
that d549699048b4 left without dev_has_header(), and SOCK_PACKET reaches
it.  It came out of reading the receive paths, not from a scan, a tool
or a report.

Because nothing had reported it, I built the reproduction before posting
rather than sending a theoretical path:

  v7.3-rc5 under QEMU, slcan on a pty (the driver RX path; vcan resets
  the headers in can_send() and does not reproduce it), defconfig plus
  CONFIG_CAN_SLCAN=y, two images from one tree differing only in the
  hunk.  Unpatched: skb_under_panic len:-65455 out of
  packet_rcv_spkt+0xe1 and a panic in interrupt.  Patched: no panic.

The v1 had an extra skb_mac_header_was_set() conjunct; v2 drops it after
Oliver asked whether af_packet.c was the right place for this, and both
kernels were re-run with the version posted here.

Re: [PATCH v2 net] net/packet: guard the ll header push in packet_rcv_spkt()

[Oliver Hartkopp]

On 28.09.26 10:09, Quchaosheng wrote:
> packet_rcv_spkt() restores the link layer header with
> 
> 	skb_push(skb, skb->data - skb_mac_header(skb));
> 
> That subtraction is only meaningful when the device actually has a link
> layer header.  packet_rcv() and tpacket_rcv() both wrap it in
> dev_has_header(), which is also the predicate the block comment at the
> top of the file states the restore in terms of; packet_rcv_spkt() does
> not.  commit d549699048b4 ("net/packet: fix packet receive on L3
> devices without visible hard header") introduced the helper and changed
> the two call sites, and this one stayed behind.
> 
> A device without a visible ll header can leave skb->mac_header at the
> 0xFFFF sentinel that __alloc_skb() initialises it to.  A CAN skb does:
> init_can_skb() sets pkt_type and ip_summed but does not reset the
> headers, and commit 9f10374bb024 ("can: remove private CAN skb
> headroom infrastructure") dropped the skb_reset_*_header() calls that
> used to be there.  skb_mac_header() is then 0xFFFF, the length becomes
> a large negative number and skb_push() reports it through
> skb_under_panic() -- from softirq context, so it is a full system panic
> even with panic_on_oops=0:
> 
>    skbuff: skb_under_panic: text:ffffffff8bd21bc1 len:-65455 put:-65471 head:... data:... tail:0x50 end:0x180 dev:can0
>    kernel BUG at net/core/skbuff.c:214!
>    RIP: 0010:skb_panic+0x50/0x60
>    Call Trace:
>     <IRQ>
>     skb_push+0x38/0x40
>     packet_rcv_spkt+0xe1/0x170
>     __netif_receive_skb_core.constprop.0+0x7e8/0xd30
>     ...
>    Kernel panic - not syncing: Fatal exception in interrupt
> 
> The socket type is reachable: packet_create() accepts SOCK_PACKET
> alongside SOCK_RAW and SOCK_DGRAM behind the same CAP_NET_RAW check,
> and neither the socket length nor a capability check keeps it away
> from a CAN interface.
> 
> The missing skb_reset_*_header() calls in init_can_skb() are a
> regression in their own right and are being fixed separately, but a
> packet socket should not turn a link layer that did not initialise its
> mac header into a kernel panic.  Guard the push the way the other two
> receive paths do.
> 
> Tested on v7.3-rc5 under QEMU with a slcan device on a pty, which is
> the driver RX path: vcan does not reproduce it, because can_send()
> resets the headers on the way out.  One SOCK_PACKET socket bound to
> can0 and one frame written into the line discipline panics an
> unpatched kernel with the trace above; the same image with this patch
> prints no panic and powers off normally.  Both kernels are this tree,
> defconfig plus CONFIG_CAN_SLCAN=y, differing only in this hunk.
> 
> Fixes: d549699048b4 ("net/packet: fix packet receive on L3 devices without visible hard header")
> Assisted-by: LLM
> Cc: stable@vger.kernel.org
> Signed-off-by: Quchaosheng <quchaosheng000406@163.com>
> ---
> Hello Oliver,
> 
> Yes -- af_packet.c is the right place, and your question made me cut the
> patch back, so this is a v2.
> 

Thanks for the explanation.

> You are right that the CAN side is already being fixed: I ran zjamg's v2
> earlier today and verified it holds.  The two are not alternatives to each
> other.  That patch restores the header initialisations the CAN stack lost,
> which is the actual regression; this one is the packet socket that should
> not panic when a link layer hands it an skb whose mac_header was never set.
> Either one stops the crash on this path, but only the pair leaves the
> producer correct and the receiver safe.
> 
> On your question about where it belongs: packet_rcv_spkt() is the third
> call site of the same subtraction, and commit d549699048b4 changed the
> other two to dev_has_header() and left this one alone.  It is still the
> only one that does the subtraction unconditionally, and it is reachable
> with SOCK_PACKET.  So this is that commit's missing hunk rather than a
> second opinion on the CAN fix.

Correct!

> 
> The v1 had an extra skb_mac_header_was_set(skb) conjunct and this version
> drops it.  dev_has_header() alone is what the block comment at the top of
> the file states the restore in terms of, and what packet_rcv() and
> tpacket_rcv() test; for a device without a visible ll header the documented
> invariant is that mac_header points at data, so the subtraction is a no-op
> push and skipping it outright is the same thing.  CAN is the case where
> that invariant does not hold, which is the panic; the extra conjunct only
> would have masked it.  I have re-run both kernels with this version.
> 
> Details of the re-run, since the earlier numbers were from the v1: v7.3-rc5
> under QEMU with slcan on a pty, defconfig plus CONFIG_CAN_SLCAN=y, two
> images from one tree differing only in this hunk.  Unpatched: skb_under_panic
> len:-65455, packet_rcv_spkt+0xe1, "Kernel panic - not syncing: Fatal
> exception in interrupt".  With this patch: no panic, powers off normally.
> checkpatch is clean apart from the unavoidable long line in the quoted trace.
> 
> Thanks,
> Quchaosheng
> 
>   net/packet/af_packet.c | 9 ++++++++-
>   1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
> index 7c83e01526edc..40c67b86a1735 100644
> --- a/net/packet/af_packet.c
> +++ b/net/packet/af_packet.c
> @@ -1911,7 +1911,14 @@ static int packet_rcv_spkt(struct sk_buff *skb, struct net_device *dev,
>   
>   	spkt = &PACKET_SKB_CB(skb)->sa.pkt;
>   
> -	skb_push(skb, skb->data - skb_mac_header(skb));
> +	/* Only a device with a visible ll header can have it restored, which is
> +	 * the test packet_rcv() and tpacket_rcv() already make.  For the others
> +	 * the header is invisible and the subtraction has no meaning: a producer
> +	 * that left skb->mac_header at its 0xFFFF sentinel turns it into a huge
> +	 * negative length that trips skb_under_panic() in softirq context.
> +	 */

Just a nitpick: AI mostly likes to introduce comments that repeat the 
argumentation already provided in the commit message itself.

I would suggest to remove this entire comment as the other "if 
(dev_has_header(dev))" call sites don't have a comment either.

Comments are needed to provide additional information when the code is 
not obvious - and not the development history including producer 
failures ;-)

> +	if (dev_has_header(dev))
> +		skb_push(skb, skb->data - skb_mac_header(skb));
>   
>   	/*
>   	 *	The SOCK_PACKET socket receives _all_ frames.

Many thanks and best regards,
Oliver