[PATCH v2] nvdimm: ndtest: Return -ENOMEM if devm_kcalloc() fails in ndtest_probe()

[PATCH v2] nvdimm: ndtest: Return -ENOMEM if devm_kcalloc() fails in ndtest_probe()

[Guangshuo Li]

devm_kcalloc() may fail. ndtest_probe() allocates three DMA address
arrays (dcr_dma, label_dma, dimm_dma) and later unconditionally uses
them in ndtest_nvdimm_init(), which can lead to a NULL pointer
dereference under low-memory conditions.

Check all three allocations and return -ENOMEM if any allocation fails.
Do not emit an extra error message since the allocator already warns on
allocation failure.

Fixes: 9399ab61ad82 ("ndtest: Add dimms to the two buses")
Cc: stable@vger.kernel.org
Signed-off-by: Guangshuo Li <lgs201920130244@gmail.com>
---
Changes in v2:
- Drop pr_err() on allocation failure; only NULL-check and return -ENOMEM.
- No other changes.
---
 tools/testing/nvdimm/test/ndtest.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/tools/testing/nvdimm/test/ndtest.c b/tools/testing/nvdimm/test/ndtest.c
index 68a064ce598c..abdbe0c1cb63 100644
--- a/tools/testing/nvdimm/test/ndtest.c
+++ b/tools/testing/nvdimm/test/ndtest.c
@@ -855,6 +855,9 @@ static int ndtest_probe(struct platform_device *pdev)
 	p->dimm_dma = devm_kcalloc(&p->pdev.dev, NUM_DCR,
 				  sizeof(dma_addr_t), GFP_KERNEL);
 
+	if (!p->dcr_dma || !p->label_dma || !p->dimm_dma)
+		return -ENOMEM;
+
 	rc = ndtest_nvdimm_init(p);
 	if (rc)
 		goto err;
-- 
2.43.0

Re: [PATCH v2] nvdimm: ndtest: Return -ENOMEM if devm_kcalloc() fails in ndtest_probe()

[Dave Jiang]

On 9/23/25 5:59 AM, Guangshuo Li wrote:
> devm_kcalloc() may fail. ndtest_probe() allocates three DMA address
> arrays (dcr_dma, label_dma, dimm_dma) and later unconditionally uses
> them in ndtest_nvdimm_init(), which can lead to a NULL pointer
> dereference under low-memory conditions.
> 
> Check all three allocations and return -ENOMEM if any allocation fails.
> Do not emit an extra error message since the allocator already warns on
> allocation failure.
> 
> Fixes: 9399ab61ad82 ("ndtest: Add dimms to the two buses")
> Cc: stable@vger.kernel.org
> Signed-off-by: Guangshuo Li <lgs201920130244@gmail.com>
> ---
> Changes in v2:
> - Drop pr_err() on allocation failure; only NULL-check and return -ENOMEM.
> - No other changes.
> ---
>  tools/testing/nvdimm/test/ndtest.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/tools/testing/nvdimm/test/ndtest.c b/tools/testing/nvdimm/test/ndtest.c
> index 68a064ce598c..abdbe0c1cb63 100644
> --- a/tools/testing/nvdimm/test/ndtest.c
> +++ b/tools/testing/nvdimm/test/ndtest.c
> @@ -855,6 +855,9 @@ static int ndtest_probe(struct platform_device *pdev)
>  	p->dimm_dma = devm_kcalloc(&p->pdev.dev, NUM_DCR,
>  				  sizeof(dma_addr_t), GFP_KERNEL);
>  
> +	if (!p->dcr_dma || !p->label_dma || !p->dimm_dma)
> +		return -ENOMEM;

Why not just check as it allocates instead of doing it all at the end? If an allocation failed, no need to attempt to allocate more (which probably leads to more failures).

DJ

> +
>  	rc = ndtest_nvdimm_init(p);
>  	if (rc)
>  		goto err;

Re: [PATCH v2] nvdimm: ndtest: Return -ENOMEM if devm_kcalloc() fails in ndtest_probe()

[Alison Schofield]

On Tue, Sep 23, 2025 at 09:38:33AM -0700, Dave Jiang wrote:
> 
> 
> On 9/23/25 5:59 AM, Guangshuo Li wrote:
> > devm_kcalloc() may fail. ndtest_probe() allocates three DMA address
> > arrays (dcr_dma, label_dma, dimm_dma) and later unconditionally uses
> > them in ndtest_nvdimm_init(), which can lead to a NULL pointer
> > dereference under low-memory conditions.
> > 
> > Check all three allocations and return -ENOMEM if any allocation fails.
> > Do not emit an extra error message since the allocator already warns on
> > allocation failure.
> > 
> > Fixes: 9399ab61ad82 ("ndtest: Add dimms to the two buses")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Guangshuo Li <lgs201920130244@gmail.com>
> > ---
> > Changes in v2:
> > - Drop pr_err() on allocation failure; only NULL-check and return -ENOMEM.
> > - No other changes.
> > ---
> >  tools/testing/nvdimm/test/ndtest.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/tools/testing/nvdimm/test/ndtest.c b/tools/testing/nvdimm/test/ndtest.c
> > index 68a064ce598c..abdbe0c1cb63 100644
> > --- a/tools/testing/nvdimm/test/ndtest.c
> > +++ b/tools/testing/nvdimm/test/ndtest.c
> > @@ -855,6 +855,9 @@ static int ndtest_probe(struct platform_device *pdev)
> >  	p->dimm_dma = devm_kcalloc(&p->pdev.dev, NUM_DCR,
> >  				  sizeof(dma_addr_t), GFP_KERNEL);
> >  
> > +	if (!p->dcr_dma || !p->label_dma || !p->dimm_dma)
> > +		return -ENOMEM;
> 
> Why not just check as it allocates instead of doing it all at the end? If an allocation failed, no need to attempt to allocate more (which probably leads to more failures).

Following on Dave's feedback and looking at the function as a whole,
it does have a pattern that this patch can replicate:

It does this now:
	rc = do_something();
	if (rc)
		goto err;

So, continue that pattern with the added NULL checks:

	p->dcr_dma = devm_kcalloc(&p->pdev.dev, NUM_DCR,
				  sizeof(dma_addr_t), GFP_KERNEL);
	if (!p->dcr_dma) {
		rc = -ENOMEM;
		goto err;
	}
and repeat above for all 3 allocs.

And, maybe even change that first ndtest_bus_register() failure
to follow the same pattern.

--Alison

> 
> DJ
> 
> > +
> >  	rc = ndtest_nvdimm_init(p);
> >  	if (rc)
> >  		goto err;
> 
> 

Re: [PATCH v2] nvdimm: ndtest: Return -ENOMEM if devm_kcalloc() fails in ndtest_probe()

[Dave Jiang]

On 9/24/25 12:42 AM, Guangshuo Li wrote:
> Hi Alison, Dave, and all,
> 
> Thanks for the feedback. I’ve adopted your suggestions. Below is what I plan to take in v3.
> 
> -       p->dcr_dma = devm_kcalloc(&p->pdev.dev <http://pdev.dev>, NUM_DCR,
> -                                 sizeof(dma_addr_t), GFP_KERNEL);
> -       p->label_dma = devm_kcalloc(&p->pdev.dev <http://pdev.dev>, NUM_DCR,
> -                                   sizeof(dma_addr_t), GFP_KERNEL);
> -       p->dimm_dma = devm_kcalloc(&p->pdev.dev <http://pdev.dev>, NUM_DCR,
> -                                  sizeof(dma_addr_t), GFP_KERNEL);
> 
> +       p->dcr_dma = devm_kcalloc(&p->pdev.dev <http://pdev.dev>, NUM_DCR,
> +                                 sizeof(dma_addr_t), GFP_KERNEL);
> +       if (!p->dcr_dma) {
> +               rc = -ENOMEM;
> +               goto err;
> +       }
> +
> +       p->label_dma = devm_kcalloc(&p->pdev.dev <http://pdev.dev>, NUM_DCR,
> +                                   sizeof(dma_addr_t), GFP_KERNEL);
> +       if (!p->label_dma) {
> +               rc = -ENOMEM;
> +               goto err;
> +       }
> +
> +       p->dimm_dma = devm_kcalloc(&p->pdev.dev <http://pdev.dev>, NUM_DCR,
> +                                  sizeof(dma_addr_t), GFP_KERNEL);
> +       if (!p->dimm_dma) {
> +               rc = -ENOMEM;
> +               goto err;
> +       }

You'll need to create new goto labels because you'll have to free previously allocated memory in the error path. Diff below is uncompiled and untested.

diff --git a/tools/testing/nvdimm/test/ndtest.c b/tools/testing/nvdimm/test/ndtest.c
index 68a064ce598c..49d326819ea9 100644
--- a/tools/testing/nvdimm/test/ndtest.c
+++ b/tools/testing/nvdimm/test/ndtest.c
@@ -841,6 +841,7 @@ static void ndtest_remove(struct platform_device *pdev)

 static int ndtest_probe(struct platform_device *pdev)
 {
+       struct device *dev = &pdev->dev;
        struct ndtest_priv *p;
        int rc;

@@ -848,12 +849,23 @@ static int ndtest_probe(struct platform_device *pdev)
        if (ndtest_bus_register(p))
                return -ENOMEM;

-       p->dcr_dma = devm_kcalloc(&p->pdev.dev, NUM_DCR,
-                                sizeof(dma_addr_t), GFP_KERNEL);
-       p->label_dma = devm_kcalloc(&p->pdev.dev, NUM_DCR,
-                                  sizeof(dma_addr_t), GFP_KERNEL);
-       p->dimm_dma = devm_kcalloc(&p->pdev.dev, NUM_DCR,
-                                 sizeof(dma_addr_t), GFP_KERNEL);
+       p->dcr_dma = devm_kcalloc(dev, NUM_DCR, sizeof(dma_addr_t), GFP_KERNEL);
+       if (!p->dcr_dma)
+               return -ENOMEM;
+
+       p->label_dma = devm_kcalloc(dev, NUM_DCR, sizeof(dma_addr_t),
+                                   GFP_KERNEL);
+       if (!p->label_dma) {
+               rc = -ENOMEM;
+               goto err_label_dma;
+       }
+
+       p->dimm_dma = devm_kcalloc(dev, NUM_DCR, sizeof(dma_addr_t),
+                                  GFP_KERNEL);
+       if (!p->dimm_dma) {
+               rc = -ENOMEM;
+               goto err_dimm_dma;
+       }

        rc = ndtest_nvdimm_init(p);
        if (rc)
@@ -863,7 +875,7 @@ static int ndtest_probe(struct platform_device *pdev)
        if (rc)
                goto err;

-       rc = devm_add_action_or_reset(&pdev->dev, put_dimms, p);
+       rc = devm_add_action_or_reset(dev, put_dimms, p);
        if (rc)
                goto err;
@@ -872,6 +884,11 @@ static int ndtest_probe(struct platform_device *pdev)
        return 0;

 err:
+       devm_kfree(dev, p->dimm_dma);
+err_dimm_dma:
+       devm_kfree(dev, p->label_dma);
+err_label_dma:
+       devm_kfree(dev, p->dcr_dma);
        pr_err("%s:%d Failed nvdimm init\n", __func__, __LINE__);
        return rc;
 }

> 
> If this looks good, I’ll send v3 accordingly. Also, if you’re comfortable with the changes, may I add your Reviewed-by tags?

Please don't add review tags until they are given.

> 
> Best regards,
> Guangshuo

Re: [PATCH v2] nvdimm: ndtest: Return -ENOMEM if devm_kcalloc() fails in ndtest_probe()

[Ira Weiny]

Guangshuo Li wrote:
> Hi Alison, Dave, and all,
> 
> Thanks for the feedback. I’ve adopted your suggestions. Below is what I
> plan to take in v3.

I would just post v3.  The review tags given on that version will be
picked up when the patch is merged if it is ok.

Thanks,
Ira

[snip]