Re: [PATCH net-next v09 2/5] hinic3: Add ethtool statistic ops

[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
---
Subject: hinic3: Add ethtool statistic ops

This commit adds PF/VF statistics functions for TX and RX processing in the
hinic3 driver. It implements various ethtool callbacks to allow detailed
netdev stats monitoring. It also adds a helper for management commands
requiring separate I/O buffers to retrieve expanded MAC stats.

diff --git a/drivers/net/ethernet/huawei/hinic3/hinic3_ethtool.c b/drivers/net/ethernet/huawei/hinic3/hinic3_ethtool.c
--- a/drivers/net/ethernet/huawei/hinic3/hinic3_ethtool.c
+++ b/drivers/net/ethernet/huawei/hinic3/hinic3_ethtool.c
[ ... ]
> +static struct hinic3_stats hinic3_function_stats[] = {
> +	HINIC3_FUNC_STAT(tx_unicast_pkts_vport),
[ ... ]
> +static struct hinic3_stats hinic3_port_stats[] = {
> +	HINIC3_PORT_STAT(mac_tx_fragment_pkt_num),

[Severity: Medium]
Does this duplicate standard network counters into the private ethtool strings
interface?

The networking driver guidelines note that stats with a standard UAPI should
not be duplicated in the private strings.

The arrays for function and port stats export standard statistics like unicast
packets or MAC errors. Could these be exposed exclusively through standard
interfaces like rtnl_link_stats64 or ethtool_mac_stats instead?

[ ... ]
diff --git a/drivers/net/ethernet/huawei/hinic3/hinic3_rx.c b/drivers/net/ethernet/huawei/hinic3/hinic3_rx.c
--- a/drivers/net/ethernet/huawei/hinic3/hinic3_rx.c
+++ b/drivers/net/ethernet/huawei/hinic3/hinic3_rx.c
[ ... ]
> @@ -588,5 +643,10 @@ int hinic3_rx_poll(struct hinic3_rxq *rxq, int budget)
>  	if (rxq->delta >= HINIC3_RX_BUFFER_WRITE)
>  		hinic3_rx_fill_buffers(rxq);
>  
> +	u64_stats_update_begin(&rxq->rxq_stats.syncp);
> +	rxq->rxq_stats.packets += (u64)nr_pkts;
> +	rxq->rxq_stats.bytes += rx_bytes;
> +	u64_stats_update_end(&rxq->rxq_stats.syncp);

[Severity: Medium]
Does this calculation undercount the number of physical packets received when
Large Receive Offload is active?

Earlier in the loop, the code accounts for aggregated wire packets by adding
the length of the headers for the aggregated packets to the bytes counter.

However, the rxq_stats.packets update only adds the number of host SKBs,
omitting the extra wire packets entirely.

[ ... ]
diff --git a/drivers/net/ethernet/huawei/hinic3/hinic3_tx.c b/drivers/net/ethernet/huawei/hinic3/hinic3_tx.c
--- a/drivers/net/ethernet/huawei/hinic3/hinic3_tx.c
+++ b/drivers/net/ethernet/huawei/hinic3/hinic3_tx.c
[ ... ]
> @@ -260,6 +267,9 @@ 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;

[Severity: High]
This is a pre-existing issue, but does ignoring the return value of the
software checksum fallback risk silent data corruption?

If the calculation in skb_checksum_help fails, for example due to an
allocation failure when ensuring the buffer is writable, the packet checksum
remains uncalculated. 

Since the error is ignored and 0 is returned, the driver proceeds to transmit
the packet without hardware offload, resulting in a corrupted packet being
sent onto the network.

[ ... ]
> +static void hinic3_get_pkt_stats(struct hinic3_txq *txq, struct sk_buff *skb)
> +{
> +	u32 hdr_len, tx_bytes;
> +	unsigned short pkts;
> +
> +	if (skb_is_gso(skb)) {
> +		hdr_len = (skb_shinfo(skb)->gso_segs - 1) *
> +			  skb_tcp_all_headers(skb);
> +		tx_bytes = skb->len + hdr_len;

[Severity: High]
Can this sequence cause an out-of-bounds read and statistics corruption for
UDP GSO packets?

The code assumes all GSO packets use TCP by calling a helper that reads the
TCP header length. If the driver handles UDP-encapsulated offloads, the
transport header is much shorter. 

Reading the data offset from a TCP header location accesses bytes past the end
of the UDP header. This could read arbitrary payload data as a header length
multiplier, corrupting the tx_bytes statistic and potentially causing a
page fault.

[ ... ]