[PATCH net v3 0/2] net/stmmac: Secure against failures of DMA memory allocation

[PATCH net v3 0/2] net/stmmac: Secure against failures of DMA memory allocation

[Jakub Raczynski]

This series fixing two issues related to fails of
__alloc_dma_rx_desc_resources(). Original issue from 1st patch is related to
page_pool that has happened in testing env, while second was requested by
Sashiko to have similar change for DMA allocation. 
To have complete fix for all failures of __alloc_dma_rx_desc_resources(),
merge two fixes into series.

---
Changes in v3:
- Added null assignment to page pointers (suggested by sashiko)
- Convert IS_ERR_OR_NULL check to just NULL check

Changes in v2:
- Added reviewed by Maxime to first patch
- Dropped null check as page_pool_destroy() does provide that
- Modified comment to reflect that

Link to v2:
https://lore.kernel.org/all/20260707174115.1264466-1-j.raczynski@samsung.com/

Link to v1:
https://lore.kernel.org/netdev/20260630100953.747868-1-j.raczynski@samsung.com/

Jakub Raczynski (2):
  net/stmmac: Set Rx queue page_pool to NULL when freeing DMA resources
  net/stmmac: Prevent dma queue NULL free on allocation failure

 .../net/ethernet/stmicro/stmmac/stmmac_main.c | 32 ++++++++++++-------
 1 file changed, 21 insertions(+), 11 deletions(-)

-- 
2.34.1

[PATCH net v3 1/2] net/stmmac: Set Rx queue page_pool to NULL when freeing DMA resources

[Jakub Raczynski]

When freeing RX descriptor resources, there is standard clearing of
descriptor page_pool via page_pool_destroy() which does destroy
page but does not set its pointer to NULL, which must be done by driver
calling this function.
It is not done in __free_dma_rx_desc_resources() when stopping interface,
which is generally not an issue, because __alloc_dma_rx_desc_resources() does
setup this regardless of previous state.
But above is true assuming reinitialization is successful.

In case of failure of page_pool_create() in __alloc_dma_rx_desc_resources(),
all non-NULL pages will be freed, including those already cleared.
So there is possible kernel panic due to wrong paging request at address.

Fix this by assigning NULL to page_pool pointer on free.
Also remove NULL check as page_pool_destroy() does check for NULL param.

Fixes: da5ec7f22a0f1 ("net: stmmac: refactor stmmac_init_rx_buffers for stmmac_reinit_rx_buffers")
Signed-off-by: Yashwant Varur <yashwant.v@samsung.com>
Signed-off-by: Jakub Raczynski <j.raczynski@samsung.com>
Reviewed-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 2a0d7eff88d3..3098971e0b66 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -2172,8 +2172,8 @@ static void __free_dma_rx_desc_resources(struct stmmac_priv *priv,
 		xdp_rxq_info_unreg(&rx_q->xdp_rxq);
 
 	kfree(rx_q->buf_pool);
-	if (rx_q->page_pool)
-		page_pool_destroy(rx_q->page_pool);
+	page_pool_destroy(rx_q->page_pool);
+	rx_q->page_pool = NULL;
 }
 
 static void free_dma_rx_desc_resources(struct stmmac_priv *priv,
-- 
2.34.1

Re: [PATCH net v3 1/2] net/stmmac: Set Rx queue page_pool to NULL when freeing DMA resources

[Mina Almasry]

On Wed, Jul 15, 2026 at 5:42 AM Jakub Raczynski <j.raczynski@samsung.com> wrote:
>
> When freeing RX descriptor resources, there is standard clearing of
> descriptor page_pool via page_pool_destroy() which does destroy
> page but does not set its pointer to NULL, which must be done by driver
> calling this function.
> It is not done in __free_dma_rx_desc_resources() when stopping interface,
> which is generally not an issue, because __alloc_dma_rx_desc_resources() does
> setup this regardless of previous state.
> But above is true assuming reinitialization is successful.
>
> In case of failure of page_pool_create() in __alloc_dma_rx_desc_resources(),
> all non-NULL pages will be freed, including those already cleared.
> So there is possible kernel panic due to wrong paging request at address.
>
> Fix this by assigning NULL to page_pool pointer on free.
> Also remove NULL check as page_pool_destroy() does check for NULL param.
>
> Fixes: da5ec7f22a0f1 ("net: stmmac: refactor stmmac_init_rx_buffers for stmmac_reinit_rx_buffers")
> Signed-off-by: Yashwant Varur <yashwant.v@samsung.com>
> Signed-off-by: Jakub Raczynski <j.raczynski@samsung.com>
> Reviewed-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
> ---
>  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 2a0d7eff88d3..3098971e0b66 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -2172,8 +2172,8 @@ static void __free_dma_rx_desc_resources(struct stmmac_priv *priv,
>                 xdp_rxq_info_unreg(&rx_q->xdp_rxq);
>
>         kfree(rx_q->buf_pool);
> -       if (rx_q->page_pool)
> -               page_pool_destroy(rx_q->page_pool);
> +       page_pool_destroy(rx_q->page_pool);
> +       rx_q->page_pool = NULL;

I wonder if it's reasonable to add pool = NULL; to the end of
page_pool_destroy. I don't know that any caller wants to ever retain
the pool pointer after that function returns, and grepping this
function I see almost every call site nulling after destroy anyway.
But anyway:

Reviewed-by: Mina Almasry <almasrymina@google.com>

-- 
Thanks,
Mina

[PATCH net v3 2/2] net/stmmac: Prevent dma queue NULL free on allocation failure

[Jakub Raczynski]

During allocation of RX/TX descriptor resources and its DMA,
there is verification of failed dma_alloc_coherent() due to lack of memory.
In case of that failure, all allocated resources are freed instantly after,
but there are no checks for dma_free_coherent() whether previous step has
failed.
This will generally result in panic due to freeing NULL address.

Fix it by adding NULL verification of memory that is to be freed.
Also assign NULL to page pointers to avoid double free scenario.

Fixes: e73b19baa3b1c ("net: stmmac: simplify DMA descriptor allocation/init/freeing")
Reported-by: Sashiko AI <sashiko-bot@kernel.org>
Signed-off-by: Jakub Raczynski <j.raczynski@samsung.com>
---
 .../net/ethernet/stmicro/stmmac/stmmac_main.c | 28 +++++++++++++------
 1 file changed, 19 insertions(+), 9 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 3098971e0b66..77604d6ab466 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -2146,7 +2146,6 @@ static void __free_dma_rx_desc_resources(struct stmmac_priv *priv,
 					 u32 queue)
 {
 	struct stmmac_rx_queue *rx_q = &dma_conf->rx_queue[queue];
-	size_t size;
 	void *addr;
 
 	/* Release the DMA RX socket buffers */
@@ -2158,15 +2157,21 @@ static void __free_dma_rx_desc_resources(struct stmmac_priv *priv,
 	rx_q->buf_alloc_num = 0;
 	rx_q->xsk_pool = NULL;
 
-	/* Free DMA regions of consistent memory previously allocated */
-	if (priv->extend_desc)
+	if (priv->extend_desc) {
 		addr = rx_q->dma_erx;
-	else
+		rx_q->dma_erx = NULL;
+	} else {
 		addr = rx_q->dma_rx;
+		rx_q->dma_rx = NULL;
+	}
 
-	size = stmmac_get_rx_desc_size(priv) * dma_conf->dma_rx_size;
+	/* Free DMA regions of consistent memory if previously allocated */
+	if (addr) {
+		size_t size;
+		size = stmmac_get_rx_desc_size(priv) * dma_conf->dma_rx_size;
 
-	dma_free_coherent(priv->device, size, addr, rx_q->dma_rx_phy);
+		dma_free_coherent(priv->device, size, addr, rx_q->dma_rx_phy);
+	}
 
 	if (xdp_rxq_info_is_reg(&rx_q->xdp_rxq))
 		xdp_rxq_info_unreg(&rx_q->xdp_rxq);
@@ -2198,7 +2203,6 @@ static void __free_dma_tx_desc_resources(struct stmmac_priv *priv,
 					 u32 queue)
 {
 	struct stmmac_tx_queue *tx_q = &dma_conf->tx_queue[queue];
-	size_t size;
 	void *addr;
 
 	/* Release the DMA TX socket buffers */
@@ -2206,15 +2210,21 @@ static void __free_dma_tx_desc_resources(struct stmmac_priv *priv,
 
 	if (priv->extend_desc) {
 		addr = tx_q->dma_etx;
+		tx_q->dma_etx = NULL;
 	} else if (tx_q->tbs & STMMAC_TBS_AVAIL) {
 		addr = tx_q->dma_entx;
+		tx_q->dma_entx = NULL;
 	} else {
 		addr = tx_q->dma_tx;
+		tx_q->dma_tx = NULL;
 	}
 
-	size = stmmac_get_tx_desc_size(priv, tx_q) * dma_conf->dma_tx_size;
+	if (addr) {
+		size_t size;
+		size = stmmac_get_tx_desc_size(priv, tx_q) * dma_conf->dma_tx_size;
 
-	dma_free_coherent(priv->device, size, addr, tx_q->dma_tx_phy);
+		dma_free_coherent(priv->device, size, addr, tx_q->dma_tx_phy);
+	}
 
 	kfree(tx_q->tx_skbuff_dma);
 	kfree(tx_q->tx_skbuff);
-- 
2.34.1

Re: [PATCH net v3 0/2] net/stmmac: Secure against failures of DMA memory allocation

[Jakub Kicinski]

On Wed, 15 Jul 2026 14:36:00 +0200 Jakub Raczynski wrote:
> This series fixing two issues related to fails of
> __alloc_dma_rx_desc_resources(). Original issue from 1st patch is related to
> page_pool that has happened in testing env, while second was requested by
> Sashiko to have similar change for DMA allocation. 
> To have complete fix for all failures of __alloc_dma_rx_desc_resources(),
> merge two fixes into series.

Clashiko is not impressed by the second patch.
Is it possible to avoid calling the functions in semi-consistent state?
-- 
pw-bot: cr

Re: [PATCH net v3 0/2] net/stmmac: Secure against failures of DMA memory allocation

[Jakub Raczynski]

[-- Attachment #1: Type: text/plain, Size: 1759 bytes --]

On Thu, Jul 23, 2026 at 07:23:52AM -0700, Jakub Kicinski wrote:
> On Wed, 15 Jul 2026 14:36:00 +0200 Jakub Raczynski wrote:
> > This series fixing two issues related to fails of
> > __alloc_dma_rx_desc_resources(). Original issue from 1st patch is related to
> > page_pool that has happened in testing env, while second was requested by
> > Sashiko to have similar change for DMA allocation. 
> > To have complete fix for all failures of __alloc_dma_rx_desc_resources(),
> > merge two fixes into series.
> 
> Clashiko is not impressed by the second patch.
> Is it possible to avoid calling the functions in semi-consistent state?

Again clash against AI lost, damn you AI. Although I cannot say its wrong.
My bad I didn't really respond to it sooner, especially 13 character Fixes tag,
wonder how that slipped past internal review...

Now being serious, regarding calling in semi-consistent, it is matter of
symmetry between open/close or alloc/dealloc paths.
Since __alloc_dma_{tx/rx}_desc_resources does full initialization,
__free_dma_{tx/rx}_desc_resources should be able to handle whole cycle.
So if __alloc_ failed in the middle, __free_ should handle that state,
whatever it might be.

One thing I will say that AI review is not even about patches themselves,
but about
"If the intent is to make __free_dma_rx_desc_resources() safe to
run twice on the same queue, [...]",
which is the point, although original patch was generated by
real issue that occured. Other issues it reports are valid but did not
trigger.

So AI is right that everything should be handled in one patchset when
this is touched, but funnily it didn't report it previous review.
Will send another version that will fix all these issues/complains
at some point.

BR
Jakub Raczynski

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH net v3 0/2] net/stmmac: Secure against failures of DMA memory allocation

[Jakub Kicinski]

On Thu, 23 Jul 2026 17:24:58 +0200 Jakub Raczynski wrote:
> On Thu, Jul 23, 2026 at 07:23:52AM -0700, Jakub Kicinski wrote:
> > On Wed, 15 Jul 2026 14:36:00 +0200 Jakub Raczynski wrote:  
> > > This series fixing two issues related to fails of
> > > __alloc_dma_rx_desc_resources(). Original issue from 1st patch is related to
> > > page_pool that has happened in testing env, while second was requested by
> > > Sashiko to have similar change for DMA allocation. 
> > > To have complete fix for all failures of __alloc_dma_rx_desc_resources(),
> > > merge two fixes into series.  
> > 
> > Clashiko is not impressed by the second patch.
> > Is it possible to avoid calling the functions in semi-consistent state?  
> 
> Again clash against AI lost, damn you AI. Although I cannot say its wrong.
> My bad I didn't really respond to it sooner, especially 13 character Fixes tag,
> wonder how that slipped past internal review...
> 
> Now being serious, regarding calling in semi-consistent, it is matter of
> symmetry between open/close or alloc/dealloc paths.
> Since __alloc_dma_{tx/rx}_desc_resources does full initialization,
> __free_dma_{tx/rx}_desc_resources should be able to handle whole cycle.
> So if __alloc_ failed in the middle, __free_ should handle that state,
> whatever it might be.
> 
> One thing I will say that AI review is not even about patches themselves,
> but about
> "If the intent is to make __free_dma_rx_desc_resources() safe to
> run twice on the same queue, [...]",
> which is the point, although original patch was generated by
> real issue that occured. Other issues it reports are valid but did not
> trigger.
> 
> So AI is right that everything should be handled in one patchset when
> this is touched, but funnily it didn't report it previous review.
> Will send another version that will fix all these issues/complains
> at some point.

As you fix these issues it'd be great to step back and figure out what
model we want to follow. Personally I find the "idempotent cleanup"
to be inferior, it's better to know what state we're in. Failing that
a single indicator of state being initialized is usually fine. Having
field-by-field safeties is a recipe for 1000 fixes. IOW stmmac is
terribly architected, so we should figure out the end goal first, 
and target that, instead of addressing issues one by one.