Re: [PATCH v2] nvdimm/pmem: Release gendisk on probe failure

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] NULL pointer dereference in sysfs attributes (write_cache) when alloc_dax() fails with -EOPNOTSUPP
--

commit b72cbfedec624805721d1461cccfb6dbba985b5b
Author: Hemanth Selam <hemanth.selam@gmail.com>

nvdimm/pmem: Release gendisk on probe failure

This commit fixes a memory leak in the pmem namespace probe by ensuring
the gendisk is properly released through the common cleanup path if
devm_init_badblocks() fails.

> diff --git a/drivers/nvdimm/pmem.c b/drivers/nvdimm/pmem.c
> index 30a51c365ce8b..648fc7d660639 100644
> --- a/drivers/nvdimm/pmem.c
> +++ b/drivers/nvdimm/pmem.c
> @@ -563,8 +563,10 @@ static int pmem_attach_disk(struct device *dev,
>  	nvdimm_namespace_disk_name(ndns, disk->disk_name);
>  	set_capacity(disk, (pmem->size - pmem->pfn_pad - pmem->data_offset)
>  			/ 512);
> -	if (devm_init_badblocks(dev, &pmem->bb))
> -		return -ENOMEM;
> +	if (devm_init_badblocks(dev, &pmem->bb)) {
> +		rc = -ENOMEM;
> +		goto out;
> +	}
>  	nvdimm_badblocks_populate(nd_region, &pmem->bb, &bb_range);
>  	disk->bb = &pmem->bb;

[Severity: High]
This isn't a bug introduced by this patch, but I wanted to ask about the error
handling for alloc_dax() just below this code.

In pmem_attach_disk(), if alloc_dax() fails with -EOPNOTSUPP, the code
gracefully falls back and leaves pmem->dax_dev as NULL:

	dax_dev = alloc_dax(pmem, &pmem_dax_ops);
	if (IS_ERR(dax_dev)) {
		rc = PTR_ERR(dax_dev);
		if (rc != -EOPNOTSUPP)
			goto out;
	} else {
		...
	}
	rc = device_add_disk(dev, disk, pmem_attribute_groups);

However, the write_cache sysfs attribute remains unconditionally visible as long
as CONFIG_ARCH_HAS_PMEM_API is enabled.

If a user reads this attribute, write_cache_show() will pass the NULL
pmem->dax_dev to dax_write_cache_enabled():

static ssize_t write_cache_show(struct device *dev,
		struct device_attribute *attr, char *buf)
{
	struct pmem_device *pmem = dev_to_disk(dev)->private_data;

	return sprintf(buf, "%d\n", !!dax_write_cache_enabled(pmem->dax_dev));
}

Similarly, write_cache_store() passes it to dax_write_cache():

static ssize_t write_cache_store(struct device *dev,
		struct device_attribute *attr, const char *buf, size_t len)
{
	...
	dax_write_cache(pmem->dax_dev, write_cache);
	return len;
}

Since dax_write_cache_enabled() (in drivers/dax/super.c) and dax_write_cache()
blindly dereference the pointer, won't this cause a NULL pointer dereference
leading to a kernel panic if the attribute is accessed?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260831043459.1298059-1-hemanth.selam@gmail.com?part=1