Re: [PATCH net v7 3/4] can: j1939: drop the message when the reassembled skb is gone

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The new NULL branch in j1939_session_completed()
  (net/can/j1939/transport.c) drops the reassembled message without…
- [Low] This patch makes the comment in j1939_xtp_rx_dpo_one()
  (net/can/j1939/transport.c, lines 1871-1883) out of date.

--

can: j1939: drop the message when the reassembled skb is gone

This adds a NULL check in j1939_session_completed() on the skb returned by
j1939_session_skb_get(). A failed lookup now drops the reassembled message
instead of passing NULL to j1939_sk_recv(). The commit message says
pkt.dpo is already bounded when an EDPO is received, so this is defense in
depth.

> diff --git a/net/can/j1939/transport.c b/net/can/j1939/transport.c
> index b0e62822b56e4..5f05c15114ce4 100644
> --- a/net/can/j1939/transport.c
> +++ b/net/can/j1939/transport.c
> @@ -1269,9 +1269,19 @@ 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);
> +
> +		/* Defense in depth: pkt.dpo is range checked when an EDPO is
> +		 * received, so the lookup should always find the skb.  Drop the
> +		 * message rather than hand a NULL skb to j1939_sk_recv() if the
> +		 * offset arithmetic ever regresses.  The only other caller of
> +		 * j1939_session_skb_get(), j1939_simple_txnext(), already does
> +		 * this.
> +		 */
> +		if (se_skb) {
> +			/* distribute among j1939 receivers */
> +			j1939_sk_recv(session->priv, se_skb);
> +			consume_skb(se_skb);
> +		}

[Severity: Low]
If the offset arithmetic does regress, would anyone notice?

When se_skb is NULL, the message is dropped with no WARN. session->err
stays at zero, and nothing is queued for userspace. The only output is the
netdev_dbg() in j1939_session_skb_get_by_offset(), which is off unless
dynamic debug is enabled.

On the userspace side, j1939_session_destroy() only reports RX_ABORT when
session->err is set:

	} else if (session->err) {
			j1939_sk_errqueue(session, J1939_ERRQUEUE_RX_ABORT);
	}

A receiver using SOF_TIMESTAMPING_RX_SOFTWARE would see
J1939_ERRQUEUE_RX_RTS, and possibly RX_DPO. After that it would get
neither the data nor an RX_ABORT.

The commit message also says:

    a dropped message is a lost datagram.

For an RTS/CTS receive session, j1939_session_completed() is reached from
j1939_xtp_rx_eoma_one() when the receiver's own EOMA comes back on
loopback. By then the originator has already been told the message was
delivered, so the two sides would disagree about delivery.

This branch cannot be reached in the current tree.
j1939_xtp_rx_dpo_one() rejects dpo >= session->pkt.total before storing
it, and TP and BAM sessions keep pkt.dpo at 0. The concern is that the
guard would not catch the regression it was added for.

Would WARN_ON_ONCE(!se_skb) plus setting session->err (for example to
-EBADMSG) fit better here? That would show the condition in the logs and
deliver the RX_ABORT errqueue notification.

[Severity: Low]
This isn't a bug, but this change makes a comment in
j1939_xtp_rx_dpo_one() out of date. That comment was added by "can: j1939:
validate the EDPO packet number" and still says:

	 * j1939_session_completed(), which asks j1939_session_skb_get() for a
	 * packet past the end of the queued skb, gets NULL, and passes it to
	 * j1939_sk_recv().

With the new if (se_skb) check, a failed lookup no longer reaches
j1939_sk_recv(). Without the EDPO bound, the message would now be
silently dropped after the receiver's EOMA has been sent.

Could that comment be updated along with this change? The last patch in
the series, "can: j1939: reject a CTS that names more packets than the
message has", only touches j1939_xtp_rx_cts_one(). The comment is still
stale at the end of the series.

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/179410166344.60581.3898011861151549156%40163.com