Re: [PATCH net-next 1/2] can: slcan: publish tty->disc_data only after register_candev()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] can: slcan: permanent TX queue stall due to lost tty_wakeup
--

commit 97d420d988d6c139c8f2ceadb7c0d2d8b4b77ede
Author: Chaosheng Qu <chaosheng.qu.linux@gmail.com>

can: slcan: publish tty->disc_data only after register_candev()

This commit modifies slcan_open() to assign the private slcan structure to
the tty layer only after the network device is successfully registered. The
goal is to simplify the error handling path so that the pointer does not
need to be cleared if register_candev() fails.

> diff --git a/drivers/net/can/slcan/slcan-core.c b/drivers/net/can/slcan/slcan-core.c
> index c9414faafe664..fc6ecf110aabf 100644
> --- a/drivers/net/can/slcan/slcan-core.c
> +++ b/drivers/net/can/slcan/slcan-core.c
> @@ -849,7 +849,6 @@ static int slcan_open(struct tty_struct *tty)
>  
>  	/* Mark ldisc channel as alive */
>  	sl->tty = tty;
> -	tty->disc_data = sl;
>  
>  	err = register_candev(dev);
>  	if (err) {
> @@ -858,6 +857,8 @@ static int slcan_open(struct tty_struct *tty)
>  		return err;
>  	}
>  
> +	tty->disc_data = sl;

[Severity: Medium]
Does this change introduce a regression that leads to a permanent TX queue
stall?

Because register_candev() drops rtnl_lock internally and makes the network
device visible to userspace, an administrator could bring up the interface
and transmit a CAN packet while slcan_open() is still executing.

If the transmit buffer fills up, slcan_netdev_xmit() stops the queue and
relies on the UART hardware calling tty_wakeup() to restart it.

However, since slcan_open() still holds the ldisc_sem write lock (acquired
during tty_set_ldisc()), tty_wakeup() will fail to acquire the read lock:

drivers/tty/tty_io.c:tty_wakeup() {
	...
	ld = tty_ldisc_ref(tty);
	if (ld) {
		if (ld->ops->write_wakeup)
			ld->ops->write_wakeup(tty);
		tty_ldisc_deref(ld);
	}
	...
}

Will this cause tty_ldisc_ref() to silently drop the wakeup, leaving the
netdev queue permanently stopped?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1792118038.git.chaosheng.qu.linux@gmail.com?part=1