Re: [PATCH net-next v9 1/2] net: wwan: core: propagate modem control signals to port drivers

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 2 · Low: 3

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] `wwan_port_fops_at_ioctl()` now does `ACQUIRE(mutex_intr,
  lock)(&port->ops_lock)` before its `switch`, so every ioctl on an…
- [Medium] `wwan_port_fops_at_ioctl()` now holds `port->ops_lock` for the
  whole function, via a function-scope ACQUIRE guard, across every user-…
- [Low] Not a valid finding (baseline decision: false).
- [Low] The comments say the wwan open/close DTR handling works 'as a TTY
  does', but the code ignores the baud field of the termios state it…
- [Low] The new `->dtr_rts` kernel-doc in include/linux/wwan.h says the
  op is 'Called on first open, on last close if HUPCL is set, on port…

--

net: wwan: core: propagate modem control signals to port drivers

This adds an optional ->dtr_rts() operation to struct wwan_port_ops. The
wwan core calls it on first open, on last close when HUPCL is set, on
removal of an open port, and when TIOCMSET/TIOCMBIS/TIOCMBIC change DTR or
RTS on an AT port. The separate data_lock is removed, and the AT/QCDM
termios and TIOCM state is now protected by ops_lock, which the ioctl
handler takes interruptibly.

> diff --git a/drivers/net/wwan/wwan_core.c b/drivers/net/wwan/wwan_core.c
> index ffbcf11e4e681..27ae6583d3e08 100644
> --- a/drivers/net/wwan/wwan_core.c
> +++ b/drivers/net/wwan/wwan_core.c

[ ... ]

> @@ -653,7 +654,10 @@ struct wwan_port *wwan_create_port(struct device *parent,
>  	mutex_init(&port->ops_lock);
>  	skb_queue_head_init(&port->rxq);
>  	init_waitqueue_head(&port->waitqueue);
> -	mutex_init(&port->data_lock);
> +
> +	/* AT ports hang up on last close by default, as a TTY does */
> +	if (type == WWAN_PORT_AT)
> +		port->at_data.termios.c_cflag = HUPCL;
>  
>  	port->dev.parent = &wwandev->dev;
>  	port->dev.type = &wwan_port_dev_type;
> @@ -679,12 +683,44 @@ struct wwan_port *wwan_create_port(struct device *parent,
>  }
>  EXPORT_SYMBOL_GPL(wwan_create_port);
>  
> +/* Raise DTR/RTS on first open of an AT port, as a TTY does. Called with
> + * ops_lock held.
> + */
> +static void wwan_port_raise_dtr_rts(struct wwan_port *port)
> +{
> +	lockdep_assert_held(&port->ops_lock);
> +
> +	if (port->type != WWAN_PORT_AT || !port->ops->dtr_rts)
> +		return;
> +
> +	port->at_data.mdmbits |= TIOCM_DTR | TIOCM_RTS;
> +	port->ops->dtr_rts(port, port->at_data.mdmbits);
> +}

[Severity: Low]
The comments here and in wwan_create_port() say this behaves as a TTY
does. Is the baud field of the termios taken into account anywhere?

wwan_create_port() sets c_cflag to just HUPCL, so TCGETS reports B0 to
userspace. Even so, wwan_port_raise_dtr_rts() asserts DTR/RTS on every
first open. On a TTY, the lines are only raised when the baud rate is
non-zero:

drivers/tty/tty_port.c:tty_port_block_til_ready() {
    ...
		if (C_BAUD(tty))
			tty_port_raise_dtr_rts(port);
    ...
}

The TTY default is also tty_std_termios (B38400 | CS8 | CREAD | HUPCL),
not bare HUPCL.

In the same way, a TCSETS with B0 only stores the termios here and never
calls ->dtr_rts(). A TTY would drop DTR in that case. Examples are
stty -F /dev/wwan0at0 0, or the POSIX hang-up of cfsetospeed(B0) followed
by tcsetattr().

Separately, the kernel-doc says "Called on first open", but the call only
happens when ->start() succeeds.

Should the comments make a weaker claim? Alternatively, could the default
termios use a non-zero baud rate so that TCGETS matches what the driver
was told?

> +
> +/* Drop DTR/RTS on an AT port. Called with ops_lock held. On last close
> + * the lines are only dropped if HUPCL is set, as for a TTY, on port
> + * removal they are always dropped.
> + */
> +static void wwan_port_drop_dtr_rts(struct wwan_port *port, bool hupcl_only)
> +{
> +	lockdep_assert_held(&port->ops_lock);
> +
> +	if (port->type != WWAN_PORT_AT || !port->ops->dtr_rts)
> +		return;
> +	if (hupcl_only && !(port->at_data.termios.c_cflag & HUPCL))
> +		return;
> +
> +	port->at_data.mdmbits &= ~(TIOCM_DTR | TIOCM_RTS);
> +	port->ops->dtr_rts(port, port->at_data.mdmbits);
> +}
> +
>  void wwan_remove_port(struct wwan_port *port)
>  {
>  	struct wwan_device *wwandev = to_wwan_dev(port->dev.parent);
>  
>  	mutex_lock(&port->ops_lock);
>  	if (port->start_count) {
> +		wwan_port_drop_dtr_rts(port, false);
>  		port->ops->stop(port);
>  		port->start_count = 0;
>  	}

[Severity: Low]
The comment above says "on port removal they are always dropped". The
->dtr_rts kernel-doc in include/linux/wwan.h also lists "on port removal"
without any condition. Is that accurate, given that the drop sits inside
the if (port->start_count) block?

The commit message describes clearing HUPCL as the way to keep DTR
asserted across close. If userspace does that and then closes the port,
wwan_port_op_stop() returns early in wwan_port_drop_dtr_rts(). A later
wwan_remove_port() then sees start_count == 0, so the driver never gets a
->dtr_rts() call on removal.

The behaviour itself matches a TTY. Could the comment and the kernel-doc
say that the lines are only dropped when a port is removed while still
open?

[ ... ]

> @@ -979,9 +1020,15 @@ static __poll_t wwan_port_fops_poll(struct file *filp, poll_table *wait)
>  static long wwan_port_fops_at_ioctl(struct wwan_port *port, unsigned int cmd,
>  				    unsigned long arg)
>  {
> -	int ret = 0;
> +	int ret;
>  
> -	mutex_lock(&port->data_lock);
> +	/* ops_lock serialises the termios/TIOCM state with port ops and
> +	 * removal. A blocking write holds it too, so wait interruptibly.
> +	 */
> +	ACQUIRE(mutex_intr, lock)(&port->ops_lock);
> +	ret = ACQUIRE_ERR(mutex_intr, &lock);
> +	if (ret)
> +		return ret;

[Severity: Medium]
The lock is now taken before the switch, so every ioctl on an AT/QCDM
port waits on ops_lock. That includes TCGETS, TCFLSH,
TIOCEXCL/TIOCNXCL/TIOCGEXCL, and commands that end up in the default
-ENOIOCTLCMD case.

FIONREAD is one of those, because wwan_port_fops_ioctl() tries the AT
handler first:

drivers/net/wwan/wwan_core.c:wwan_port_fops_ioctl() {
    ...
		res = wwan_port_fops_at_ioctl(port, cmd, arg);
		if (res != -ENOIOCTLCMD)
			return res;
	}

	switch (cmd) {
	case TIOCINQ: {	/* aka SIOCINQ aka FIONREAD */
    ...
}

wwan_port_op_tx() holds ops_lock across ->tx_blocking(). For
rpmsg_wwan_ctrl that is a blocking rpmsg_send(). Suppose thread A is in a
blocking write():

wwan_port_fops_write()
  wwan_port_op_tx()
    mutex_lock(&port->ops_lock)
    port->ops->tx_blocking()
      rpmsg_send()

Thread B's ioctl(FIONREAD) or tcgetattr() now waits behind it. A signal
during that wait makes mutex_lock_interruptible() return -EINTR. That
value is not -ENOIOCTLCMD, so it goes straight back to userspace, and
SA_RESTART won't restart the call.

Before this patch these paths only took data_lock, which the write path
never held.

Could ops_lock be taken only for the commands that touch the
termios/TIOCM state or call ->dtr_rts()?

[ ... ]

> @@ -1024,18 +1082,24 @@ static long wwan_port_fops_at_ioctl(struct wwan_port *port, unsigned int cmd,
>  	case TIOCMSET:
>  	case TIOCMBIC:
>  	case TIOCMBIS: {
> -		int mdmbits;
> +		int mdmbits, old = port->at_data.mdmbits;
>  
>  		if (copy_from_user(&mdmbits, (int __user *)arg, sizeof(int))) {

[Severity: Medium]
The ACQUIRE guard holds ops_lock for the whole function, so several user
memory accesses now run with ops_lock held:

  - this copy_from_user()
  - the copy_to_user() calls for TCGETS/TCGETS2
  - the copy_from_user() calls for TCSETS*/TCSETS2*
  - the put_user() for TIOCMGET

Can userspace hold ops_lock indefinitely this way?

Suppose arg points into memory whose page fault never completes, for
example an mmap of a file on a FUSE mount whose daemon never answers. The
thread then sleeps in the fault handler with ops_lock held. The
interruptible acquisition doesn't help once the lock is owned.

wwan_remove_port(), wwan_port_op_start(), wwan_port_op_stop(),
wwan_port_op_tx() and wwan_port_fops_poll() all take ops_lock with plain
mutex_lock(). Device removal, for example via mhi_wwan_ctrl_remove() or a
cdc-wdm disconnect, would then hang in D state:

ioctl(fd, TIOCMSET, ptr_into_fuse_mapping)
  wwan_port_fops_at_ioctl()
    ACQUIRE(mutex_intr, lock)(&port->ops_lock)
    copy_from_user()   <- waits on FUSE

wwan_remove_port()
  mutex_lock(&port->ops_lock)   <- blocks

Any other process that opens, closes or polls the port would block in the
same way.

Before this patch the ioctl user copies ran under the ioctl-private
data_lock. wwan_port_fops_write() copies user data before calling
wwan_port_op_tx(), so nothing touched user memory under ops_lock.

Would it work to copy the argument in before taking the lock, and copy
results out after dropping it, as the TTY core does?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261012172219.3668624-1-peter.hunt%40opengear.com