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 17:53:17 +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: Drivers: hv: vmbus: One function call less in
 create_gpadl_header() after error detection

From: Markus Elfring <Markus.Elfring@....de> Sent: Wednesday, January 10, 2024 9:08 AM
> 
> > It occurred to me overnight that the existing error handling
> > in create_gpadl_header() is unnecessarily complicated.  Here's
> > an approach that I think would fix what you have flagged, and
> > would reduce complexity instead of increasing it.  Thoughts?
> 
> I find this development view interesting.
> 
> 
> > diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c
> > index 56f7e06c673e..44b1d5c8dfed 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;
> > +			return -ENOMEM;
> >
> >  		INIT_LIST_HEAD(&msgheader->submsglist);
> >  		msgheader->msgsize = msgsize;
> > @@ -386,8 +386,8 @@ static int create_gpadl_header(enum hv_gpadl_type type, void *kbuffer,
> >  					list_del(&pos->msglistentry);
> >  					kfree(pos);
> >  				}
> > -
> > -				goto nomem;
> > +				kfree(msgheader);
> > +				return -ENOMEM;
> >  			}
> >
> >  			msgbody->msgsize = msgsize;
> > @@ -416,8 +416,8 @@ static int create_gpadl_header(enum hv_gpadl_type type, void *kbuffer,
> >  			  sizeof(struct vmbus_channel_gpadl_header) +
> >  			  sizeof(struct gpa_range) + pagecount * sizeof(u64);
> >  		msgheader = kzalloc(msgsize, GFP_KERNEL);
> > -		if (msgheader == NULL)
> > -			goto nomem;
> > +		if (!msgheader)
> > +			return -ENOMEM;
> >
> >  		INIT_LIST_HEAD(&msgheader->submsglist);
> >  		msgheader->msgsize = msgsize;
> > @@ -437,10 +437,6 @@ static int create_gpadl_header(enum hv_gpadl_type type, void *kbuffer,
> >  	}
> >
> >  	return 0;
> > -nomem:
> > -	kfree(msgheader);
> > -	kfree(msgbody);
> > -	return -ENOMEM;
> >  }
> >
> >  /*
> >
> 
> Should up to two memory areas still be released after a data processing
> failure?

The function create_gpadl_header() is responsible only for allocating
the memory and filling in various fields.  It doesn't do any processing of
the data and doesn't generate any errors other than memory allocation
failures.  If create_gpadl_header() succeeds, it returns a pointer to the
allocated memory via the msginfo parameter, and the caller becomes
responsible for free'ing the memory.

The only caller is __vmbus_establish_gpadl(), which *does* free the
memory after communicating with Hyper-V.   Processing errors may
occur when communicating with Hyper-V, but in a quick review,
__vmbus_establish_gpadl() seems to handle those errors and to
correctly free the memory.

Michael