Re: [PATCH net-next v3 1/6] net: xilinx: axienet: Introduce axienet_config for MAC-specific ops

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] Incomplete abstraction for TX checksum offload leads to broken checksums if misconfigured.

Pre-existing issues:
- [Critical] TX ring desynchronization in axienet_start_xmit_dmaengine() error paths leads to use-after-free and double-free in DMA callbacks.
--

commit 166384442bde8d7169bb6c9fd071e535fa3f9c73
Author: Suraj Gupta <suraj.gupta2@amd.com>

net: xilinx: axienet: Introduce axienet_config for MAC-specific ops

Introduce an axienet_config structure to abstract MAC-type-specific
operations, preparing the driver for supporting additional MAC types
beyond the existing 1G Ethernet.

> diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> index 1722b7038f340..a4963da53b306 100644
> --- a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> +++ b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
[ ... ]
> @@ -939,19 +976,21 @@ axienet_start_xmit_dmaengine(struct sk_buff *skb, struct net_device *ndev)
>  		goto xmit_error_drop_skb;
>  
>  	/* Fill up app fields for checksum */
> -	if (skb->ip_summed == CHECKSUM_PARTIAL) {
> -		if (lp->features & XAE_FEATURE_FULL_TX_CSUM) {
> -			/* Tx Full Checksum Offload Enabled */
> -			app_metadata[0] |= 2;
> -		} else if (lp->features & XAE_FEATURE_PARTIAL_TX_CSUM) {
> -			csum_start_off = skb_transport_offset(skb);
> -			csum_index_off = csum_start_off + skb->csum_offset;
> -			/* Tx Partial Checksum Offload Enabled */
> -			app_metadata[0] |= 1;
> -			app_metadata[1] = (csum_start_off << 16) | csum_index_off;
> +	if (lp->axienet_config->dma_tx_csum) {
> +		if (skb->ip_summed == CHECKSUM_PARTIAL) {

[Severity: Critical]
This is a pre-existing issue, but does this error path permanently
desynchronize the software ring from the hardware ring?

Looking at axienet_start_xmit_dmaengine(), if a failure occurs during
mapping or preparation, the code jumps to xmit_error_drop_skb. However,
lp->tx_ring_head was already incremented earlier and is not rolled back.
This leaves the abandoned ring slot containing the skb pointer and sg_len
from the previous time the slot was used.

When the next successful packet finishes transmission, axienet_dma_tx_cb()
will pop the oldest entry using lp->tx_ring_tail++ and retrieve this
abandoned slot. Can this lead to unmapping the wrong scatterlist and
double-freeing the old skb?

[ ... ]
> @@ -2861,9 +2993,6 @@ static int axienet_probe(struct platform_device *pdev)
>  	/* Setup checksum offload, but default to off if not specified */
>  	lp->features = 0;
>  
> -	if (axienet_ior(lp, XAE_ABILITY_OFFSET) & XAE_ABILITY_STATS)
> -		lp->features |= XAE_FEATURE_STATS;
> -
>  	ret = of_property_read_u32(pdev->dev.of_node, "xlnx,txcsum", &value);
>  	if (!ret) {
>  		switch (value) {
>  		case 1:
>  			lp->features |= XAE_FEATURE_PARTIAL_TX_CSUM;
>  			/* Can checksum any contiguous range */
>  			ndev->features |= NETIF_F_HW_CSUM;

[Severity: High]
Does axienet_probe() advertise hardware checksum capabilities without
verifying the new MAC capability flag?

While axienet_start_xmit_dmaengine() now correctly gates the DMA metadata
setup behind lp->axienet_config->dma_tx_csum, axienet_probe() still parses
the xlnx,txcsum device tree property and unconditionally sets
NETIF_F_HW_CSUM.

If the device tree specifies this property for a MAC that does not support
TX checksums, will the network stack pass partial checksums that the
hardware never completes?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260831150816.1020883-1-suraj.gupta2@amd.com?part=1