[PATCH] dm: Use common error handling code in three functions

[PATCH] dm: Use common error handling code in three functions

[Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Tue, 9 Jun 2026 21:54:38 +0200

Use additional labels so that a bit of exception handling can be better
reused at the end of three if branches.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/md/dm-ioctl.c  | 7 +++----
 drivers/md/dm-raid1.c  | 4 ++--
 drivers/md/dm-stripe.c | 7 +++----
 3 files changed, 8 insertions(+), 10 deletions(-)

diff --git a/drivers/md/dm-ioctl.c b/drivers/md/dm-ioctl.c
index ac77dc0ca225..9537e60f94c3 100644
--- a/drivers/md/dm-ioctl.c
+++ b/drivers/md/dm-ioctl.c
@@ -227,10 +227,8 @@ static struct hash_cell *alloc_cell(const char *name, const char *uuid,
 		return NULL;
 
 	hc->name = kstrdup(name, GFP_KERNEL);
-	if (!hc->name) {
-		kfree(hc);
-		return NULL;
-	}
+	if (!hc->name)
+		goto free_hc;
 
 	if (!uuid)
 		hc->uuid = NULL;
@@ -239,6 +237,7 @@ static struct hash_cell *alloc_cell(const char *name, const char *uuid,
 		hc->uuid = kstrdup(uuid, GFP_KERNEL);
 		if (!hc->uuid) {
 			kfree(hc->name);
+free_hc:
 			kfree(hc);
 			return NULL;
 		}
diff --git a/drivers/md/dm-raid1.c b/drivers/md/dm-raid1.c
index de5c00704e69..359ca80a67b3 100644
--- a/drivers/md/dm-raid1.c
+++ b/drivers/md/dm-raid1.c
@@ -915,8 +915,7 @@ static struct mirror_set *alloc_context(unsigned int nr_mirrors,
 	ms->io_client = dm_io_client_create();
 	if (IS_ERR(ms->io_client)) {
 		ti->error = "Error creating dm_io client";
-		kfree(ms);
-		return NULL;
+		goto free_ms;
 	}
 
 	ms->rh = dm_region_hash_create(ms, dispatch_bios, wakeup_mirrord,
@@ -926,6 +925,7 @@ static struct mirror_set *alloc_context(unsigned int nr_mirrors,
 	if (IS_ERR(ms->rh)) {
 		ti->error = "Error creating dirty region hash";
 		dm_io_client_destroy(ms->io_client);
+free_ms:
 		kfree(ms);
 		return NULL;
 	}
diff --git a/drivers/md/dm-stripe.c b/drivers/md/dm-stripe.c
index 750865fd3ae7..58fa0badc90f 100644
--- a/drivers/md/dm-stripe.c
+++ b/drivers/md/dm-stripe.c
@@ -148,10 +148,8 @@ static int stripe_ctr(struct dm_target *ti, unsigned int argc, char **argv)
 		sc->stripes_shift = __ffs(stripes);
 
 	r = dm_set_target_max_io_len(ti, chunk_size);
-	if (r) {
-		kfree(sc);
-		return r;
-	}
+	if (r)
+		goto free_sc;
 
 	ti->num_flush_bios = stripes;
 	ti->num_discard_bios = stripes;
@@ -176,6 +174,7 @@ static int stripe_ctr(struct dm_target *ti, unsigned int argc, char **argv)
 			ti->error = "Couldn't parse stripe destination";
 			while (i--)
 				dm_put_device(ti, sc->stripe[i].dev);
+free_sc:
 			kfree(sc);
 			return r;
 		}
-- 
2.54.0

Re: [PATCH] dm: Use common error handling code in three functions

[Mikulas Patocka]

On Tue, 9 Jun 2026, Markus Elfring wrote:

> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Tue, 9 Jun 2026 21:54:38 +0200
> 
> Use additional labels so that a bit of exception handling can be better
> reused at the end of three if branches.
> 
> This issue was detected by using the Coccinelle software.

Hi

I think that jumping into a nested block isn't good practice and it makes 
the code harder to read and maintain. I would redo the patches so that 
they jump to the end of the topmost function block, for example:

static struct hash_cell *alloc_cell(const char *name, const char *uuid,
				    struct mapped_device *md)
{
	struct hash_cell *hc;

	hc = kmalloc_obj(*hc);
	if (!hc)
		goto ret;
...
	hc->name = kstrdup(name, GFP_KERNEL);
	if (!hc->name)
		goto free_hc_ret;
...
	hc->uuid = kstrdup(uuid, GFP_KERNEL);
	if (!hc->uuid)
		goto free_hc_name_ret;
	
...
	return hc;

free_hc_name_ret:
	kfree(hc->name);
free_hc_ret:
	kfree(hc);
ret:
	return NULL;
}

Mikulas

> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  drivers/md/dm-ioctl.c  | 7 +++----
>  drivers/md/dm-raid1.c  | 4 ++--
>  drivers/md/dm-stripe.c | 7 +++----
>  3 files changed, 8 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/md/dm-ioctl.c b/drivers/md/dm-ioctl.c
> index ac77dc0ca225..9537e60f94c3 100644
> --- a/drivers/md/dm-ioctl.c
> +++ b/drivers/md/dm-ioctl.c
> @@ -227,10 +227,8 @@ static struct hash_cell *alloc_cell(const char *name, const char *uuid,
>  		return NULL;
>  
>  	hc->name = kstrdup(name, GFP_KERNEL);
> -	if (!hc->name) {
> -		kfree(hc);
> -		return NULL;
> -	}
> +	if (!hc->name)
> +		goto free_hc;
>  
>  	if (!uuid)
>  		hc->uuid = NULL;
> @@ -239,6 +237,7 @@ static struct hash_cell *alloc_cell(const char *name, const char *uuid,
>  		hc->uuid = kstrdup(uuid, GFP_KERNEL);
>  		if (!hc->uuid) {
>  			kfree(hc->name);
> +free_hc:
>  			kfree(hc);
>  			return NULL;
>  		}
> diff --git a/drivers/md/dm-raid1.c b/drivers/md/dm-raid1.c
> index de5c00704e69..359ca80a67b3 100644
> --- a/drivers/md/dm-raid1.c
> +++ b/drivers/md/dm-raid1.c
> @@ -915,8 +915,7 @@ static struct mirror_set *alloc_context(unsigned int nr_mirrors,
>  	ms->io_client = dm_io_client_create();
>  	if (IS_ERR(ms->io_client)) {
>  		ti->error = "Error creating dm_io client";
> -		kfree(ms);
> -		return NULL;
> +		goto free_ms;
>  	}
>  
>  	ms->rh = dm_region_hash_create(ms, dispatch_bios, wakeup_mirrord,
> @@ -926,6 +925,7 @@ static struct mirror_set *alloc_context(unsigned int nr_mirrors,
>  	if (IS_ERR(ms->rh)) {
>  		ti->error = "Error creating dirty region hash";
>  		dm_io_client_destroy(ms->io_client);
> +free_ms:
>  		kfree(ms);
>  		return NULL;
>  	}
> diff --git a/drivers/md/dm-stripe.c b/drivers/md/dm-stripe.c
> index 750865fd3ae7..58fa0badc90f 100644
> --- a/drivers/md/dm-stripe.c
> +++ b/drivers/md/dm-stripe.c
> @@ -148,10 +148,8 @@ static int stripe_ctr(struct dm_target *ti, unsigned int argc, char **argv)
>  		sc->stripes_shift = __ffs(stripes);
>  
>  	r = dm_set_target_max_io_len(ti, chunk_size);
> -	if (r) {
> -		kfree(sc);
> -		return r;
> -	}
> +	if (r)
> +		goto free_sc;
>  
>  	ti->num_flush_bios = stripes;
>  	ti->num_discard_bios = stripes;
> @@ -176,6 +174,7 @@ static int stripe_ctr(struct dm_target *ti, unsigned int argc, char **argv)
>  			ti->error = "Couldn't parse stripe destination";
>  			while (i--)
>  				dm_put_device(ti, sc->stripe[i].dev);
> +free_sc:
>  			kfree(sc);
>  			return r;
>  		}
> -- 
> 2.54.0
> 

Re: dm: Use common error handling code in three functions

[Markus Elfring]

>> Use additional labels so that a bit of exception handling can be better
>> reused at the end of three if branches.
>>
>> This issue was detected by using the Coccinelle software.
> 
> Hi
> 
> I think that jumping into a nested block isn't good practice and it makes 
> the code harder to read and maintain. I would redo the patches so that 
> they jump to the end of the topmost function block, for example:
Thanks for your constructive feedback.

Can any further adjustments become feasible for the design goal “Centralized exiting of functions”?
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/coding-style.rst?h=v7.1-rc7#n526

Would you find goto chains applicable for another while?
https://cmu-sei.github.io/secure-coding-standards/sei-cert-c-coding-standard/recommendations/memory-management-mem/mem12-c/

How do you think about to increase the application of scope-based resource management?
https://elixir.bootlin.com/linux/v7.1-rc7/source/include/linux/cleanup.h

Regards,
Markus

Re: dm: Use common error handling code in three functions

[Mikulas Patocka]

[-- Attachment #1: Type: text/plain, Size: 1707 bytes --]



On Thu, 11 Jun 2026, Markus Elfring wrote:

> >> Use additional labels so that a bit of exception handling can be better
> >> reused at the end of three if branches.
> >>
> >> This issue was detected by using the Coccinelle software.
> > 
> > Hi
> > 
> > I think that jumping into a nested block isn't good practice and it makes 
> > the code harder to read and maintain. I would redo the patches so that 
> > they jump to the end of the topmost function block, for example:
> Thanks for your constructive feedback.
> 
> Can any further adjustments become feasible for the design goal “Centralized exiting of functions”?
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/coding-style.rst?h=v7.1-rc7#n526

I think that this document is OK.

> Would you find goto chains applicable for another while?
> https://cmu-sei.github.io/secure-coding-standards/sei-cert-c-coding-standard/recommendations/memory-management-mem/mem12-c/

I think goto chains are ok if there are few variables to free.

> How do you think about to increase the application of scope-based resource management?
> https://elixir.bootlin.com/linux/v7.1-rc7/source/include/linux/cleanup.h

I am not a fan of these. I think that goto is OK if there are few 
variables to clean-up. If there is a large number of variables to clean 
up, it's better to zero-fill the whole structure and free the entries 
unconditionally (kfree does noting if it gets NULL) - see for example 
dm_integrity_ctr - it allocates the structure with kzalloc_obj and on any 
error it jumps to the label "bad" that calls dm_integrity_dtr - and 
dm_integrity_dtr frees all non-NULL entries.

Mikulas

> Regards,
> Markus