Re: [PATCH v4 01/12] nvdimm/pmem: Fix leak on dax_add_host() failure
On 2024-02-08 17:12, Andrew Morton wrote: On Thu, 8 Feb 2024 17:04:52 -0500 Mathieu Desnoyers wrote: [...] Should I keep this patch 01/12 within the series for v5 or should I send it separately ? Doesn't matter much, but perfectionism does say "standalone patch please". Will do. I plan to add the following statement to the commit message to make it clear that there is a dependency between the patch series and this fix: [ Based on commit "nvdimm/pmem: Fix leak on dax_add_host() failure". ] Thanks, Mathieu -- Mathieu Desnoyers EfficiOS Inc. https://www.efficios.com
Re: [PATCH v4 01/12] nvdimm/pmem: Fix leak on dax_add_host() failure
On Thu, 8 Feb 2024 17:04:52 -0500 Mathieu Desnoyers wrote: > > The series seems useful but is at v4 without much sign of review > > activity. I think I'll take silence as assent and shall slam it all > > into -next and see who shouts at me. > > > > Thanks Andrew for picking it up! Dan just reacted with feedback that > will help reducing the patch series size by removing intermediate > commits. I'll implement the requested changes and post a v5 in a few > days. Yup. I'll leave v4 out there for testers to bet on. > So far there are not behavior changes requested in Dan's feedback. > > Should I keep this patch 01/12 within the series for v5 or should I > send it separately ? Doesn't matter much, but perfectionism does say "standalone patch please".
Re: [PATCH v4 01/12] nvdimm/pmem: Fix leak on dax_add_host() failure
On 2024-02-08 16:21, Andrew Morton wrote: On Thu, 8 Feb 2024 13:49:02 -0500 Mathieu Desnoyers wrote: Fix a leak on dax_add_host() error, where "goto out_cleanup_dax" is done before setting pmem->dax_dev, which therefore issues the two following calls on NULL pointers: out_cleanup_dax: kill_dax(pmem->dax_dev); put_dax(pmem->dax_dev); Seems inappropriate that this fix is within this patch series? otoh I assume dax_add_host() has never failed so it doesn't matter much. The series seems useful but is at v4 without much sign of review activity. I think I'll take silence as assent and shall slam it all into -next and see who shouts at me. Thanks Andrew for picking it up! Dan just reacted with feedback that will help reducing the patch series size by removing intermediate commits. I'll implement the requested changes and post a v5 in a few days. So far there are not behavior changes requested in Dan's feedback. Should I keep this patch 01/12 within the series for v5 or should I send it separately ? Thanks, Mathieu -- Mathieu Desnoyers EfficiOS Inc. https://www.efficios.com
RE: [PATCH v4 01/12] nvdimm/pmem: Fix leak on dax_add_host() failure
Mathieu Desnoyers wrote: > Fix a leak on dax_add_host() error, where "goto out_cleanup_dax" is done > before setting pmem->dax_dev, which therefore issues the two following > calls on NULL pointers: > > out_cleanup_dax: > kill_dax(pmem->dax_dev); > put_dax(pmem->dax_dev); > > Signed-off-by: Mathieu Desnoyers Looks good to me. Reviewed-by: Dan Williams
Re: [PATCH v4 01/12] nvdimm/pmem: Fix leak on dax_add_host() failure
On Thu, 8 Feb 2024 13:49:02 -0500 Mathieu Desnoyers wrote: > Fix a leak on dax_add_host() error, where "goto out_cleanup_dax" is done > before setting pmem->dax_dev, which therefore issues the two following > calls on NULL pointers: > > out_cleanup_dax: > kill_dax(pmem->dax_dev); > put_dax(pmem->dax_dev); Seems inappropriate that this fix is within this patch series? otoh I assume dax_add_host() has never failed so it doesn't matter much. The series seems useful but is at v4 without much sign of review activity. I think I'll take silence as assent and shall slam it all into -next and see who shouts at me.
[PATCH v4 01/12] nvdimm/pmem: Fix leak on dax_add_host() failure
Fix a leak on dax_add_host() error, where "goto out_cleanup_dax" is done before setting pmem->dax_dev, which therefore issues the two following calls on NULL pointers: out_cleanup_dax: kill_dax(pmem->dax_dev); put_dax(pmem->dax_dev); Signed-off-by: Mathieu Desnoyers Cc: Alasdair Kergon Cc: Mike Snitzer Cc: Mikulas Patocka Cc: Andrew Morton Cc: Linus Torvalds Cc: Dan Williams Cc: Vishal Verma Cc: Dave Jiang Cc: Matthew Wilcox Cc: Arnd Bergmann Cc: Russell King Cc: linux-a...@vger.kernel.org Cc: linux-...@vger.kernel.org Cc: linux-fsde...@vger.kernel.org Cc: linux...@kvack.org Cc: linux-...@vger.kernel.org Cc: dm-devel@lists.linux.dev Cc: nvd...@lists.linux.dev --- drivers/nvdimm/pmem.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/drivers/nvdimm/pmem.c b/drivers/nvdimm/pmem.c index 4e8fdcb3f1c8..9fe358090720 100644 --- a/drivers/nvdimm/pmem.c +++ b/drivers/nvdimm/pmem.c @@ -566,12 +566,11 @@ static int pmem_attach_disk(struct device *dev, set_dax_nomc(dax_dev); if (is_nvdimm_sync(nd_region)) set_dax_synchronous(dax_dev); + pmem->dax_dev = dax_dev; rc = dax_add_host(dax_dev, disk); if (rc) goto out_cleanup_dax; dax_write_cache(dax_dev, nvdimm_has_cache(nd_region)); - pmem->dax_dev = dax_dev; - rc = device_add_disk(dev, disk, pmem_attribute_groups); if (rc) goto out_remove_host; -- 2.39.2
[RFC PATCH v4 01/12] nvdimm/pmem: Fix leak on dax_add_host() failure
Fix a leak on dax_add_host() error, where "goto out_cleanup_dax" is done before setting pmem->dax_dev, which therefore issues the two following calls on NULL pointers: out_cleanup_dax: kill_dax(pmem->dax_dev); put_dax(pmem->dax_dev); Signed-off-by: Mathieu Desnoyers Cc: Alasdair Kergon Cc: Mike Snitzer Cc: Mikulas Patocka Cc: Andrew Morton Cc: Linus Torvalds Cc: Dan Williams Cc: Vishal Verma Cc: Dave Jiang Cc: Matthew Wilcox Cc: Arnd Bergmann Cc: Russell King Cc: linux-a...@vger.kernel.org Cc: linux-...@vger.kernel.org Cc: linux-fsde...@vger.kernel.org Cc: linux...@kvack.org Cc: linux-...@vger.kernel.org Cc: dm-devel@lists.linux.dev Cc: nvd...@lists.linux.dev --- drivers/nvdimm/pmem.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/drivers/nvdimm/pmem.c b/drivers/nvdimm/pmem.c index 4e8fdcb3f1c8..9fe358090720 100644 --- a/drivers/nvdimm/pmem.c +++ b/drivers/nvdimm/pmem.c @@ -566,12 +566,11 @@ static int pmem_attach_disk(struct device *dev, set_dax_nomc(dax_dev); if (is_nvdimm_sync(nd_region)) set_dax_synchronous(dax_dev); + pmem->dax_dev = dax_dev; rc = dax_add_host(dax_dev, disk); if (rc) goto out_cleanup_dax; dax_write_cache(dax_dev, nvdimm_has_cache(nd_region)); - pmem->dax_dev = dax_dev; - rc = device_add_disk(dev, disk, pmem_attribute_groups); if (rc) goto out_remove_host; -- 2.39.2
