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

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

[Chaosheng Qu]

slcan_open() and can327_ldisc_open() publish the private structure to the
tty layer through tty->disc_data and register the net_device afterwards.
If register_candev() fails, free_candev() releases the net_device and the
private area with it, while tty->disc_data still points at the freed
memory. register_candev() fails with -ENOMEM from register_netdevice()
when the device name node or the kobject cannot be allocated.

Both patches publish the pointer only after the device is registered, so
the error path has nothing to unwind.

The slcan side is a regression. From the driver's addition in 2010 until
the 2022 rewrite the error path cleared the pointer in err_free_chan;
commit cfcb4465e992 ("can: slcan: remove legacy infrastructure") folded
that label away and dropped the tty->disc_data = NULL with it. can327 was
written later against this tree and never had the cleanup.

The stale pointer is not observable today. tty_set_termios_ldisc() clears
tty->disc_data before .open() runs and again when a failed ldisc change
falls back, and the ldisc entry points are serialised against .open() by
tty_ldisc_ref() (async) and tty_ldisc_ref_wait() (syscall). The series
removes a trap for the next modification of these functions rather than
fixing a bug that can be hit now.

Both patches apply with git am to mkl/linux-can-next testing and build
without new warnings on x86_64 with CAN_SLCAN and CAN_CAN327 enabled;
checkpatch.pl --strict is clean. The success path was tested by attaching
N_SLCAN and N_CAN327 to a pty under QEMU: the ldisc opens, creates the
net_device and SIOCGIFNAME returns its name. The register_candev()
failure path cannot be forced from userspace and is only compile tested.

Assisted-by: LLM

Chaosheng Qu (2):
  can: slcan: publish tty->disc_data only after register_candev()
  can: can327: publish tty->disc_data only after register_candev()

 drivers/net/can/can327.c           | 3 ++-
 drivers/net/can/slcan/slcan-core.c | 3 ++-
 2 files changed, 4 insertions(+), 2 deletions(-)

-- 
2.43.0

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

[Chaosheng Qu]

slcan_open() publishes the private structure to the tty layer before
registering the net_device:

	tty->disc_data = sl;

	err = register_candev(dev);
	if (err) {
		free_candev(dev);
		pr_err("can't register candev\n");
		return err;
	}

register_candev() fails with -ENOMEM from register_netdevice() when the
device name node or the kobject cannot be allocated. The error path then
frees the net_device and the slcan structure in its private area, while
tty->disc_data still points at the freed memory.

This is a regression from the 2022 rewrite. From the driver's addition in
2010 until then the error path cleared the pointer:

	err_free_chan:
		sl->tty = NULL;
		tty->disc_data = NULL;
		clear_bit(SLF_INUSE, &sl->flags);

Commit cfcb4465e992 ("can: slcan: remove legacy infrastructure") folded
that label away and dropped the tty->disc_data = NULL with it.

Publish the pointer only after the device is registered, so the error
path has nothing to unwind. ppp_async and nci_uart assign it the same
way; nci_uart was changed to that in commit fc27ab48904c ("NFC: nci:
uart: Set tty->disc_data only in success path").

The stale pointer is not observable today: tty_set_termios_ldisc() clears
tty->disc_data before .open() runs and again when a failed ldisc change
falls back, and the ldisc entry points are serialised against .open() by
tty_ldisc_ref() (async) and tty_ldisc_ref_wait() (syscall). The change
removes a trap for the next modification of these functions.

The success path was tested by attaching N_SLCAN to a pty under QEMU: the
ldisc opens, creates the net_device and SIOCGIFNAME returns its name. The
register_candev() failure path cannot be forced from userspace and is
only compile tested.

Assisted-by: LLM
Signed-off-by: Chaosheng Qu <chaosheng.qu.linux@gmail.com>
---
 drivers/net/can/slcan/slcan-core.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/can/slcan/slcan-core.c b/drivers/net/can/slcan/slcan-core.c
index c9414faafe66..fc6ecf110aab 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;
+
 	netdev_info(dev, "slcan on %s.\n", tty->name);
 	/* TTY layer expects 0 on success */
 	return 0;
-- 
2.43.0

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

[Chaosheng Qu]

can327_ldisc_open() publishes the private structure to the tty layer
before registering the net_device:

	tty->disc_data = elm;

	err = register_candev(elm->dev);
	if (err) {
		free_candev(elm->dev);
		return err;
	}

register_candev() fails with -ENOMEM from register_netdevice() when the
device name node or the kobject cannot be allocated. The error path then
frees the net_device and the can327 structure in its private area, while
tty->disc_data still points at the freed memory.

Publish the pointer only after the device is registered, so the error
path has nothing to unwind. slcan does the same after its own patch, as
do ppp_async and nci_uart.

The stale pointer is not observable today: tty_set_termios_ldisc() clears
tty->disc_data before .open() runs and again when a failed ldisc change
falls back, and the ldisc entry points are serialised against .open() by
tty_ldisc_ref() (async) and tty_ldisc_ref_wait() (syscall). The change
removes a trap for the next modification of these functions.

The success path was tested by attaching N_CAN327 to a pty under QEMU:
the ldisc opens, creates the net_device and SIOCGIFNAME returns its name.
The register_candev() failure path cannot be forced from userspace and is
only compile tested.

Assisted-by: LLM
Signed-off-by: Chaosheng Qu <chaosheng.qu.linux@gmail.com>
---
 drivers/net/can/can327.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/can/can327.c b/drivers/net/can/can327.c
index 90f5e35f3c8f..d40dadbb5733 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;
+
 	netdev_info(elm->dev, "can327 on %s.\n", tty->name);
 
 	return 0;
-- 
2.43.0

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

[Max]

Hi Chaosheng,

Thank you for your patch.

I won't have time to review this properly until the end of the month.

If someone else feels comfortable reviewing this, please go ahead!


Thanks,

Max

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

[sashiko-bot]

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

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

[sashiko-bot]

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