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 <[email protected]> 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/[email protected]?part=1
