Re: [PATCH net-next 2/2] can: can327: 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] can327: lost tty_wakeup due to netdev registration before ldisc_sem unlock
--

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

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

This commit modifies the can327 line discipline to assign tty->disc_data
only after register_candev() completes successfully. This prevents the
error path from unwinding while leaving a stale pointer in tty->disc_data
if device registration fails.

> diff --git a/drivers/net/can/can327.c b/drivers/net/can/can327.c
> index 90f5e35f3c8f9..d40dadbb57330 100644
> --- a/drivers/net/can/can327.c
> +++ b/drivers/net/can/can327.c
> @@ -1037,7 +1037,6 @@ static int can327_ldisc_open(struct tty_struct *tty)
>  
>  	/* Mark ldisc channel as alive */
>  	elm->tty = tty;
> -	tty->disc_data = elm;
>  
>  	/* Let 'er rip */
>  	err = register_candev(elm->dev);
> @@ -1046,6 +1045,8 @@ static int can327_ldisc_open(struct tty_struct *tty)
>  		return err;
>  	}
>  
> +	tty->disc_data = elm;
> +

[Severity: Medium]
Does this sequence result in a lost wakeup that can permanently stall
transmissions?

By calling register_candev() here, the network device is exposed to userspace
before can327_ldisc_open() returns. If userspace asynchronously brings the
interface up, it can trigger a transmission that eventually results in a call
to tty_wakeup() when the UART transmits.

However, the caller of can327_ldisc_open() (which is tty_set_ldisc() in
drivers/tty/tty_ldisc.c) holds tty->ldisc_sem for writing.

When tty_wakeup() runs, it tries to get the ldisc safely:

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);
        }
        ...
}

Because ldisc_sem is still held for writing by the open path, tty_ldisc_ref()
fails to acquire the read lock and returns NULL. This causes tty_wakeup()
to silently skip the write_wakeup callback. 

If the wakeup is lost, the transmission remains permanently stalled, leaving
the CAN interface bricked until it is manually brought down and back up. The
commit message notes that tty_ldisc_ref() serializes against the open path,
but relies on skipping the wakeup to prevent a use-after-free. Could we
defer registering the network device until after the ldisc semaphore is
released, or is there another way to prevent the transmission stall?

>  	netdev_info(elm->dev, "can327 on %s.\n", tty->name);
>  
>  	return 0;

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