Re: [PATCH net v2] net: wwan: t7xx: check skb_clone in control TX

[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
---
net: wwan: t7xx: check skb_clone in control TX

This patch checks the return value of skb_clone() in t7xx_port_ctrl_tx()
and returns -ENOMEM if it fails, preventing a NULL pointer dereference.

> diff --git a/drivers/net/wwan/t7xx/t7xx_port_wwan.c b/drivers/net/wwan/t7xx/t7xx_port_wwan.c
> --- a/drivers/net/wwan/t7xx/t7xx_port_wwan.c
> +++ b/drivers/net/wwan/t7xx/t7xx_port_wwan.c
> @@ -106,6 +106,8 @@ static int t7xx_port_ctrl_tx(struct t7xx_port *port, struct sk_buff *skb)
>  
>  	while (cur) {
>  		cloned = skb_clone(cur, GFP_KERNEL);
> +		if (!cloned)
> +			return -ENOMEM;

[Severity: High]
If we return -ENOMEM directly here when skb_clone() fails on a subsequent
fragment, does this discard the count of successfully transmitted bytes
accumulated in cnt?

If t7xx_port_send_skb() previously succeeded for earlier fragments, returning
-ENOMEM to wwan_port_fops_write() will leave userspace unaware that partial
data was already consumed. Will userspace then retry writing the entire
buffer and duplicate the already-sent fragments on the WWAN stream?

>  		cloned->len = skb_headlen(cur);

Simon says: I have trimmed the remaining issues flagged in the AI-generated
            review as I believe that they are pre-existing and do not
	    warrant being considered in the context of this patch.

	    However, you may want to look over them on sashiko.dev
	    in the context of possible follow-up separate to this patch.