Re: [PATCH net-next v12 1/8] hinic3: Fix drop skb when skb_checksum_help() fails

This is an AI-generated review of your patch. Larysa is right, this patch
looks like a fix. And there seems to be more bugs in the csum handling
according to AI.
---
> hinic3: Fix drop skb when skb_checksum_help() fails

[Severity: Low]
A couple of things about the changelog itself:

The subject reads a bit garbled ("Fix drop skb when ..."); something like
"hinic3: drop skb when skb_checksum_help() fails" would be easier to parse.
The body also carries a stray two-space indentation on the first line.

There is no Fixes: tag.  git blame on the pre-patch hunk

	skb_checksum_help(skb);
	return 0;

in hinic3_tx_csum() points at 17fcb3dc12bb ("hinic3: module initialization
and tx/rx logic"), so would it make sense to add:

Fixes: 17fcb3dc12bb ("hinic3: module initialization and tx/rx logic")

The body ("Changes hinic3_tx_csum()/hinic3_tx_offload() to propagate
skb_checksum_help() failures into a TX drop.") restates the diff but not the
observable defect.  skb_checksum_help() only sets skb->ip_summed =
CHECKSUM_NONE on its success path (out_set_summed) and jumps to out on every
error, so ip_summed stays CHECKSUM_PARTIAL.  The old code ignored that and
returned 0, so HINIC3_TX_OFFLOAD_CSUM was not set and the frame went out with
just the pseudo-header checksum in place.  Could the changelog describe that
user-visible effect?

> diff --git a/drivers/net/ethernet/huawei/hinic3/hinic3_tx.c b/drivers/net/ethernet/huawei/hinic3/hinic3_tx.c
> index 9306bf0020ca..53c065194136 100644
> --- a/drivers/net/ethernet/huawei/hinic3/hinic3_tx.c
> +++ b/drivers/net/ethernet/huawei/hinic3/hinic3_tx.c
> @@ -261,8 +261,7 @@ static int hinic3_tx_csum(struct hinic3_txq *txq, struct hinic3_sq_task *task,
>  		    ((struct udphdr *)skb_transport_header(skb))->dest !=
>  		    VXLAN_OFFLOAD_PORT_LE) {
>  			/* Unsupported tunnel packet, disable csum offload */
> -			skb_checksum_help(skb);
> -			return 0;
> +			return skb_checksum_help(skb);
>  		}
>  	}

[Severity: High]
This isn't a bug introduced by this patch, since the old code called
skb_checksum_help() in the same place and also kept transmitting, but this
patch reworks exactly this return path so it may be the right place to deal
with it.

On the success side skb_checksum_help() can linearize the skb:

net/core/dev.c:skb_checksum_help() {
	...
	if (skb_has_shared_frag(skb)) {
		ret = __skb_linearize(skb);
		if (ret)
			goto out;
	}
	...
}

__skb_linearize() -> __pskb_pull_tail(skb, skb->data_len) collapses all page
fragments, so skb_shinfo(skb)->nr_frags becomes 0 and 0 is returned, meaning
hinic3_tx_offload() continues on to transmit.

But hinic3_send_one_skb() sampled the fragment count before calling
hinic3_tx_offload():

	num_sge = skb_shinfo(skb)->nr_frags + 1;
	/* assume normal wqe format + 1 wqebb for task info */
	wqebb_cnt = num_sge + 1;
	...
	offload = hinic3_tx_offload(skb, &task, &queue_info, txq);

and then keeps using that stale num_sge for both the WQEBB reservation
(hinic3_set_wqe_combo()) and the descriptor count programmed into the
control word (hinic3_prepare_sq_ctrl() -> SQ_CTRL_SET(nr_descs,
BUFDESC_NUM)), while hinic3_tx_map_skb() only fills BD0 plus a loop over the
now-zero skb_shinfo(skb)->nr_frags.

Can the device then fetch buffer descriptors that were never written on this
pass, i.e. leftover addresses from previously completed and already
dma_unmap'ed WQEs at the same ring slots?  That would mean DMA reads of
stale addresses appended to the frame, or an IOMMU translation fault.

The path looks reachable for a non-GSO CHECKSUM_PARTIAL encapsulated skb
carrying SKBFL_SHARED_FRAG page fragments (MSG_ZEROCOPY / MSG_SPLICE_PAGES /
sendfile) over a tunnel that is not UDP dst port 4789, on a port that
advertises VXLAN offload.

Would recomputing num_sge and wqebb_cnt after hinic3_tx_offload() returns be
the right fix?

>  
> @@ -412,6 +411,10 @@ static u32 hinic3_tx_offload(struct sk_buff *skb, struct hinic3_sq_task *task,
>  		offload |= HINIC3_TX_OFFLOAD_TSO;
>  	} else {
>  		tso_cs_en = hinic3_tx_csum(txq, task, skb);
> +		if (tso_cs_en < 0) {
> +			offload = HINIC3_TX_OFFLOAD_INVALID;
> +			return offload;
> +		}

[Severity: Medium]
Should this new failure path bump a counter?

HINIC3_TX_OFFLOAD_INVALID sends hinic3_send_one_skb() to err_drop_pkt, which
does only:

err_drop_pkt:
	dev_kfree_skb_any(skb);
err_out:
	return NETDEV_TX_OK;

Since NETDEV_TX_OK is returned, the core does not account the drop either,
so the frame disappears with tx_dropped still reading zero in
'ip -s link'.

The driver already declares counters for this class of event in struct
hinic3_txq_stats (dropped, offload_cow_skb_err, unknown_tunnel_pkt), and
hinic3_get_stats64() already exports txq_stats.dropped as
rtnl_link_stats64.tx_dropped, but a grep shows those fields are only cleared
in hinic3_txq_clean_stats() and read back in hinic3_get_stats64() — nothing
ever increments them.

Would incrementing txq->txq_stats.dropped here under the existing
u64_stats_update_begin()/u64_stats_update_end() pair make the drop visible?
The other err_drop_pkt users (hinic3_tso() failure, PLDOFF overflow, DMA map
failure) share the same gap, so only this new site comes from this patch.

>  		if (tso_cs_en)
>  			offload |= HINIC3_TX_OFFLOAD_CSUM;
>  	}
-- 
pw-bot: cr