[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

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 1/2] can: slcan: publish tty->disc_data only after register_candev()

[Chaosheng Qu]

Hi sashiko,

Thanks -- the mechanism is real and the cover letter is wrong about it. Let me
confirm what you describe, then be precise about what this series does and does
not fix.

The lost callback is real. `tty_wakeup()` is the only caller of
`ld->ops->write_wakeup`, and it takes the ldisc reference with
`tty_ldisc_ref()`, which is a `ldsem_down_read_trylock()`. While `slcan_open()`
runs we hold `ldisc_sem` for writing (`tty_set_ldisc()` takes it around
`tty_ldisc_open()`), so the trylock fails, `tty_ldisc_ref()` returns NULL, and
the callback is silently skipped:

```c
	if (test_bit(TTY_DO_WRITE_WAKEUP, &tty->flags)) {
		ld = tty_ldisc_ref(tty);
		if (ld) {
			if (ld->ops->write_wakeup)
				ld->ops->write_wakeup(tty);
			tty_ldisc_deref(ld);
		}
	}
	wake_up_interruptible_poll(&tty->write_wait, EPOLLOUT);
```

So a UART `write_wakeup()` arriving inside that window loses
`slcan_write_wakeup()` -> `schedule_work(&sl->tx_work)` never happens ->
`slcan_transmit()` never runs -> `netif_wake_queue()` is never reached and the
queue stays stopped. The only other thing `tty_wakeup()` does is wake the
`write_wait` pollers, so nothing else notices. `slcan_transmit_cmd()` has the
same hole: it waits on `xcmd_wait` for the `SLF_XCMD` completion that only
`slcan_transmit()` can clear, so a wakeup lost there ends in `-ETIMEDOUT` after
one second.

Two caveats, in the interest of accuracy rather than defence:

1. This window is not introduced by the series. Pre-series the pointer is
   stored even earlier, just above `register_candev()`, so the same race exists
   in the tree today. What the patch fixes is the other half: the dangling
   `tty->disc_data` left behind when `register_candev()` fails, which both
   `slcan_write_wakeup()` and `slcan_netdev_xmit()` dereference without a NULL
   check.

2. Deferring `register_candev()` until after the semaphore is released is not
   expressible from `ldisc->open()`. `tty_set_ldisc()` holds `ldisc_sem` across
   `tty_ldisc_open()` and only unlocks it at the `out:` label after `.open()`
   has returned, and `tty_ldisc_setup()` opens the primary tty with `ldisc_sem`
   already held. The only way to publish after the unlock is to move
   `register_candev()` onto a work item queued at the end of `.open()` -- which
   also means `.open()` can no longer report a registration failure:
   `slcan_close()` would have to handle a not-yet-registered device, and the
   ldisc change would succeed before we know whether the netdev exists.

The failure is also bounded and recoverable: the window lasts only as long as
`slcan_open()`, and afterwards a bring-down/bring-up of the interface (or a tty
hangup) goes through `slcan_netdev_close()`/`slcan_open()`, which clear
`TTY_DO_WRITE_WAKEUP`, `flush_work(&sl->tx_work)` and start the queue again. It
needs the UART wakeup to land exactly inside the tail of `.open()`, so this is a
narrow race, not a stall from normal use.

For the next revision I will drop the "the stale pointer is not observable
today" sentence from the cover letter -- you are right that it is wrong for the
`write_wakeup` path -- and state the above instead. If you think moving the
registration to a work item is the right trade-off, I would rather do it as a
separate patch on top: folding an error-handling change into a two-line
ordering fix is not something I want to hide in this series.

Thanks,

Chaosheng

[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 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

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

[Chaosheng Qu]

Hi sashiko,

The mechanism is the same one I confirmed in the reply to 1/2, but I do not
think the outcome is: can327 does not restart its transmit queue from
`write_wakeup`.

`can327_ldisc_tx_worker()` only drains `elm->txleft` and clears
`TTY_DO_WRITE_WAKEUP` once the buffer is empty -- there is no
`netif_wake_queue()` in it. The queue is restarted from
`can327_handle_prompt()`, after the `ATMA` read command has been answered and
the controller is back in the default state, and again from the
`CAN327_TX_DO_CAN_DATA` branch of `can327_parse_rxbuf()`. Both run off the
receive path, and receive is not blocked by `ldisc_sem`: `flush_to_ldisc()`
takes no ldisc reference at all, it stops at the `buf->priority` check while the
ldisc is being changed, and `tty_set_ldisc()` restarts it with
`tty_buffer_restart_work()` after the unlock. So a wakeup lost inside
`can327_ldisc_open()` delays the queue restart until the next prompt is parsed,
but does not strand it.

What the patch is actually for is the failure path: when `register_candev()`
fails, `can327_ldisc_open()` returns without ever setting `tty->disc_data`, so
the `can327_ldisc_tx_wakeup()` that dereferences it unchecked can never be
reached with a stale pointer. Before the patch the pointer was published above
`register_candev()` and `free_candev()` dropped it out from under the tty.

On the suggestion to defer the registration: as in the 1/2 reply, `.open()`
cannot release `ldisc_sem` -- `tty_set_ldisc()` holds it across
`tty_ldisc_open()` and unlocks only after `.open()` returns -- so "after the
semaphore is released" can only be reached via a work item, at the cost of
`.open()` no longer being able to fail on registration.

Thanks,

Chaosheng

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

[Max Staudt]

Hi Chaosheng,

Please excuse my late reply, and thank you very much for your patience.


As for your patch, thank you for suggesting the improvement. I think 
it's a good idea to leave tty->disc_data in a sane, or saner, state upon 
failure.

However, I have to NAK your current patches:

By moving the line that sets tty->disc_data after register_candev(), you 
create a race against .ndo_open() and friends. Right now the implicit 
contract within the code is that the ldisc is set up first, then the 
netdev. This is not interleaved. But your patch interleaves these, thus 
requiring a review of SMT safety between the ldisc part and the netdev 
part, and potentially introducing new locking within the drivers.

That's a bit much for a cosmetic issue when debugging.

To make my point a bit clearer, quoting from your 0/2 top message:

 > The series removes a trap for the next modification of these 
functions rather than fixing a bug that can be hit now.

Racing against .ndo_*() may not be a problem today (I haven't checked 
though), but it'd be counterintuitive for the next one to touch this 
code. We'd be trading a "trap" for another one, like you say.


By the way, with such a patch that crosses a SMT boundary, I'd be 
helpful for the patch message to include a discussion of why this is 
okay to do. It makes review easier, and shows that you've thought it 
through :)


I've been scratching my head over this patch for a while. It's a small, 
well meant patch, where I see the good intention of not adding more 
code, only moving it. And yet, it actually complicates the code in a way 
I haven't seen a patch do before. Something felt off, and I couldn't 
quite put my finger on it.

Then it dawned on me, and I'd appreciate if you answered purely for my 
own curiosity:
Is this patch something that an LLM suggested?

After all, LLMs are trained to sound convincing, and maybe it led you, 
and then me, down a very neat looking rabbit hole :)


Okay, so here is an alternative suggestion, which I think can make us 
both happy. Instead of deferring the "disc_data = ..." line, how about 
simply setting disc_data = NULL upon failure? That keeps the SMT 
separation as-is, you get your nice NULL value, and everyone was happy 
ever after?


Max

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

[Chaosheng Qu]

Hi Max,

Thank you for the thorough review, and for taking the time to work out why the
patch felt wrong. You are right, and I am dropping this approach.

The point I missed is the first one you made: "ldisc first, then netdev" is an
implicit contract in these two drivers, and my patch interleaves it. Even if
nothing races today, the next person to touch these functions would have to
reason about SMT safety that was previously not theirs to reason about. That is
swapping one trap for another, which is exactly what I claimed to be removing.

So v3 will use your alternative instead of mine, and nothing else:

	err = register_candev(dev);
	if (err) {
+		tty->disc_data = NULL;
		free_candev(dev);
		pr_err("can't register candev\n");
		return err;
	}

Same in can327. The ordering stays as it is, so the implicit contract is
untouched, and the failure path no longer leaves tty->disc_data pointing at
memory that free_candev() has just released. The commit message will say that
plainly, and will state why no barrier or extra locking is needed: the pointer
is read exactly where it was read before, and the only write on this path
happens before the interface can be brought up.

The sashiko report on 1/2 (a lost tty_wakeup while ldisc_sem is held) also goes
away with your version, since nothing is reordered any more.

On your question: yes. I use an AI assistant when I write patches, and the
disclosure line required by Documentation/process/coding-assistants.rst will be
on v3. Your instinct about why this one looked too neat is right: the assistant
was good at exactly one thing here, finding the smallest diff that removes the
dangling pointer, and moving the assignment is a smaller diff than clearing it.
That is the move that quietly crossed the SMT boundary. Knowing when the
smaller diff is the wrong one is still my job, and I got it wrong.

v3 in a day or two, after I have re-read both drivers end to end rather than
trusting the diff.

Thanks,
Chaosheng