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

[Chaosheng Qu <chaosheng.qu.linux@gmail.com>]

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