linux-kernel - RE: 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]

Date: Wed, 10 Jan 2024 22:17:17 +0000
From: Michael Kelley <mhklinux@...look.com>
To: Dan Carpenter <dan.carpenter@...aro.org>
CC: 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>, "cocci@...ia.fr" <cocci@...ia.fr>, LKML
	<linux-kernel@...r.kernel.org>
Subject: RE: Drivers: hv: vmbus: One function call less in
 create_gpadl_header() after error detection

From: Dan Carpenter <dan.carpenter@...aro.org> Sent: Wednesday, January 10, 2024 10:38 AM
> 
> The second half of the if statement is basically duplicated.  It doesn't
> need to be treated as a special case.  We could do something like below.
> I deliberately didn't delete the tabs.  Also I haven't tested it.

Indeed!  I looked at the history, and this function has been
structured with the duplication since sometime in 2010, which
pre-dates my involvement by several years.  I don't know of
any reason why the duplication is needed, and agree it could
be eliminated.

Assuming Markus is OK with my proposal on the handling of
memory allocation failures, a single patch could simplify this
function quite a bit.

Dan -- do you want to create and submit the patch?  I'll test the
code on Hyper-V.  Or I can create, test, and submit the patch with
a "Suggested-by: Dan Carpenter".

Michael

> 
> regards,
> dan carpenter
> 
> diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c
> index 56f7e06c673e..2ba65f9ad3f1 100644
> --- a/drivers/hv/channel.c
> +++ b/drivers/hv/channel.c
> @@ -328,9 +328,9 @@ static int create_gpadl_header(enum hv_gpadl_type type, void *kbuffer,
>  		  sizeof(struct gpa_range);
>  	pfncount = pfnsize / sizeof(u64);
> 
> -	if (pagecount > pfncount) {
> -		/* we need a gpadl body */
> -		/* fill in the header */
> +	if (pagecount < pfncount)
> +		pfncount = pagecount;
> +
>  		msgsize = sizeof(struct vmbus_channel_msginfo) +
>  			  sizeof(struct vmbus_channel_gpadl_header) +
>  			  sizeof(struct gpa_range) + pfncount * sizeof(u64);
> @@ -410,31 +410,6 @@ static int create_gpadl_header(enum hv_gpadl_type type, void *kbuffer,
>  			pfnsum += pfncurr;
>  			pfnleft -= pfncurr;
>  		}
> -	} else {
> -		/* everything fits in a header */
> -		msgsize = sizeof(struct vmbus_channel_msginfo) +
> -			  sizeof(struct vmbus_channel_gpadl_header) +
> -			  sizeof(struct gpa_range) + pagecount * sizeof(u64);
> -		msgheader = kzalloc(msgsize, GFP_KERNEL);
> -		if (msgheader == NULL)
> -			goto nomem;
> -
> -		INIT_LIST_HEAD(&msgheader->submsglist);
> -		msgheader->msgsize = msgsize;
> -
> -		gpadl_header = (struct vmbus_channel_gpadl_header *)
> -			msgheader->msg;
> -		gpadl_header->rangecount = 1;
> -		gpadl_header->range_buflen = sizeof(struct gpa_range) +
> -					 pagecount * sizeof(u64);
> -		gpadl_header->range[0].byte_offset = 0;
> -		gpadl_header->range[0].byte_count = hv_gpadl_size(type, size);
> -		for (i = 0; i < pagecount; i++)
> -			gpadl_header->range[0].pfn_array[i] = hv_gpadl_hvpfn(
> -				type, kbuffer, size, send_offset, i);
> -
> -		*msginfo = msgheader;
> -	}
> 
>  	return 0;
>  nomem: