libnvdimm: call devm_namespace_disable() on error
diff mbox series

Message ID 20200703111856.40280-1-hare@suse.de
State New
Headers show
Series
  • libnvdimm: call devm_namespace_disable() on error
Related show

Commit Message

Hannes Reinecke July 3, 2020, 11:18 a.m. UTC
Once devm_namespace_enable() has been called the error path in the
calling function will not call devm_namespace_disable(), leaving the
namespace enabled on error.

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/dax/pmem/core.c   |  2 +-
 drivers/nvdimm/btt.c      |  5 ++++-
 drivers/nvdimm/claim.c    |  8 +++++++-
 drivers/nvdimm/pfn_devs.c |  1 +
 drivers/nvdimm/pmem.c     | 20 ++++++++++----------
 5 files changed, 23 insertions(+), 13 deletions(-)

Comments

Santosh Sivaraj July 9, 2020, 5:39 a.m. UTC | #1
Hi Hannes,

Hannes Reinecke <hare@suse.de> writes:

> Once devm_namespace_enable() has been called the error path in the
> calling function will not call devm_namespace_disable(), leaving the
> namespace enabled on error.
>
> Signed-off-by: Hannes Reinecke <hare@suse.de>
> ---
>  drivers/dax/pmem/core.c   |  2 +-
>  drivers/nvdimm/btt.c      |  5 ++++-
>  drivers/nvdimm/claim.c    |  8 +++++++-
>  drivers/nvdimm/pfn_devs.c |  1 +
>  drivers/nvdimm/pmem.c     | 20 ++++++++++----------
>  5 files changed, 23 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/dax/pmem/core.c b/drivers/dax/pmem/core.c
> index 2bedf8414fff..4b26434f0aca 100644
> --- a/drivers/dax/pmem/core.c
> +++ b/drivers/dax/pmem/core.c
> @@ -31,9 +31,9 @@ struct dev_dax *__dax_pmem_probe(struct device *dev, enum dev_dax_subsys subsys)
>  	if (rc)
>  		return ERR_PTR(rc);
>  	rc = nvdimm_setup_pfn(nd_pfn, &pgmap);
> +	devm_namespace_disable(dev, ndns);
>  	if (rc)
>  		return ERR_PTR(rc);
> -	devm_namespace_disable(dev, ndns);
>  
>  	/* reserve the metadata area, device-dax will reserve the data */
>  	pfn_sb = nd_pfn->pfn_sb;
> diff --git a/drivers/nvdimm/btt.c b/drivers/nvdimm/btt.c
> index 48e9d169b6f9..bd4747f2c99b 100644
> --- a/drivers/nvdimm/btt.c
> +++ b/drivers/nvdimm/btt.c
> @@ -1704,13 +1704,16 @@ int nvdimm_namespace_attach_btt(struct nd_namespace_common *ndns)
>  		dev_dbg(&nd_btt->dev, "%s must be at least %ld bytes\n",
>  				dev_name(&ndns->dev),
>  				ARENA_MIN_SIZE + nd_btt->initial_offset);
> +		devm_namespace_disable(&nd_btt->dev, ndns);
>  		return -ENXIO;
>  	}
>  	nd_region = to_nd_region(nd_btt->dev.parent);
>  	btt = btt_init(nd_btt, rawsize, nd_btt->lbasize, nd_btt->uuid,
>  			nd_region);
> -	if (!btt)
> +	if (!btt) {
> +		devm_namespace_disable(&nd_btt->dev, ndns);
>  		return -ENOMEM;
> +	}
>  	nd_btt->btt = btt;
>  
>  	return 0;
> diff --git a/drivers/nvdimm/claim.c b/drivers/nvdimm/claim.c
> index 45964acba944..15fd1b92d32f 100644
> --- a/drivers/nvdimm/claim.c
> +++ b/drivers/nvdimm/claim.c
> @@ -314,12 +314,18 @@ int devm_nsio_enable(struct device *dev, struct nd_namespace_io *nsio,
>  	}
>  
>  	ndns->rw_bytes = nsio_rw_bytes;
> -	if (devm_init_badblocks(dev, &nsio->bb))
> +	if (devm_init_badblocks(dev, &nsio->bb)) {
> +		devm_release_mem_region(dev, res->start, size);
>  		return -ENOMEM;
> +	}
>  	nvdimm_badblocks_populate(to_nd_region(ndns->dev.parent), &nsio->bb,
>  			&nsio->res);
>  
>  	nsio->addr = devm_memremap(dev, res->start, size, ARCH_MEMREMAP_PMEM);
> +	if (IS_ERR(nsio->addr)) {
> +		devm_exit_badblocks(dev, &nsio->bb);
> +		devm_release_mem_region(dev, res->start, size);
> +	}
>  
>  	return PTR_ERR_OR_ZERO(nsio->addr);
>  }
> diff --git a/drivers/nvdimm/pfn_devs.c b/drivers/nvdimm/pfn_devs.c
> index 34db557dbad1..9faa92662643 100644
> --- a/drivers/nvdimm/pfn_devs.c
> +++ b/drivers/nvdimm/pfn_devs.c
> @@ -408,6 +408,7 @@ static int nd_pfn_clear_memmap_errors(struct nd_pfn *nd_pfn)
>  				nsoff += chunk;
>  			}
>  			if (rc) {
> +				devm_namespace_disable(&nd_pfn->dev, ndns);

The disable here seems to be wrong.

This function called from the pmem_attach_disk path, where its expected the
namespace is enabled after the setup_pfn. Also in case of an error, we do
another disable in pmem_attach_disk.
Thanks,
Santosh

>  				dev_err(&nd_pfn->dev,
>  					"error clearing %x badblocks at %llx\n",
>  					num_bad, first_bad);
> diff --git a/drivers/nvdimm/pmem.c b/drivers/nvdimm/pmem.c
> index d25e66fd942d..4f667fe6ef72 100644
> --- a/drivers/nvdimm/pmem.c
> +++ b/drivers/nvdimm/pmem.c
> @@ -401,8 +401,10 @@ static int pmem_attach_disk(struct device *dev,
>  	if (is_nd_pfn(dev)) {
>  		nd_pfn = to_nd_pfn(dev);
>  		rc = nvdimm_setup_pfn(nd_pfn, &pmem->pgmap);
> -		if (rc)
> +		if (rc) {
> +			devm_namespace_disable(dev, ndns);
>  			return rc;
> +		}
>  	}
>  
>  	/* we're attaching a block device, disable raw namespace access */
> @@ -549,17 +551,15 @@ static int nd_pmem_probe(struct device *dev)
>  	ret = nd_pfn_probe(dev, ndns);
>  	if (ret == 0)
>  		return -ENXIO;
> -	else if (ret == -EOPNOTSUPP)
> -		return ret;
> -
> -	ret = nd_dax_probe(dev, ndns);
> -	if (ret == 0)
> -		return -ENXIO;
> -	else if (ret == -EOPNOTSUPP)
> -		return ret;
> -
> +	else if (ret != EOPNOTSUPP) {
> +		ret = nd_dax_probe(dev, ndns);
> +		if (ret == 0)
> +			return -ENXIO;
> +	}
>  	/* probe complete, attach handles namespace enabling */
>  	devm_namespace_disable(dev, ndns);
> +	if (ret == -EOPNOTSUPP)
> +		return ret;
>  
>  	return pmem_attach_disk(dev, ndns);
>  }
> -- 
> 2.16.4
> _______________________________________________
> Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
> To unsubscribe send an email to linux-nvdimm-leave@lists.01.org
Santosh Sivaraj July 9, 2020, 5:57 a.m. UTC | #2
Hi! Hannes,

Hannes Reinecke <hare@suse.de> writes:

> Once devm_namespace_enable() has been called the error path in the
> calling function will not call devm_namespace_disable(), leaving the
> namespace enabled on error.
>
> Signed-off-by: Hannes Reinecke <hare@suse.de>
> ---
>  drivers/dax/pmem/core.c   |  2 +-
>  drivers/nvdimm/btt.c      |  5 ++++-
>  drivers/nvdimm/claim.c    |  8 +++++++-
>  drivers/nvdimm/pfn_devs.c |  1 +
>  drivers/nvdimm/pmem.c     | 20 ++++++++++----------
>  5 files changed, 23 insertions(+), 13 deletions(-)
>

[snip]

> diff --git a/drivers/nvdimm/pmem.c b/drivers/nvdimm/pmem.c
> index d25e66fd942d..4f667fe6ef72 100644
> --- a/drivers/nvdimm/pmem.c
> +++ b/drivers/nvdimm/pmem.c
> @@ -401,8 +401,10 @@ static int pmem_attach_disk(struct device *dev,
>  	if (is_nd_pfn(dev)) {
>  		nd_pfn = to_nd_pfn(dev);
>  		rc = nvdimm_setup_pfn(nd_pfn, &pmem->pgmap);
> -		if (rc)
> +		if (rc) {
> +			devm_namespace_disable(dev, ndns);
>  			return rc;
> +		}

As mentioned in the previous mail, when rc != 0, disable is called again
here.

>  	}
>  
>  	/* we're attaching a block device, disable raw namespace access */
> @@ -549,17 +551,15 @@ static int nd_pmem_probe(struct device *dev)
>  	ret = nd_pfn_probe(dev, ndns);
>  	if (ret == 0)
>  		return -ENXIO;
> -	else if (ret == -EOPNOTSUPP)
> -		return ret;
> -
> -	ret = nd_dax_probe(dev, ndns);
> -	if (ret == 0)
> -		return -ENXIO;
> -	else if (ret == -EOPNOTSUPP)
> -		return ret;
> -
> +	else if (ret != EOPNOTSUPP) {
                        ^ -EOPNOTSUPP

Thanks,
Santosh
> +		ret = nd_dax_probe(dev, ndns);
> +		if (ret == 0)
> +			return -ENXIO;
> +	}
>  	/* probe complete, attach handles namespace enabling */
>  	devm_namespace_disable(dev, ndns);
> +	if (ret == -EOPNOTSUPP)
> +		return ret;
>  
>  	return pmem_attach_disk(dev, ndns);
>  }
> -- 
> 2.16.4
> _______________________________________________
> Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
> To unsubscribe send an email to linux-nvdimm-leave@lists.01.org
Hannes Reinecke July 13, 2020, 2:35 p.m. UTC | #3
On 7/9/20 7:39 AM, Santosh Sivaraj wrote:
> Hi Hannes,
> 
> Hannes Reinecke <hare@suse.de> writes:
> 
>> Once devm_namespace_enable() has been called the error path in the
>> calling function will not call devm_namespace_disable(), leaving the
>> namespace enabled on error.
>>
>> Signed-off-by: Hannes Reinecke <hare@suse.de>
>> ---
>>   drivers/dax/pmem/core.c   |  2 +-
>>   drivers/nvdimm/btt.c      |  5 ++++-
>>   drivers/nvdimm/claim.c    |  8 +++++++-
>>   drivers/nvdimm/pfn_devs.c |  1 +
>>   drivers/nvdimm/pmem.c     | 20 ++++++++++----------
>>   5 files changed, 23 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/dax/pmem/core.c b/drivers/dax/pmem/core.c
>> index 2bedf8414fff..4b26434f0aca 100644
>> --- a/drivers/dax/pmem/core.c
>> +++ b/drivers/dax/pmem/core.c
>> @@ -31,9 +31,9 @@ struct dev_dax *__dax_pmem_probe(struct device *dev, enum dev_dax_subsys subsys)
>>   	if (rc)
>>   		return ERR_PTR(rc);
>>   	rc = nvdimm_setup_pfn(nd_pfn, &pgmap);
>> +	devm_namespace_disable(dev, ndns);
>>   	if (rc)
>>   		return ERR_PTR(rc);
>> -	devm_namespace_disable(dev, ndns);
>>   
>>   	/* reserve the metadata area, device-dax will reserve the data */
>>   	pfn_sb = nd_pfn->pfn_sb;
>> diff --git a/drivers/nvdimm/btt.c b/drivers/nvdimm/btt.c
>> index 48e9d169b6f9..bd4747f2c99b 100644
>> --- a/drivers/nvdimm/btt.c
>> +++ b/drivers/nvdimm/btt.c
>> @@ -1704,13 +1704,16 @@ int nvdimm_namespace_attach_btt(struct nd_namespace_common *ndns)
>>   		dev_dbg(&nd_btt->dev, "%s must be at least %ld bytes\n",
>>   				dev_name(&ndns->dev),
>>   				ARENA_MIN_SIZE + nd_btt->initial_offset);
>> +		devm_namespace_disable(&nd_btt->dev, ndns);
>>   		return -ENXIO;
>>   	}
>>   	nd_region = to_nd_region(nd_btt->dev.parent);
>>   	btt = btt_init(nd_btt, rawsize, nd_btt->lbasize, nd_btt->uuid,
>>   			nd_region);
>> -	if (!btt)
>> +	if (!btt) {
>> +		devm_namespace_disable(&nd_btt->dev, ndns);
>>   		return -ENOMEM;
>> +	}
>>   	nd_btt->btt = btt;
>>   
>>   	return 0;
>> diff --git a/drivers/nvdimm/claim.c b/drivers/nvdimm/claim.c
>> index 45964acba944..15fd1b92d32f 100644
>> --- a/drivers/nvdimm/claim.c
>> +++ b/drivers/nvdimm/claim.c
>> @@ -314,12 +314,18 @@ int devm_nsio_enable(struct device *dev, struct nd_namespace_io *nsio,
>>   	}
>>   
>>   	ndns->rw_bytes = nsio_rw_bytes;
>> -	if (devm_init_badblocks(dev, &nsio->bb))
>> +	if (devm_init_badblocks(dev, &nsio->bb)) {
>> +		devm_release_mem_region(dev, res->start, size);
>>   		return -ENOMEM;
>> +	}
>>   	nvdimm_badblocks_populate(to_nd_region(ndns->dev.parent), &nsio->bb,
>>   			&nsio->res);
>>   
>>   	nsio->addr = devm_memremap(dev, res->start, size, ARCH_MEMREMAP_PMEM);
>> +	if (IS_ERR(nsio->addr)) {
>> +		devm_exit_badblocks(dev, &nsio->bb);
>> +		devm_release_mem_region(dev, res->start, size);
>> +	}
>>   
>>   	return PTR_ERR_OR_ZERO(nsio->addr);
>>   }
>> diff --git a/drivers/nvdimm/pfn_devs.c b/drivers/nvdimm/pfn_devs.c
>> index 34db557dbad1..9faa92662643 100644
>> --- a/drivers/nvdimm/pfn_devs.c
>> +++ b/drivers/nvdimm/pfn_devs.c
>> @@ -408,6 +408,7 @@ static int nd_pfn_clear_memmap_errors(struct nd_pfn *nd_pfn)
>>   				nsoff += chunk;
>>   			}
>>   			if (rc) {
>> +				devm_namespace_disable(&nd_pfn->dev, ndns);
> 
> The disable here seems to be wrong.
> 
> This function called from the pmem_attach_disk path, where its expected the
> namespace is enabled after the setup_pfn. Also in case of an error, we do
> another disable in pmem_attach_disk.

Oh. Correct. Will be fixing it up.

Cheers,

Hannes

Patch
diff mbox series

diff --git a/drivers/dax/pmem/core.c b/drivers/dax/pmem/core.c
index 2bedf8414fff..4b26434f0aca 100644
--- a/drivers/dax/pmem/core.c
+++ b/drivers/dax/pmem/core.c
@@ -31,9 +31,9 @@  struct dev_dax *__dax_pmem_probe(struct device *dev, enum dev_dax_subsys subsys)
 	if (rc)
 		return ERR_PTR(rc);
 	rc = nvdimm_setup_pfn(nd_pfn, &pgmap);
+	devm_namespace_disable(dev, ndns);
 	if (rc)
 		return ERR_PTR(rc);
-	devm_namespace_disable(dev, ndns);
 
 	/* reserve the metadata area, device-dax will reserve the data */
 	pfn_sb = nd_pfn->pfn_sb;
diff --git a/drivers/nvdimm/btt.c b/drivers/nvdimm/btt.c
index 48e9d169b6f9..bd4747f2c99b 100644
--- a/drivers/nvdimm/btt.c
+++ b/drivers/nvdimm/btt.c
@@ -1704,13 +1704,16 @@  int nvdimm_namespace_attach_btt(struct nd_namespace_common *ndns)
 		dev_dbg(&nd_btt->dev, "%s must be at least %ld bytes\n",
 				dev_name(&ndns->dev),
 				ARENA_MIN_SIZE + nd_btt->initial_offset);
+		devm_namespace_disable(&nd_btt->dev, ndns);
 		return -ENXIO;
 	}
 	nd_region = to_nd_region(nd_btt->dev.parent);
 	btt = btt_init(nd_btt, rawsize, nd_btt->lbasize, nd_btt->uuid,
 			nd_region);
-	if (!btt)
+	if (!btt) {
+		devm_namespace_disable(&nd_btt->dev, ndns);
 		return -ENOMEM;
+	}
 	nd_btt->btt = btt;
 
 	return 0;
diff --git a/drivers/nvdimm/claim.c b/drivers/nvdimm/claim.c
index 45964acba944..15fd1b92d32f 100644
--- a/drivers/nvdimm/claim.c
+++ b/drivers/nvdimm/claim.c
@@ -314,12 +314,18 @@  int devm_nsio_enable(struct device *dev, struct nd_namespace_io *nsio,
 	}
 
 	ndns->rw_bytes = nsio_rw_bytes;
-	if (devm_init_badblocks(dev, &nsio->bb))
+	if (devm_init_badblocks(dev, &nsio->bb)) {
+		devm_release_mem_region(dev, res->start, size);
 		return -ENOMEM;
+	}
 	nvdimm_badblocks_populate(to_nd_region(ndns->dev.parent), &nsio->bb,
 			&nsio->res);
 
 	nsio->addr = devm_memremap(dev, res->start, size, ARCH_MEMREMAP_PMEM);
+	if (IS_ERR(nsio->addr)) {
+		devm_exit_badblocks(dev, &nsio->bb);
+		devm_release_mem_region(dev, res->start, size);
+	}
 
 	return PTR_ERR_OR_ZERO(nsio->addr);
 }
diff --git a/drivers/nvdimm/pfn_devs.c b/drivers/nvdimm/pfn_devs.c
index 34db557dbad1..9faa92662643 100644
--- a/drivers/nvdimm/pfn_devs.c
+++ b/drivers/nvdimm/pfn_devs.c
@@ -408,6 +408,7 @@  static int nd_pfn_clear_memmap_errors(struct nd_pfn *nd_pfn)
 				nsoff += chunk;
 			}
 			if (rc) {
+				devm_namespace_disable(&nd_pfn->dev, ndns);
 				dev_err(&nd_pfn->dev,
 					"error clearing %x badblocks at %llx\n",
 					num_bad, first_bad);
diff --git a/drivers/nvdimm/pmem.c b/drivers/nvdimm/pmem.c
index d25e66fd942d..4f667fe6ef72 100644
--- a/drivers/nvdimm/pmem.c
+++ b/drivers/nvdimm/pmem.c
@@ -401,8 +401,10 @@  static int pmem_attach_disk(struct device *dev,
 	if (is_nd_pfn(dev)) {
 		nd_pfn = to_nd_pfn(dev);
 		rc = nvdimm_setup_pfn(nd_pfn, &pmem->pgmap);
-		if (rc)
+		if (rc) {
+			devm_namespace_disable(dev, ndns);
 			return rc;
+		}
 	}
 
 	/* we're attaching a block device, disable raw namespace access */
@@ -549,17 +551,15 @@  static int nd_pmem_probe(struct device *dev)
 	ret = nd_pfn_probe(dev, ndns);
 	if (ret == 0)
 		return -ENXIO;
-	else if (ret == -EOPNOTSUPP)
-		return ret;
-
-	ret = nd_dax_probe(dev, ndns);
-	if (ret == 0)
-		return -ENXIO;
-	else if (ret == -EOPNOTSUPP)
-		return ret;
-
+	else if (ret != EOPNOTSUPP) {
+		ret = nd_dax_probe(dev, ndns);
+		if (ret == 0)
+			return -ENXIO;
+	}
 	/* probe complete, attach handles namespace enabling */
 	devm_namespace_disable(dev, ndns);
+	if (ret == -EOPNOTSUPP)
+		return ret;
 
 	return pmem_attach_disk(dev, ndns);
 }