linux-kernel - RE: [PATCH v2 1/2] Drivers: hv: util: Don't force error code to ENODEV in util

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:
 <SN6PR02MB4157ED3DA55558829D6152D5D4332@SN6PR02MB4157.namprd02.prod.outlook.com>
Date: Sun, 8 Dec 2024 23:12:15 +0000
From: Michael Kelley <mhklinux@...look.com>
To: Saurabh Singh Sengar <ssengar@...ux.microsoft.com>
CC: "kys@...rosoft.com" <kys@...rosoft.com>, "haiyangz@...rosoft.com"
	<haiyangz@...rosoft.com>, "wei.liu@...nel.org" <wei.liu@...nel.org>,
	"decui@...rosoft.com" <decui@...rosoft.com>, "gregkh@...uxfoundation.org"
	<gregkh@...uxfoundation.org>, "vkuznets@...hat.com" <vkuznets@...hat.com>,
	"linux-hyperv@...r.kernel.org" <linux-hyperv@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH v2 1/2] Drivers: hv: util: Don't force error code to
 ENODEV in util_probe()

From: Saurabh Singh Sengar <ssengar@...ux.microsoft.com> Sent: Sunday, December 8, 2024 9:31 AM
> 
> On Wed, Nov 06, 2024 at 07:42:46AM -0800, mhkelley58@...il.com wrote:
> > From: Michael Kelley <mhklinux@...look.com>
> >
> > If the util_init function call in util_probe() returns an error code,
> > util_probe() always return ENODEV, and the error code from the util_init
> > function is lost. The error message output in the caller, vmbus_probe(),
> > doesn't show the real error code.
> >
> > Fix this by just returning the error code from the util_init function.
> > There doesn't seem to be a reason to force ENODEV, as other errors
> > such as ENOMEM can already be returned from util_probe(). And the
> > code in call_driver_probe() implies that ENODEV should mean that a
> > matching driver wasn't found, which is not the case here.
> >
> > Suggested-by: Dexuan Cui <decui@...rosoft.com>
> > Signed-off-by: Michael Kelley <mhklinux@...look.com>
> > ---
> > Changes in v2: None. This is the first version of Patch 1 of this series.
> > The "v2" is due to changes to Patch 2 of the series.
> >
> >  drivers/hv/hv_util.c | 4 +---
> >  1 file changed, 1 insertion(+), 3 deletions(-)
> >
> > diff --git a/drivers/hv/hv_util.c b/drivers/hv/hv_util.c
> > index c4f525325790..370722220134 100644
> > --- a/drivers/hv/hv_util.c
> > +++ b/drivers/hv/hv_util.c
> > @@ -590,10 +590,8 @@ static int util_probe(struct hv_device *dev,
> >  	srv->channel = dev->channel;
> >  	if (srv->util_init) {
> >  		ret = srv->util_init(srv);
> > -		if (ret) {
> > -			ret = -ENODEV;
> > +		if (ret)
> >  			goto error1;
> > -		}
> 
> After reviewing V2 of this series, I couldn’t find any scenario where
> 'util_init' in any driver returns a value other than 0. 

Yeah, I noticed the same thing when doing this patch set.

> In such cases,
> could we consider making all these functions 'void' ?
> 
> After this ee can remove the check for util_int return type.

I decided against making these changes. It seemed like code churn
for not much benefit. And there's the possibility of some future
change reintroducing an error code in one of the util_init functions,
in which case we would need to put things back like they are now.
Certainly this is a judgment call, but my take was to leave things
as they are.

The changes you suggest would probably go as a third patch in
the series. Wei Liu has already picked up the two patches as they
are, so it would be fine to create an independent patch with the
changes you suggest, if we want to go that route. My preference
isn't that strong either way.

Michael