Re: [2/3] net: amd-xgbe: add ARP offload ethtool self-test

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.

For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
---
net: amd-xgbe: add ARP offload ethtool self-test

This patch adds an ethtool self-test to verify the hardware ARP offload
functionality. The test creates an ARP request, enables ARP offload, and
verifies the hardware responds with an ARP reply.

> diff --git a/drivers/net/ethernet/amd/xgbe/xgbe-selftest.c b/drivers/net/ethernet/amd/xgbe/xgbe-selftest.c
> --- a/drivers/net/ethernet/amd/xgbe/xgbe-selftest.c
> +++ b/drivers/net/ethernet/amd/xgbe/xgbe-selftest.c

[ ... ]

> +static int xgbe_test_arpoffload(struct xgbe_prv_data *pdata)
> +{

[ ... ]

> +	tdata->pt.type = htons(ETH_P_ARP);
> +	tdata->pt.func = xgbe_test_arp_validate;
> +	tdata->pt.dev = pdata->netdev;
> +	tdata->pt.af_packet_priv = tdata;
> +	tdata->packet = &attr;
> +	dev_add_pack(&tdata->pt);

[ ... ]

> +	/* Create ARP request packet */
> +	skb = arp_create(ARPOP_REQUEST, ETH_P_ARP, htonl(dst_ip),
> +			 pdata->netdev,	htonl(src_ip), NULL, src, bcast);
> +	if (!skb) {
> +		ret = -ENOMEM;
> +		goto free;
> +	}

Can this cause a use-after-free? The packet handler is registered via
dev_add_pack() before arp_create() is called. When arp_create() fails,
'goto free' jumps directly to kfree(tdata) without calling
dev_remove_pack(&tdata->pt).

This leaves the packet handler registered with a dangling pointer
(tdata->pt.af_packet_priv = tdata). When any ARP packet subsequently
arrives, xgbe_test_arp_validate() is called and dereferences freed memory:

    struct net_test_priv *tdata = pt->af_packet_priv;
    ...
    if (!ether_addr_equal_unaligned(eth_hdr->h_dest, tdata->packet->src))

Should 'goto free' be changed to 'goto cleanup' here to ensure
dev_remove_pack() is called?
-- 
pw-bot: cr