Re: [net-next,v20,3/7] net: mtip: Add buffers management functions to the L2 switch driver

Hi Jakub,

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: mtip: Add buffers management functions to the L2 switch driver

This patch adds buffer management functions for the MTIP L2 switch
driver, implementing mtip_free_buffers(), mtip_create_page_pool(), and
mtip_alloc_buffers() for RX page pool and TX bounce buffer handling.

quoted

diff --git a/drivers/net/ethernet/freescale/mtipsw/mtipl2sw.c

b/drivers/net/ethernet/freescale/mtipsw/mtipl2sw.c index
03c5163d6508..9c21a3d29c1c 100644 ---
a/drivers/net/ethernet/freescale/mtipsw/mtipl2sw.c +++
b/drivers/net/ethernet/freescale/mtipsw/mtipl2sw.c @@ -888,11
+888,96 @@ static void mtip_get_drvinfo(struct net_device *dev,

 static void mtip_free_buffers(struct net_device *dev)
 {
+	struct mtip_ndev_priv *priv = netdev_priv(dev);
+	struct switch_enet_private *fep = priv->fep;
+	int i;
+
+	for (i = 0; i < RX_RING_SIZE; i++) {
+		page_pool_put_full_page(fep->page_pool,
+					fep->page[i], false);  

Can fep->page[i] be NULL here?  When mtip_alloc_buffers() fails
partway through the RX allocation loop, it calls mtip_free_buffers()
via the error path.  At that point, only some fep->page[] entries
have been allocated - the rest remain NULL from the kzalloc
initialization.

The call chain page_pool_put_full_page()->page_pool_put_netmem()->
page_pool_unref_and_test()->page_pool_unref_netmem()->
netmem_get_pp_ref_count_ref() will attempt to access
&NULL->pp_ref_count when passed a NULL page, causing a NULL pointer
dereference.

Should there be a NULL check before calling page_pool_put_full_page()?

Yes, such check shall be added.

quoted

+		fep->page[i] = NULL;
+	}
+
+	page_pool_destroy(fep->page_pool);
+	fep->page_pool = NULL;
+
+	for (i = 0; i < TX_RING_SIZE; i++)
+		kfree(fep->tx_bounce[i]);
 }  

[ ... ]

quoted

+static int mtip_alloc_buffers(struct net_device *dev)
+{
+	struct mtip_ndev_priv *priv = netdev_priv(dev);
+	struct switch_enet_private *fep = priv->fep;
+	struct cbd_t *bdp;
+	struct page *page;
+	int i, ret;
+
+	ret = mtip_create_page_pool(fep, RX_RING_SIZE);
+	if (ret < 0) {
+		dev_err(&fep->pdev->dev, "Failed to create page
pool\n");
+		return ret;
+	}
+
+	bdp = fep->rx_bd_base;
+	for (i = 0; i < RX_RING_SIZE; i++) {
+		page = page_pool_dev_alloc_pages(fep->page_pool);
+		if (!page) {
+			dev_err(&fep->pdev->dev,
+				"Failed to allocate page for rx
buffer\n");
+			goto err;
+		}  

[ ... ]

quoted

+ err:
+	mtip_free_buffers(dev);  

When this error path is taken, fep->page[] may contain NULL entries
for pages that were never allocated.

quoted

+	return -ENOMEM;
 }  

I will add proper fix for v21.

-- 
Best regards,

Łukasz Majewski