Re: [PATCH net 17/19] can: isotp: use unconditional synchronize_rcu() in isotp_release()

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] If a CAN ISOTP socket is closed concurrently with the unregistration of its bound netdevice, `dev_get_by_index()` fails, causing `isotp_release()` to skip CAN RX filter unregistration. This permanently leaks the filter and leaves it active, leading to a Use-After-Free.
- [High] isotp_sendmsg() arbitrarily aborts an ongoing concurrent transmission if interrupted by a signal while waiting.
--

--- Patch [17]: [PATCH net 17/19] can: isotp: use unconditional synchronize_rcu() in isotp_release() ---
commit c0a31977a8d3b86aee8a7406999fdf381f2bbded
Author: Oliver Hartkopp <[email protected]>

can: isotp: use unconditional synchronize_rcu() in isotp_release()

isotp_notify() unregisters the CAN filters without waiting for a grace
period. This patch makes isotp_release() always call synchronize_rcu()
before cancelling the timers, closing a race condition where a timer
could be cancelled while an in-flight isotp_rcv() is still executing.

Closes: https://lore.kernel.org/linux-can/[email protected]/
Link: https://patch.msgid.link/[email protected]

> diff --git a/net/can/isotp.c b/net/can/isotp.c
> index c48b4a818297e..d30937345bcdc 100644
> --- a/net/can/isotp.c
> +++ b/net/can/isotp.c
> @@ -1237,11 +1237,18 @@ static int isotp_release(struct socket *sock)
>  						  SINGLE_MASK(so->txid),
>  						  isotp_rcv_echo, sk);
>  				dev_put(dev);
> -				synchronize_rcu();
>  			}
>  		}
>  	}
>  
> +	/* Always wait for a grace period before touching the timers below.
> +	 * A concurrent NETDEV_UNREGISTER may have already unregistered our
> +	 * filters and cleared so->bound in isotp_notify() without waiting
> +	 * for in-flight isotp_rcv() callers to finish, so this call must not
> +	 * be skipped just because so->bound is already 0 here.
> +	 */
> +	synchronize_rcu();
> +

[Severity: High]
This is a pre-existing issue, but I have a question about how the socket
teardown handles missing network devices.

If unlist_netdevice() removes the device from the index before isotp_release()
calls dev_get_by_index(), will the lookup fail?

net/can/isotp.c:isotp_release() {
    ...
    dev = dev_get_by_index(net, so->ifindex);
    if (dev) {
        ...
        can_rx_unregister(...);
        ...
    }
    ...
    synchronize_rcu();
}

If dev_get_by_index() fails, can_rx_unregister() is skipped. Since
isotp_release() also removes the socket from the notifier list, will the
subsequent NETDEV_UNREGISTER event also fail to unregister the filter?

If the filter remains active in the device's RCU-protected lists, does
synchronize_rcu() adequately protect against new readers? While it waits for
existing readers, could a new softirq reader start after synchronize_rcu()
returns, find the leaked filter, and cause a use-after-free by accessing the
freed socket?

Could this be mitigated by tracking the net_device directly with
netdevice_tracker?

>  	hrtimer_cancel(&so->txfrtimer);
>  	hrtimer_cancel(&so->txtimer);
>  	hrtimer_cancel(&so->rxtimer);

I also noticed another pre-existing issue in this file.

[Severity: High]
This isn't a bug introduced by this patch, but does the error handling in
isotp_sendmsg() safely manage concurrent senders?

If multiple threads attempt to send on the same socket, one thread may acquire
the ISOTP_SENDING state while another thread waits in
wait_event_interruptible(). If the waiting thread receives a signal:

net/can/isotp.c:isotp_sendmsg() {
    ...
    err = wait_event_interruptible(so->wait, so->tx.state == ISOTP_IDLE);
    if (err)
        goto err_event_drop;
    ...
err_event_drop:
    so->tx.state = ISOTP_IDLE;
    hrtimer_cancel(&so->txfrtimer);
    ...
}

Does jumping to err_event_drop unconditionally force so->tx.state to ISOTP_IDLE
and cancel the transmission timers, directly aborting the active thread's
transmission?

If the socket state is reset, new transmissions could start, but could hardware
echoes or FC frames from the aborted transmission still arrive and corrupt the
new transmission's state, since so->cfecho is not cleared?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=17