Re: [PATCH v1 09/15] net/xsc: add ethernet RX path and statistics

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] net/xsc: missing RQ state check in xsc_eth_post_rx_wqes() and NAPI order bug
- [Medium] net/xsc: unchecked initial RQ refill and doorbell write on alloc failure
- [Medium] net/xsc: xsc_skb_set_hash() classifies all IPv4/IPv6 packets as L4 hash
- [Medium] net/xsc: missing drop stats and dead clamp in xsc_eth_handle_rx_cqe()
- [Medium] net/xsc: host-endian u16 read in XSC_GET_EPP2SOC_PPH_EXT_TUNNEL_TYPE()
- [Low] net/xsc: xsc_eth_open_channels() zeroes chls->num_chl before logging it
--

commit 1ef80bdcef5b1d9741776856d8624e7ce0755662
Author: Xin Tian <tianx@yunsilicon.com>

net/xsc: add ethernet RX path and statistics

This commit implements the ethernet receive path using page pools, linear and
non-linear SKB construction, CQE checksum and RSS hash handling, and RSS RQ
creation. It also wires up ndo_get_stats64 and advertises RX checksum and RX
hash offloads.

> diff --git a/drivers/net/ethernet/yunsilicon/xsc/net/main.c b/drivers/net/ethernet/yunsilicon/xsc/net/main.c
> index a02f95145d018..e56e0eec30515 100644
> --- a/drivers/net/ethernet/yunsilicon/xsc/net/main.c
> +++ b/drivers/net/ethernet/yunsilicon/xsc/net/main.c
> @@ -686,6 +686,14 @@ static int xsc_eth_open_channel(struct xsc_adapter *adapter,
>  static int xsc_eth_modify_qps_channel(struct xsc_adapter *adapter,
>  				      struct xsc_channel *c)
>  {
> +	int ret;
> +
> +	c->qp.rq.post_wqes(&c->qp.rq);
> +	ret = xsc_core_eth_modify_qp_status(adapter->xdev, c->qp.rq.rqn,
> +					    XSC_CMD_OP_RTR2RTS_QP);
> +	if (ret)
> +		return ret;
> +
>  	return xsc_core_eth_modify_qp_status(adapter->xdev, c->qp.sq.sqn,
>  					     XSC_CMD_OP_RTR2RTS_QP);
>  }

[ ... ]

> @@ -745,6 +1075,14 @@ static int xsc_eth_open_channels(struct xsc_adapter *adapter)
>  			goto err_close_channel;
>  	}
>  
> +	ret = xsc_eth_open_rss_qp_rqs(adapter,
> +				      &chl_param->rq_param,
> +				      chls,
> +				      chls->num_chl);
> +	if (ret)
> +		goto err_close_channel;
> +	free_rq = true;
> +
>  	for (i = 0; i < chls->num_chl; i++)
>  		napi_enable(&chls->c[i].napi);
>  
> @@ -763,7 +1101,7 @@ static int xsc_eth_open_channels(struct xsc_adapter *adapter)
>  
>  err_close_channel:
>  	for (--i; i >= 0; i--)
> -		xsc_eth_close_channel(&chls->c[i], false);
> +		xsc_eth_close_channel(&chls->c[i], free_rq);
>  
>  	kvfree(chl_param);
>  err_free_ch:

[Severity: Low]
When xsc_eth_open_channels() fails (for example if xsc_eth_open_rss_qp_rqs()
or xsc_eth_modify_qps() returns an error), it jumps through err_close_channel
to err_out:

err_out:
	chls->num_chl = 0;
	netdev_err(adapter->netdev, "failed to open %d channels, err=%d\n",
		   chls->num_chl, ret);

Since chls->num_chl is cleared to 0 right before netdev_err(), won't this
always log "failed to open 0 channels" instead of the number of channels that
failed to open?

[ ... ]

> diff --git a/drivers/net/ethernet/yunsilicon/xsc/net/xsc_eth_rx.c b/drivers/net/ethernet/yunsilicon/xsc/net/xsc_eth_rx.c
> new file mode 100644
> index 0000000000000..00ac7eacc0e99
> --- /dev/null
> +++ b/drivers/net/ethernet/yunsilicon/xsc/net/xsc_eth_rx.c
> @@ -0,0 +1,556 @@

[ ... ]

> +static void xsc_skb_set_hash(struct xsc_adapter *adapter,
> +			     struct xsc_cqe *cqe,
> +			     struct sk_buff *skb)
> +{
> +	struct xsc_rss_params *rss = &adapter->rss_param;
> +	bool l3_hash = false;
> +	bool l4_hash = false;
> +	u32 hash_field;
> +	int ht = 0;
> +
> +	if (adapter->netdev->features & NETIF_F_RXHASH) {
> +		if (skb->protocol == htons(ETH_P_IP)) {
> +			hash_field = rss->rx_hash_fields[XSC_TT_IPV4_TCP];
> +			if (hash_field & XSC_HASH_FIELD_SEL_SRC_IP ||
> +			    hash_field & XSC_HASH_FIELD_SEL_DST_IP)
> +				l3_hash = true;
> +
> +			if (hash_field & XSC_HASH_FIELD_SEL_SPORT ||
> +			    hash_field & XSC_HASH_FIELD_SEL_DPORT)
> +				l4_hash = true;
> +		} else if (skb->protocol == htons(ETH_P_IPV6)) {
> +			hash_field = rss->rx_hash_fields[XSC_TT_IPV6_TCP];
> +			if (hash_field & XSC_HASH_FIELD_SEL_SRC_IPV6 ||
> +			    hash_field & XSC_HASH_FIELD_SEL_DST_IPV6)
> +				l3_hash = true;
> +
> +			if (hash_field & XSC_HASH_FIELD_SEL_SPORT_V6 ||
> +			    hash_field & XSC_HASH_FIELD_SEL_DPORT_V6)
> +				l4_hash = true;
> +		}
> +
> +		if (l3_hash && l4_hash)
> +			ht = PKT_HASH_TYPE_L4;
> +		else if (l3_hash)
> +			ht = PKT_HASH_TYPE_L3;
> +		if (ht)
> +			skb_set_hash(skb, le32_to_cpu(cqe->vni), ht);
> +	}
> +}

[Severity: Medium]
In xsc_skb_set_hash(), any packet with skb->protocol equal to htons(ETH_P_IP)
looks up rss->rx_hash_fields[XSC_TT_IPV4_TCP], and any packet with
htons(ETH_P_IPV6) looks up rss->rx_hash_fields[XSC_TT_IPV6_TCP].

In xsc_build_rss_param(), those two entries are initialized to
XSC_HASH_IP_PORTS and XSC_HASH_IP6_PORTS, which always have both the L3 IP
address bits and L4 port bits set.

Because the actual L4 protocol and fragmentation status of the received packet
are not checked here, won't every IPv4 and IPv6 packet (including ICMP, other
non-TCP/UDP protocols, and fragmented IP packets) be unconditionally reported
to the network stack with PKT_HASH_TYPE_L4?

> +
> +static void xsc_handle_csum(struct xsc_cqe *cqe, struct xsc_rq *rq,
> +			    struct sk_buff *skb, struct xsc_wqe_frag_info *wi)
> +{
> +	struct xsc_dma_info *dma_info;
> +	struct net_device *netdev;
> +	void *hw_pph;
> +	struct xsc_channel *c;
> +	int offset_from;
> +
> +	c = rq->cq.channel;
> +	netdev = c->adapter->netdev;
> +	dma_info = wi->di;
> +	offset_from = wi->offset;
> +	hw_pph = page_address(dma_info->page) + offset_from;
> +
> +	if (unlikely((netdev->features & NETIF_F_RXCSUM) == 0))
> +		goto csum_none;
> +
> +	if (!(le32_to_cpu(cqe->data0) & XSC_CQE_DATA0_HAS_PPH))
> +		goto csum_none;
> +
> +	dma_sync_single_for_cpu(rq->cq.xdev->device,
> +				dma_info->addr + offset_from,
> +				XSC_PPH_HEAD_LEN, DMA_FROM_DEVICE);
> +
> +	if (unlikely(XSC_GET_EPP2SOC_PPH_ERROR_BITMAP(hw_pph) & PACKET_UNKNOWN))
> +		goto csum_none;
> +
> +	if (XSC_GET_EPP2SOC_PPH_EXT_TUNNEL_TYPE(hw_pph) &&
> +	    (!(FIELD_GET(XSC_CQE_DATA0_CSUM_ERR_MASK, le32_to_cpu(cqe->data0)) &
> +	       OUTER_AND_INNER))) {

[Severity: Medium]
In xsc_handle_csum(), XSC_GET_EPP2SOC_PPH_EXT_TUNNEL_TYPE(hw_pph) is used to
check tunnel encapsulation on the DMA-synced packet parse header. Looking at
drivers/net/ethernet/yunsilicon/xsc/net/xsc_pph.h:

#define XSC_GET_EPP2SOC_PPH_EXT_TUNNEL_TYPE(PPH_BASE_ADDR)	\
	((*(u16 *)((u8 *)(PPH_BASE_ADDR) +	\
	EPP2SOC_PPH_EXT_TUNNEL_TYPE_OFFSET) &	\
	EPP2SOC_PPH_EXT_TUNNEL_TYPE_BIT_MASK) >>	\
	EPP2SOC_PPH_EXT_TUNNEL_TYPE_BIT_OFFSET)

Since this dereferences a u16 from the DMA buffer in host byte order without
endianness conversion and masks with 0x0f00 >> 8, won't it read the wrong
byte of hw_pph on big-endian architectures and misclassify tunnel checksums?

[ ... ]

> +void xsc_eth_handle_rx_cqe(struct xsc_cqwq *cqwq,
> +			   struct xsc_rq *rq, struct xsc_cqe *cqe)
> +{
> +	struct xsc_wq_cyc *wq = &rq->wqe.wq;
> +	u8 cqe_opcode = get_cqe_opcode(cqe);
> +	struct xsc_wqe_frag_info *wi;
> +	struct sk_buff *skb;
> +	u32 cqe_bcnt;
> +	u16 ci;
> +
> +	ci = xsc_wq_cyc_ctr2ix(wq, cqwq->cc);
> +	wi = get_frag(rq, ci);
> +	if (unlikely(cqe_opcode & BIT(7))) {
> +		xsc_dump_error_rqcqe(rq, cqe);
> +		goto free_wqe;
> +	}
> +
> +	cqe_bcnt = le32_to_cpu(cqe->msg_len);
> +	if ((le32_to_cpu(cqe->data0) & XSC_CQE_DATA0_HAS_PPH) &&
> +	    cqe_bcnt <= XSC_PPH_HEAD_LEN)
> +		goto free_wqe;
> +
> +	if (unlikely(cqe_bcnt < ETH_HLEN + ((le32_to_cpu(cqe->data0) &
> +					    XSC_CQE_DATA0_HAS_PPH) ?
> +					    XSC_PPH_HEAD_LEN : 0))) {
> +		rq->stats->dropped++;
> +		goto free_wqe;
> +	}
> +
> +	if (unlikely(cqe_bcnt > rq->frags_sz))
> +		goto free_wqe;
> +
> +	cqe_bcnt = min_t(u32, cqe_bcnt, rq->frags_sz);

[Severity: Medium]
In xsc_eth_handle_rx_cqe(), when XSC_CQE_DATA0_HAS_PPH is set and cqe_bcnt is
less than or equal to XSC_PPH_HEAD_LEN, or when cqe_bcnt > rq->frags_sz, the
frame jumps to free_wqe without incrementing rq->stats->dropped (which
xsc_eth_fold_sw_stats64() folds into rx_length_errors).

In addition, the second check (cqe_bcnt < ETH_HLEN + XSC_PPH_HEAD_LEN) already
covers cqe_bcnt <= XSC_PPH_HEAD_LEN when XSC_CQE_DATA0_HAS_PPH is set, and the
min_t(u32, cqe_bcnt, rq->frags_sz) clamp right after the cqe_bcnt > rq->frags_sz
branch is unreachable.

Should the first check and the redundant min_t() be removed, and should the
cqe_bcnt > rq->frags_sz path also increment rq->stats->dropped?

[ ... ]

> +bool xsc_eth_post_rx_wqes(struct xsc_rq *rq)
> +{
> +	struct xsc_wq_cyc *wq = &rq->wqe.wq;
> +	u8 wqe_bulk, wqe_bulk_min;
> +	int err = 0;
> +	int alloc;
> +	u16 head;
> +
> +	wqe_bulk = rq->wqe.info.wqe_bulk;
> +	wqe_bulk_min = rq->wqe.info.wqe_bulk_min;
> +	if (xsc_wq_cyc_missing(wq) < wqe_bulk)
> +		return false;
> +
> +	do {
> +		head = xsc_wq_cyc_get_head(wq);
> +
> +		alloc = min_t(int, wqe_bulk, xsc_wq_cyc_missing(wq));
> +		if (alloc < wqe_bulk && alloc >= wqe_bulk_min)
> +			alloc = alloc & 0xfffffffe;
> +
> +		if (alloc > 0) {
> +			err = xsc_alloc_rx_wqes(rq, head, alloc);
> +			if (unlikely(err))
> +				break;
> +
> +			xsc_wq_cyc_push_n(wq, alloc);
> +		}
> +	} while (xsc_wq_cyc_missing(wq) >= wqe_bulk_min);
> +
> +	dma_wmb();
> +
> +	/* ensure wqes are visible to device before updating doorbell record */
> +	xsc_rq_notify_hw(rq);
> +
> +	return !!err;
> +}

[Severity: Medium]
If xsc_alloc_rx_wqes() fails on the first iteration of the loop in
xsc_eth_post_rx_wqes(), no WQEs are pushed to wq, but execution still falls
through to xsc_rq_notify_hw(rq) and writes the unchanged wq->wqe_ctr to the
hardware RX doorbell.

In addition, during ndo_open, xsc_eth_modify_qps_channel() calls
c->qp.rq.post_wqes(&c->qp.rq) without checking its return value or whether the
RQ ring is still empty before transitioning the RQ to RTS. If initial page
allocation fails, won't the RQ enter RTS with zero posted WQEs and never
receive completions to trigger a refill?

[ ... ]

> diff --git a/drivers/net/ethernet/yunsilicon/xsc/net/xsc_eth_txrx.c b/drivers/net/ethernet/yunsilicon/xsc/net/xsc_eth_txrx.c
> index 262421740e974..42e6305478913 100644
> --- a/drivers/net/ethernet/yunsilicon/xsc/net/xsc_eth_txrx.c
> +++ b/drivers/net/ethernet/yunsilicon/xsc/net/xsc_eth_txrx.c
> @@ -118,7 +118,9 @@ int xsc_eth_napi_poll(struct napi_struct *napi, int budget)
>  {
>  	struct xsc_channel *c = container_of(napi, struct xsc_channel, napi);
>  	struct xsc_eth_params *params = &c->adapter->nic_param;
> +	struct xsc_rq *rq = &c->qp.rq;
>  	bool busy = false;
> +	int work_done = 0;
>  	int tx_budget;
>  
>  	rcu_read_lock();
> @@ -126,18 +128,29 @@ int xsc_eth_napi_poll(struct napi_struct *napi, int budget)
>  	tx_budget = params->sq_size >> 2;
>  	busy |= xsc_poll_tx_cq(&c->qp.sq.cq, tx_budget);
>  
> +	/* budget=0 means: don't poll rx rings */
> +	if (likely(budget)) {
> +		work_done = xsc_poll_rx_cq(&rq->cq, budget);
> +		busy |= work_done == budget;
> +	}
> +
> +	busy |= rq->post_wqes(rq);

[Severity: High]
Can xsc_eth_post_rx_wqes() race with NAPI or run while the RQ is disabled?

Looking at the channel open sequence in xsc_eth_sw_init():

xsc_eth_open_channels()
    for (i = 0; i < chls->num_chl; i++)
        napi_enable(&chls->c[i].napi);
    smp_wmb();
    xsc_eth_modify_qps()
        xsc_eth_modify_qps_channel()
            c->qp.rq.post_wqes(&c->qp.rq);
            xsc_core_eth_modify_qp_status(..., XSC_CMD_OP_RTR2RTS_QP);
xsc_eth_activate_priv_channels()
    xsc_eth_activate_channel()
        xsc_activate_rq() -> set_bit(XSC_ETH_RQ_STATE_ENABLED, &c->qp.rq.state)

Unlike xsc_poll_rx_cq(), xsc_eth_post_rx_wqes() does not check
test_bit(XSC_ETH_RQ_STATE_ENABLED, &rq->state), and xsc_eth_napi_poll() calls
rq->post_wqes(rq) unconditionally (even when budget == 0 or when
XSC_ETH_RQ_STATE_ENABLED is not set).

This appears to allow several issues:

1. Because napi_enable() is called before xsc_eth_modify_qps_channel() calls
   c->qp.rq.post_wqes(&c->qp.rq) in process context without BH disabled, a
   concurrent NAPI poll (via busy-poll, netpoll, or a CQ interrupt) can run
   rq->post_wqes(rq) at the same time on the same RQ, corrupting wq->wqe_ctr,
   wq->cur_sz, frag->di->page, and the non-thread-safe page_pool alloc cache.

2. While xsc_eth_modify_qps() is iterating over channels across sleeping
   firmware commands, earlier channels have already transitioned to RTS while
   XSC_ETH_RQ_STATE_ENABLED is still 0, so incoming packets can trigger
   xsc_eth_completion_event() while xsc_poll_rx_cq() refuses to drain the CQ.

3. During teardown in xsc_eth_deactivate_channels(), xsc_deactivate_rq()
   clears XSC_ETH_RQ_STATE_ENABLED before napi_disable(), but in-flight NAPI
   polls will still call rq->post_wqes(rq), allocating pages and ringing the
   RX doorbell on the deactivated RQ (and if page allocation fails, returning
   busy = true so xsc_eth_napi_poll() never completes NAPI).

Should xsc_eth_post_rx_wqes() check test_bit(XSC_ETH_RQ_STATE_ENABLED,
&rq->state), and should XSC_ETH_RQ_STATE_ENABLED and initial WQE posting happen
before napi_enable() and the RTR2RTS transition?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261104155650.37053-1-tianx@yunsilicon.com?part=9