Re: [PATCH v4 6/7] net: wwan: t9xx: Add AT & MBIM WWAN ports

[Simon Horman]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://sashiko.dev
---
--- Patch [6]: [PATCH v4 6/7] net: wwan: t9xx: Add AT & MBIM WWAN ports ---
commit 1739a55610bf7a44ce24874819e01c07236d9442
Author: Jack Wu <jackbb_wu@xxxxxxxxxx>

net: wwan: t9xx: Add AT & MBIM WWAN ports

Add AT & MBIM ports to the port infrastructure.
The WWAN initialization method is responsible for creating the
corresponding ports using the WWAN framework infrastructure. The
implemented WWAN port operations are start, stop, tx, tx_blocking
and tx_poll.

> diff --git a/drivers/net/wwan/t9xx/mtk_port_io.c b/drivers/net/wwan/t9xx/mtk_port_io.c
> index e3a2de6d2f297..882254b74026f 100644
> --- a/drivers/net/wwan/t9xx/mtk_port_io.c
> +++ b/drivers/net/wwan/t9xx/mtk_port_io.c

[ ... ]

> @@ -234,6 +376,198 @@ static const struct port_ops port_internal_ops = {
>  	.recv = mtk_port_internal_recv,
>  };
>  
> +static int mtk_port_wwan_open(struct wwan_port *w_port)

[ ... ]

> +static int mtk_port_wwan_write(struct wwan_port *w_port, struct sk_buff *skb)
> +{
> +	struct mtk_port *port = wwan_port_get_drvdata(w_port);
> +	union user_buf user_buf;
> +	int ret;
> +
> +	if (unlikely(!skb->len)) {
> +		consume_skb(skb);
> +		return 0;
> +	}
> +
> +	port->info.flags &= ~PORT_F_BLOCKING;
> +	user_buf.kbuf = (void *)skb->data;
> +	ret = mtk_port_common_write(port, user_buf, skb->len, false);
> +	if (ret < 0)
> +		return ret;

[Severity: High]
If mtk_port_common_write() successfully sends the first few fragments but
fails on subsequent ones, returning a positive partial write count, does
this code silently drop the remaining data?

The condition ret < 0 is false for partial writes, so the SKB is consumed
and 0 is returned to the caller, falsely reporting complete success to the
WWAN core.

> +
> +	consume_skb(skb);
> +	return 0;
> +}
> +
> +static int mtk_port_wwan_write_blocking(struct wwan_port *w_port, struct sk_buff *skb)
> +{
> +	struct mtk_port *port = wwan_port_get_drvdata(w_port);
> +	union user_buf user_buf;
> +	int ret;
> +
> +	if (unlikely(!skb->len)) {
> +		consume_skb(skb);
> +		return 0;
> +	}
> +
> +	port->info.flags |= PORT_F_BLOCKING;
> +	user_buf.kbuf = (void *)skb->data;
> +	ret = mtk_port_common_write(port, user_buf, skb->len, false);
> +	if (ret < 0)
> +		return ret;

[Severity: High]
Can the same silent drop of partially written SKBs happen here?

> +
> +	consume_skb(skb);
> +	return 0;
> +}

[ ... ]

> +static void mtk_port_wwan_enable(struct mtk_port *port)
> +{
> +	struct mtk_port_mngr *port_mngr;
> +	int ret;
> +
> +	port_mngr = port->port_mngr;
> +
> +	if (test_bit(PORT_S_ENABLE, &port->status))
> +		return;
> +
> +	ret = mtk_port_ch_enable(port);
> +	if (ret && ret != -EBUSY)
> +		return;
> +
> +	port->w_priv.w_port = wwan_create_port(port_mngr->ctrl_blk->mdev->dev,
> +					       port->w_priv.w_type,
> +					       &wwan_ops, NULL, port);
> +	if (IS_ERR(port->w_priv.w_port)) {

[Severity: High]
Can a race condition occur here if wwan_create_port() fails and returns an
error pointer?

Once mtk_port_ch_enable() completes, hardware RX can start. If
wwan_create_port() fails, an ERR_PTR is stored in port->w_priv.w_port
without holding w_lock. Concurrently, the RX interrupt handler could call
mtk_port_wwan_recv():

	mutex_lock(&port->w_priv.w_lock);
	if (!port->w_priv.w_port) {
		mutex_unlock(&port->w_priv.w_lock);
		return -ENXIO;
	}
	wwan_port_rx(port->w_priv.w_port, skb);

Because an ERR_PTR is not NULL, mtk_port_wwan_recv() will pass it to
wwan_port_rx(), which will then dereference the error pointer.

> +		dev_warn(port_mngr->ctrl_blk->mdev->dev,
> +			 "Failed to create wwan port for (%s)\n", port->info.name);
> +		port->w_priv.w_port = NULL;
> +		mtk_port_ch_disable(port);
> +		return;
> +	}
> +
> +	set_bit(PORT_S_WR, &port->status);
> +	set_bit(PORT_S_ENABLE, &port->status);
> +}
> +