linux-kernel - RE: [PATCH] Drivers: hv: vmbus: One function call less in create_gpadl

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID:
 <SN6PR02MB4157AA51AD8AEBB24D0668B7D4692@SN6PR02MB4157.namprd02.prod.outlook.com>
Date: Wed, 10 Jan 2024 05:41:00 +0000
From: Michael Kelley <mhklinux@...look.com>
To: Markus Elfring <Markus.Elfring@....de>, "linux-hyperv@...r.kernel.org"
	<linux-hyperv@...r.kernel.org>, "kernel-janitors@...r.kernel.org"
	<kernel-janitors@...r.kernel.org>, Dexuan Cui <decui@...rosoft.com>, Haiyang
 Zhang <haiyangz@...rosoft.com>, "K. Y. Srinivasan" <kys@...rosoft.com>, Wei
 Liu <wei.liu@...nel.org>
CC: LKML <linux-kernel@...r.kernel.org>, "cocci@...ia.fr" <cocci@...ia.fr>
Subject: RE: [PATCH] Drivers: hv: vmbus: One function call less in
 create_gpadl_header() after error detection

From: Markus Elfring <Markus.Elfring@....de> Sent: Tuesday, December 26, 2023 11:09 AM
> 
> The kfree() function was called in two cases by
> the create_gpadl_header() function during error handling
> even if the passed variable contained a null pointer.
> This issue was detected by using the Coccinelle software.
> 
> Thus use another label.

Interestingly, there's a third case in this function where
"goto nomem" is done, and in this case, msgbody is NULL.
Does Coccinelle not complain about that case as well?

As I'm sure you know, the code is correct as is, because kfree()
checks for a NULL argument.  So this is really an exercise in
making Coccinelle happy.  To me, the additional label is
incremental complexity for someone to deal with when
reading the code at some time in the future.  So I'd vote for
leaving the code as is.  But it's not a big deal either way.  I
can see you've been cleaning up a lot of Coccinelle-reported
issues across the kernel, most of which result in code
simplifications.  If leaving this unchanged causes you problems,
then I won't object (though perhaps that 3rd "goto nomem"
should be dealt with as well for consistency).

Michael

> 
> Signed-off-by: Markus Elfring <elfring@...rs.sourceforge.net>
> ---
>  drivers/hv/channel.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c
> index 56f7e06c673e..4d1bbda895d8 100644
> --- a/drivers/hv/channel.c
> +++ b/drivers/hv/channel.c
> @@ -336,7 +336,7 @@ static int create_gpadl_header(enum hv_gpadl_type type, void *kbuffer,
>  			  sizeof(struct gpa_range) + pfncount * sizeof(u64);
>  		msgheader =  kzalloc(msgsize, GFP_KERNEL);
>  		if (!msgheader)
> -			goto nomem;
> +			goto free_body;
> 
>  		INIT_LIST_HEAD(&msgheader->submsglist);
>  		msgheader->msgsize = msgsize;
> @@ -417,7 +417,7 @@ static int create_gpadl_header(enum hv_gpadl_type type, void *kbuffer,
>  			  sizeof(struct gpa_range) + pagecount * sizeof(u64);
>  		msgheader = kzalloc(msgsize, GFP_KERNEL);
>  		if (msgheader == NULL)
> -			goto nomem;
> +			goto free_body;
> 
>  		INIT_LIST_HEAD(&msgheader->submsglist);
>  		msgheader->msgsize = msgsize;
> @@ -439,6 +439,7 @@ static int create_gpadl_header(enum hv_gpadl_type type, void *kbuffer,
>  	return 0;
>  nomem:
>  	kfree(msgheader);
> +free_body:
>  	kfree(msgbody);
>  	return -ENOMEM;
>  }
> --
> 2.43.0
>