[PATCH net] hinic3: Fix SKB linearization mismatch and silent TX drops

[PATCH net] hinic3: Fix SKB linearization mismatch and silent TX drops

[Fan Gong]

Previously, hinic3_send_one_skb() cached the SKB fragment count before
calling hinic3_tx_offload(). If hinic3_tx_csum() falls back to
skb_checksum_help() for unsupported tunnel packets, the SKB may be
linearized. Continuing to build the TX descriptor with the stale
fragment count leads to a descriptor mismatch, which can trigge
out-of-bounds DMA reads or IOMMU faults.

Furthermore, the old code ignored the return value of skb_checksum_help(),
transmitting corrupted packets with incomplete checksums upon failure. It
also failed to increment drop statistics across various TX error paths,
causing packets to be dropped silently without notifying the user.

Fix this by:
1. Moving the hinic3_tx_offload() call before calculating 'num_sge' to
   ensure the correct fragment count is used if the SKB is linearized.
2. Propagating skb_checksum_help() errors and returning
   HINIC3_TX_OFFLOAD_INVALID to properly drop the skb.
3. Adding missing statistics increments (dropped, map_frag_err,
   unknown_tunnel_pkt, skb_pad_err) across the TX error paths so these
   events are correctly reflected in interface statistics.

Fixes: 17fcb3dc12bb ("hinic3: module initialization and tx/rx logic")
Co-developed-by: Teng Peisen <tengpeisen@huawei.com>
Signed-off-by: Teng Peisen <tengpeisen@huawei.com>
Co-developed-by: Wu Di <wudi234@huawei.com>
Signed-off-by: Wu Di <wudi234@huawei.com>
Signed-off-by: Fan Gong <gongfan1@huawei.com>
---
 .../net/ethernet/huawei/hinic3/hinic3_tx.c    | 31 ++++++++++++++++---
 1 file changed, 26 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/huawei/hinic3/hinic3_tx.c b/drivers/net/ethernet/huawei/hinic3/hinic3_tx.c
index 9306bf0020ca..45effdddb434 100644
--- a/drivers/net/ethernet/huawei/hinic3/hinic3_tx.c
+++ b/drivers/net/ethernet/huawei/hinic3/hinic3_tx.c
@@ -97,8 +97,12 @@ static int hinic3_tx_map_skb(struct net_device *netdev, struct sk_buff *skb,

 	dma_info[0].dma = dma_map_single(&pdev->dev, skb->data,
 					 skb_headlen(skb), DMA_TO_DEVICE);
-	if (dma_mapping_error(&pdev->dev, dma_info[0].dma))
+	if (dma_mapping_error(&pdev->dev, dma_info[0].dma)) {
+		u64_stats_update_begin(&txq->txq_stats.syncp);
+		txq->txq_stats.map_frag_err++;
+		u64_stats_update_end(&txq->txq_stats.syncp);
 		return -EFAULT;
+	}

 	dma_info[0].len = skb_headlen(skb);

@@ -117,6 +121,9 @@ static int hinic3_tx_map_skb(struct net_device *netdev, struct sk_buff *skb,
 						     skb_frag_size(frag),
 						     DMA_TO_DEVICE);
 		if (dma_mapping_error(&pdev->dev, dma_info[idx].dma)) {
+			u64_stats_update_begin(&txq->txq_stats.syncp);
+			txq->txq_stats.map_frag_err++;
+			u64_stats_update_end(&txq->txq_stats.syncp);
 			err = -EFAULT;
 			goto err_unmap_page;
 		}
@@ -260,9 +267,11 @@ static int hinic3_tx_csum(struct hinic3_txq *txq, struct hinic3_sq_task *task,
 		if (l4_proto != IPPROTO_UDP ||
 		    ((struct udphdr *)skb_transport_header(skb))->dest !=
 		    VXLAN_OFFLOAD_PORT_LE) {
+			u64_stats_update_begin(&txq->txq_stats.syncp);
+			txq->txq_stats.unknown_tunnel_pkt++;
+			u64_stats_update_end(&txq->txq_stats.syncp);
 			/* Unsupported tunnel packet, disable csum offload */
-			skb_checksum_help(skb);
-			return 0;
+			return skb_checksum_help(skb);
 		}
 	}

@@ -412,6 +421,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;
+		}
 		if (tso_cs_en)
 			offload |= HINIC3_TX_OFFLOAD_CSUM;
 	}
@@ -539,12 +552,17 @@ static netdev_tx_t hinic3_send_one_skb(struct sk_buff *skb,
 	int err;

 	if (unlikely(skb->len < MIN_SKB_LEN)) {
-		if (skb_pad(skb, MIN_SKB_LEN - skb->len))
+		if (skb_pad(skb, MIN_SKB_LEN - skb->len)) {
+			u64_stats_update_begin(&txq->txq_stats.syncp);
+			txq->txq_stats.skb_pad_err++;
+			u64_stats_update_end(&txq->txq_stats.syncp);
 			goto err_out;
+		}

 		skb->len = MIN_SKB_LEN;
 	}

+	offload = hinic3_tx_offload(skb, &task, &queue_info, txq);
 	num_sge = skb_shinfo(skb)->nr_frags + 1;
 	/* assume normal wqe format + 1 wqebb for task info */
 	wqebb_cnt = num_sge + 1;
@@ -560,7 +578,6 @@ static netdev_tx_t hinic3_send_one_skb(struct sk_buff *skb,
 		return NETDEV_TX_BUSY;
 	}

-	offload = hinic3_tx_offload(skb, &task, &queue_info, txq);
 	if (unlikely(offload == HINIC3_TX_OFFLOAD_INVALID)) {
 		goto err_drop_pkt;
 	} else if (!offload) {
@@ -604,6 +621,10 @@ static netdev_tx_t hinic3_send_one_skb(struct sk_buff *skb,
 err_drop_pkt:
 	dev_kfree_skb_any(skb);
 err_out:
+	u64_stats_update_begin(&txq->txq_stats.syncp);
+	txq->txq_stats.dropped++;
+	u64_stats_update_end(&txq->txq_stats.syncp);
+
 	return NETDEV_TX_OK;
 }


base-commit: 2195424c3da2ef1829a63b807e3a900a90e57d85
--
2.54.0

Re: [PATCH net] hinic3: Fix SKB linearization mismatch and silent TX drops

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email says "Let's add the counters in a separate patch targetting
net-next."
---
hinic3: Fix SKB linearization mismatch and silent TX drops

This moves the hinic3_tx_offload() call in hinic3_send_one_skb() ahead of
the num_sge computation so a linearizing skb_checksum_help() cannot leave a
stale fragment count behind, and propagates skb_checksum_help() errors as
HINIC3_TX_OFFLOAD_INVALID.  It also adds dropped, map_frag_err,
unknown_tunnel_pkt and skb_pad_err accounting to several TX error paths.

No Link: tags in the commit header.

> diff --git a/drivers/net/ethernet/huawei/hinic3/hinic3_tx.c b/drivers/net/ethernet/huawei/hinic3/hinic3_tx.c
> index 9306bf0020caf..45effdddb4341 100644
> --- a/drivers/net/ethernet/huawei/hinic3/hinic3_tx.c
> +++ b/drivers/net/ethernet/huawei/hinic3/hinic3_tx.c
> @@ -97,8 +97,12 @@ static int hinic3_tx_map_skb(struct net_device *netdev, struct sk_buff *skb,
>  
>  	dma_info[0].dma = dma_map_single(&pdev->dev, skb->data,
>  					 skb_headlen(skb), DMA_TO_DEVICE);
> -	if (dma_mapping_error(&pdev->dev, dma_info[0].dma))
> +	if (dma_mapping_error(&pdev->dev, dma_info[0].dma)) {
> +		u64_stats_update_begin(&txq->txq_stats.syncp);
> +		txq->txq_stats.map_frag_err++;
> +		u64_stats_update_end(&txq->txq_stats.syncp);
>  		return -EFAULT;
> +	}

[Severity: Low]
Can these three new counters actually be observed by a user?

Of the four counters added here, only dropped has a consumer.  The single
reader of struct hinic3_txq_stats is hinic3_get_stats64() in
hinic3_netdev_ops.c, which copies only bytes, packets and dropped into
struct rtnl_link_stats64:

	do {
		start = u64_stats_fetch_begin(&txq_stats->syncp);
		bytes += txq_stats->bytes;
		packets += txq_stats->packets;
		dropped += txq_stats->dropped;
	} while (u64_stats_fetch_retry(&txq_stats->syncp, start));

and hinic3_ethtool_ops in hinic3_ethtool.c implements no
get_ethtool_stats/get_sset_count/get_strings callbacks, so there is no
ethtool -S path either.

That makes map_frag_err, unknown_tunnel_pkt and skb_pad_err dead stores.
The changelog says these events are "correctly reflected in interface
statistics" - does that hold for anything other than dropped?

>  
>  	dma_info[0].len = skb_headlen(skb);
>  
> @@ -117,6 +121,9 @@ static int hinic3_tx_map_skb(struct net_device *netdev, struct sk_buff *skb,
>  						     skb_frag_size(frag),
>  						     DMA_TO_DEVICE);
>  		if (dma_mapping_error(&pdev->dev, dma_info[idx].dma)) {
> +			u64_stats_update_begin(&txq->txq_stats.syncp);
> +			txq->txq_stats.map_frag_err++;
> +			u64_stats_update_end(&txq->txq_stats.syncp);
>  			err = -EFAULT;
>  			goto err_unmap_page;
>  		}

[Severity: High]
This isn't a bug introduced by this patch, but since it instruments exactly
this failure path it may be worth fixing in the same series: can the mapping
failure return here leave a dangling txq->tx_info[pi].skb?

In hinic3_send_one_skb() the ring slot is published before mapping:

	tx_info = &txq->tx_info[pi];
	tx_info->skb = skb;
	tx_info->wqebb_cnt = wqebb_cnt;

	err = hinic3_tx_map_skb(netdev, skb, txq, tx_info, &wqe_combo);
	if (err) {
		/* Rollback work queue to reclaim the wqebb we did not use */
		txq->sq->wq.prod_idx = saved_wq_prod_idx;
		txq->sq->owner = saved_sq_owner;
		goto err_drop_pkt;
	}

err_drop_pkt calls dev_kfree_skb_any(skb) but tx_info->skb is not reset, and
because prod_idx is rolled back, hinic3_tx_poll() - the only place that
clears tx_info->skb during normal operation - never visits that slot.

If the queue is torn down before that slot is reused
(hinic3_free_txrxq_resources() -> hinic3_free_txqs_res() ->
free_all_tx_skbs()), the stale pointer is consumed:

	if (tx_info->skb) {
		hinic3_tx_unmap_skb(netdev, tx_info->skb,
				    tx_info->dma_info);
		dev_kfree_skb_any(tx_info->skb);
		tx_info->skb = NULL;
	}

hinic3_tx_unmap_skb() reads skb_shinfo(skb)->nr_frags of the freed SKB and
dma_unmap()s stale entries (when the head mapping failed, dma_info[0].dma
holds DMA_MAPPING_ERROR and dma_info[0].len is stale), then the same SKB is
freed a second time.

Would clearing tx_info->skb (and wqebb_cnt) in the rollback block before the
goto be enough here?

> @@ -260,9 +267,11 @@ static int hinic3_tx_csum(struct hinic3_txq *txq, struct hinic3_sq_task *task,
>  		if (l4_proto != IPPROTO_UDP ||
>  		    ((struct udphdr *)skb_transport_header(skb))->dest !=
>  		    VXLAN_OFFLOAD_PORT_LE) {
> +			u64_stats_update_begin(&txq->txq_stats.syncp);
> +			txq->txq_stats.unknown_tunnel_pkt++;
> +			u64_stats_update_end(&txq->txq_stats.syncp);
>  			/* Unsupported tunnel packet, disable csum offload */
> -			skb_checksum_help(skb);
> -			return 0;
> +			return skb_checksum_help(skb);
>  		}
>  	}

[Severity: Low]
Is the comment still accurate after this change?  It says "disable csum
offload", but hinic3_tx_offload() now turns a negative return into a drop:

		tso_cs_en = hinic3_tx_csum(txq, task, skb);
		if (tso_cs_en < 0) {
			offload = HINIC3_TX_OFFLOAD_INVALID;
			return offload;
		}

The comment has also been separated from the statement it describes by the
new accounting block; moving it back above the return, and mentioning the
drop-on-failure behaviour, would read more clearly.

[Severity: Medium]
This is a pre-existing issue, but this hunk sits right on top of it: the
TUNNEL_FLAG bit is set into the task descriptor earlier in hinic3_tx_csum(),
before the tunnel type is known:

		task->pkt_info0 |= cpu_to_le32(SQ_TASK_INFO0_SET(1,
								 TUNNEL_FLAG));

On this software-fallback path nothing clears it again.  When
skb_checksum_help() succeeds the function returns 0, so
HINIC3_TX_OFFLOAD_CSUM is not set, but if the same SKB carries a VLAN tag
hinic3_tx_offload() still sets HINIC3_TX_OFFLOAD_VLAN and
hinic3_send_one_skb() publishes the task section:

	if (offload)
		*wqe_combo.task = task;

Does the device mind being told the frame is tunnelled while INNER_L3_EN,
INNER_L4_EN, PLDOFF and MSS are all zero?  This looks reachable with e.g.
GRE or GENEVE traffic on a VLAN interface.

>  
> @@ -412,6 +421,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;
> +		}
>  		if (tso_cs_en)
>  			offload |= HINIC3_TX_OFFLOAD_CSUM;
>  	}

[ ... ]

> @@ -539,12 +552,17 @@ static netdev_tx_t hinic3_send_one_skb(struct sk_buff *skb,
>  	int err;
>  
>  	if (unlikely(skb->len < MIN_SKB_LEN)) {
> -		if (skb_pad(skb, MIN_SKB_LEN - skb->len))
> +		if (skb_pad(skb, MIN_SKB_LEN - skb->len)) {
> +			u64_stats_update_begin(&txq->txq_stats.syncp);
> +			txq->txq_stats.skb_pad_err++;
> +			u64_stats_update_end(&txq->txq_stats.syncp);
>  			goto err_out;
> +		}
>  
>  		skb->len = MIN_SKB_LEN;
>  	}
>  
> +	offload = hinic3_tx_offload(skb, &task, &queue_info, txq);
>  	num_sge = skb_shinfo(skb)->nr_frags + 1;

[Severity: Medium]
The offload call moved above the free-wqebb check, but the
HINIC3_TX_OFFLOAD_INVALID test stayed below it.  Should the INVALID test
move up with the call?

As it stands, a packet that hinic3_tx_offload() has already declared
untransmittable (skb_checksum_help() failure, hinic3_tso()/skb_cow_head()
failure, or PLDOFF above SQ_CTRL_MAX_PLDOFF) hits this first:

	if (unlikely(hinic3_wq_free_wqebbs(&txq->sq->wq) < wqebb_cnt)) {
		...
		return NETDEV_TX_BUSY;
	}

	if (unlikely(offload == HINIC3_TX_OFFLOAD_INVALID)) {
		goto err_drop_pkt;

so when the SQ is full the qdisc re-queues a packet that will fail
deterministically on every retry, holding up that txq until the SQ drains,
and neither dropped nor busy is accounted for it.

There is a second effect of the reordering: all of hinic3_tx_offload()'s
side effects on the SKB - skb_cow_head(), the in-place pseudo-header writes
in hinic3_tso(), skb_checksum_help() with its possible __skb_linearize() and
ip_summed = CHECKSUM_NONE, and the new unknown_tunnel_pkt increment - are
now performed before the driver knows it can enqueue, so they are repeated
and thrown away on every NETDEV_TX_BUSY attempt.  Since skb_checksum_help()
leaves ip_summed at CHECKSUM_PARTIAL when it fails, that work is redone on
each retry too.

The changelog says only that the call moved "before calculating 'num_sge'";
could it also mention that it now runs before the NETDEV_TX_BUSY return?

[Severity: High]
This isn't a regression from this patch, but this is the line where the
check would belong: num_sge is derived straight from nr_frags with no clamp
against the hardware limit HINIC3_MAX_SQ_SGE (18, hinic3_nic_io.h) and no
skb_linearize() fallback.

hinic3_tx_map_skb() then writes one dma_info entry per fragment with no
bound:

	for (i = 0; i < skb_shinfo(skb)->nr_frags; i++) {
		...
		idx = i + 1;
		dma_info[idx].dma = skb_frag_dma_map(...);

and the per-slot slice is tx_info[idx].dma_info = &tqres->bds[idx *
HINIC3_BDS_PER_SQ_WQEBB], where HINIC3_SQ_WQEBB_SIZE is 16 and
sizeof(struct hinic3_sq_bufdesc) is 16, so HINIC3_BDS_PER_SQ_WQEBB is 1.
The array is allocated in hinic3_alloc_txqs_res() with exactly
HINIC3_MAX_SQ_SGE entries of tail slack:

		tqres->bds = kzalloc_objs(*tqres->bds,
					  sq_depth * HINIC3_BDS_PER_SQ_WQEBB + HINIC3_MAX_SQ_SGE);

With CONFIG_MAX_SKB_FRAGS raised above 18 (net/Kconfig permits up to 45), a
transmit at the tail of the ring writes DMA addresses and lengths past the
end of that allocation, and programs a WQE whose BUFDESC_NUM exceeds the
18-SGE limit.  Grep finds no comparison of num_sge against
HINIC3_MAX_SQ_SGE anywhere in the driver, and the declared frag_size_err and
frag_len_overflow counters are never incremented, which suggests a bounds
check was intended here.  Can this overflow tqres->bds[]?

>  	/* assume normal wqe format + 1 wqebb for task info */
>  	wqebb_cnt = num_sge + 1;
>  
> @@ -560,7 +578,6 @@ static netdev_tx_t hinic3_send_one_skb(struct sk_buff *skb,
>  		return NETDEV_TX_BUSY;
>  	}
>  
> -	offload = hinic3_tx_offload(skb, &task, &queue_info, txq);
>  	if (unlikely(offload == HINIC3_TX_OFFLOAD_INVALID)) {
>  		goto err_drop_pkt;
>  	} else if (!offload) {

[Severity: Low]
These are pre-existing gaps rather than something this patch introduces, but
they are in the same area the patch is closing, so it may be worth covering
them too:

- this NETDEV_TX_BUSY return never increments txq_stats.busy, which is
  already declared and zeroed by hinic3_txq_clean_stats().

- hinic3_xmit_frame() has its own err_drop_pkt label for the carrier-down
  and out-of-range q_id cases, and it still frees the SKB and returns
  NETDEV_TX_OK without touching any counter:

	err_drop_pkt:
		dev_kfree_skb_any(skb);

		return NETDEV_TX_OK;

  so "silent TX drops" remain for those two cases.

- offload_cow_skb_err stays permanently zero even though this patch reworks
  the hinic3_tso()/skb_cow_head() failure branch that the counter was
  declared for:

	tso_cs_en = hinic3_tso(task, queue_info, skb);
	if (tso_cs_en < 0) {
		offload = HINIC3_TX_OFFLOAD_INVALID;
		return offload;
	}

[ ... ]

> @@ -604,6 +621,10 @@ static netdev_tx_t hinic3_send_one_skb(struct sk_buff *skb,
>  err_drop_pkt:
>  	dev_kfree_skb_any(skb);
>  err_out:
> +	u64_stats_update_begin(&txq->txq_stats.syncp);
> +	txq->txq_stats.dropped++;
> +	u64_stats_update_end(&txq->txq_stats.syncp);
> +
>  	return NETDEV_TX_OK;
>  }

[Severity: Medium]
This is a pre-existing problem, not one this patch creates, but the patch
makes it easier to notice: hinic3_get_stats64() publishes tx_packets and
tx_bytes from fields that nothing ever increments.

	stats->tx_packets = packets;
	stats->tx_bytes   = bytes;
	stats->tx_dropped = dropped;

The only site in the driver that touches txq_stats->packets and
txq_stats->bytes is hinic3_txq_clean_stats():

	txq_stats->bytes = 0;
	txq_stats->packets = 0;

The TX path accounts bytes through netif_subqueue_sent() and
netif_subqueue_completed_wake() (BQL) instead, so "ip -s link show" reports
tx_packets and tx_bytes as 0.  After this patch tx_dropped becomes non-zero
while those two stay at zero.  Should the packet and byte counters be
incremented in hinic3_send_one_skb() as well?

[Severity: Low]
This is a pre-existing style point about the reader side of these counters
rather than a bug in this patch, and no user is affected today:
hinic3_get_stats64() accumulates with "+=" inside the retry loop:

	do {
		start = u64_stats_fetch_begin(&txq_stats->syncp);
		bytes += txq_stats->bytes;
		packets += txq_stats->packets;
		dropped += txq_stats->dropped;
	} while (u64_stats_fetch_retry(&txq_stats->syncp, start));

A retry would add each counter twice.  It cannot happen here, because
drivers/net/ethernet/huawei/hinic3/Kconfig has "depends on PCI_MSI && 64BIT"
and the 64-bit __u64_stats_fetch_retry() in include/linux/u64_stats_sync.h
is a compile-time "return false".  Would using local temporaries assigned
inside the loop and adding them after it be preferable, so the loop body
stays idempotent?
-- 
pw-bot: cr