Re: [PATCH net] can: j1939: fix skb refcount underflow in session teardown paths

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] can: j1939: skb and sock leak in j1939_session_destroy() and j1939_session_skb_drop_old()
--

commit 48d3734ee23304ec7eb1426db9d761018f0ff9d3
Author: Tristan Madani <tristmd@gmail.com>

can: j1939: fix skb refcount underflow in session teardown paths

This commit removes the explicit skb_unref() calls before kfree_skb() in
j1939_session_destroy() and j1939_session_skb_drop_old() to avoid a
refcount underflow.

> diff --git a/net/can/j1939/transport.c b/net/can/j1939/transport.c
> index 8fcfd13e5e6f8..3173936bd6f6e 100644
> --- a/net/can/j1939/transport.c
> +++ b/net/can/j1939/transport.c
> @@ -277,11 +277,8 @@ static void j1939_session_destroy(struct j1939_session *session)
>  	WARN_ON_ONCE(!list_empty(&session->sk_session_queue_entry));
>  	WARN_ON_ONCE(!list_empty(&session->active_session_list_entry));
>  
> -	while ((skb = skb_dequeue(&session->skb_queue)) != NULL) {
> -		/* drop ref taken in j1939_session_skb_queue() */
> -		skb_unref(skb);
> +	while ((skb = skb_dequeue(&session->skb_queue)) != NULL)
>  		kfree_skb(skb);
> -	}
>  	__j1939_session_drop(session);
>  	netdev_put(session->priv->ndev, &session->priv_dev_tracker);
>  	j1939_priv_put(session->priv);
> @@ -342,8 +339,6 @@ static void j1939_session_skb_drop_old(struct j1939_session *session)
>  
>  	if ((do_skcb->offset + do_skb->len) < offset_start) {
>  		__skb_unlink(do_skb, &session->skb_queue);
> -		/* drop ref taken in j1939_session_skb_queue() */
> -		skb_unref(do_skb);
>  		spin_unlock_irqrestore(&session->skb_queue.lock, flags);
>  
>  		kfree_skb(do_skb);

[Severity: High]
Does removing skb_unref() in j1939_session_destroy() and
j1939_session_skb_drop_old() leak every session-queued sk_buff and its
associated socket reference?

Every sk_buff queued into session->skb_queue starts with skb->users set to 1
from allocation (via alloc_skb() in j1939_session_fresh_new() for RX
sessions, or j1939_sk_alloc_skb() in j1939_sk_send_loop() for TX sessions).

When the initial skb is queued in j1939_session_new(), it takes a second
reference while callers do not drop the initial allocation reference on
success:

net/can/j1939/transport.c:j1939_session_new() {
	...
	skb_queue_head_init(&session->skb_queue);
	skb_queue_tail(&session->skb_queue, skb_get(skb));
	...
}

net/can/j1939/transport.c:j1939_session_fresh_new() {
	...
	skb = alloc_skb(size + sizeof(struct can_skb_priv), GFP_ATOMIC);
	...
	session = j1939_session_new(priv, skb, size);
	if (!session) {
		kfree_skb(skb);
		return NULL;
	}

	/* alloc data area */
	skb_put_zero(skb, size);
	/* skb is recounted in j1939_session_new() */
	return session;
}

Similarly, in net/can/j1939/socket.c, j1939_sk_send_loop() allocates each TX
segment skb with skb->users == 1 and passes it to j1939_tp_send() ->
j1939_session_new() or to j1939_session_skb_queue(), which also increments
skb->users to 2 without the caller dropping the initial reference:

net/can/j1939/transport.c:j1939_session_skb_queue() {
	...
	skb_get(skb);
	skb_queue_tail(&session->skb_queue, skb);
	...
}

Because transient users of the queued skbs (such as j1939_session_tx_dat(),
j1939_simple_txnext(), j1939_session_completed(), and
j1939_xtp_rx_dat_one()) acquire their own reference via
j1939_session_skb_get_by_offset() or j1939_session_skb_get() and balance it
with consume_skb() or kfree_skb(), every skb resting in session->skb_queue
has a refcount of 2.

Without the skb_unref() calls in j1939_session_destroy() and
j1939_session_skb_drop_old() (or alternatively removing the extra skb_get()
in j1939_session_new() and j1939_session_skb_queue()), won't kfree_skb()
only decrement skb->users from 2 to 1, leaking the sk_buff and preventing
sock_wfree() from ever releasing sk->sk_wmem_alloc and the struct sock
reference on TX sessions?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261102155345.1267524-1-tristan@talencesecurity.com?part=1