linux-kernel - Re: [PATCH v5 1/4] device property: Allow error pointer to be passed to fwnode APIs

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:   Fri, 8 Apr 2022 15:44:03 +0300
From:   Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To:     Michael Walle <michael@...le.cc>
Cc:     linux-acpi@...r.kernel.org, linux-kernel@...r.kernel.org,
        Daniel Scally <djrscally@...il.com>,
        Heikki Krogerus <heikki.krogerus@...ux.intel.com>,
        Sakari Ailus <sakari.ailus@...ux.intel.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        "Rafael J. Wysocki" <rafael@...nel.org>,
        Len Brown <lenb@...nel.org>,
        Nuno Sá <nuno.sa@...log.com>
Subject: Re: [PATCH v5 1/4] device property: Allow error pointer to be passed
 to fwnode APIs

On Thu, Apr 07, 2022 at 01:19:44PM +0300, Andy Shevchenko wrote:
> On Wed, Apr 06, 2022 at 08:05:23PM +0200, Michael Walle wrote:

...

> > > +	if (IS_ERR_OR_NULL(fwnode))
> > > +		return -ENOENT;
> > > +
> > >  	ret = fwnode_call_int_op(fwnode, get_reference_args, prop, nargs_prop,
> > >  				 nargs, index, args);
> > > +	if (ret == 0)
> > > +		return ret;
> > > 
> > > -	if (ret < 0 && !IS_ERR_OR_NULL(fwnode) &&
> > > -	    !IS_ERR_OR_NULL(fwnode->secondary))
> > > -		ret = fwnode_call_int_op(fwnode->secondary, get_reference_args,
> > > -					 prop, nargs_prop, nargs, index, args);
> > > +	if (IS_ERR_OR_NULL(fwnode->secondary))
> > > +		return -ENOENT;
> > 
> > Doesn't this mean you overwrite any return code != 0 with -ENOENT?
> > Is this intended?
> 
> Indeed, it would shadow the error code.

I was thinking more on this and am not sure about the best approach here.
On one hand in the original code this returns the actual error code from
the call against primary fwnode. But it can be at least -ENOENT or -EINVAL.

But when we check the secondary fwnode we want to have understanding that it's
secondary fwnode which has not been found, but this requires to have a good
distinguishing between error codes from the callback.

That said, the error codes convention of ->get_reference_args() simply
sucks. Sakari, do you have it on your TODO to fix this mess out, if it's
even feasible?

To be on safest side, I will change as suggested in previous mail (see below)
so it won't have impact on -EINVAL case.

> So, it should go with
> 
> 	if (IS_ERR_OR_NULL(fwnode->secondary))
> 		return ret;
> 
> then.

-- 
With Best Regards,
Andy Shevchenko