linux-kernel - Re: [PATCH v2 1/4] driver core: Ignore 0 in dev_err

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: <ZtWjqkQUi74JFN1s@smile.fi.intel.com>
Date: Mon, 2 Sep 2024 14:38:18 +0300
From: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To: Dan Carpenter <dan.carpenter@...aro.org>
Cc: Baolin Wang <baolin.wang@...ux.alibaba.com>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>,
	Hans de Goede <hdegoede@...hat.com>, linux-kernel@...r.kernel.org,
	platform-driver-x86@...r.kernel.org,
	"Rafael J. Wysocki" <rafael@...nel.org>,
	Daniel Scally <djrscally@...il.com>
Subject: Re: [PATCH v2 1/4] driver core: Ignore 0 in dev_err_probe()

On Mon, Sep 02, 2024 at 02:10:41PM +0300, Dan Carpenter wrote:
> On Mon, Sep 02, 2024 at 01:16:17PM +0300, Andy Shevchenko wrote:
> > On Sat, Aug 31, 2024 at 11:53:51AM +0300, Dan Carpenter wrote:
> > > On Sat, Aug 31, 2024 at 11:25:54AM +0300, Dan Carpenter wrote:
> > > > On Thu, Aug 22, 2024 at 04:05:38PM +0300, Andy Shevchenko wrote:
> > > > > In the similar way, ignore 0 error code (AKA "success") in
> > > > > dev_err_probe(). This helps to simplify a code such as
> > > > > 
> > > > >   if (ret < 0)
> > > > >     return dev_err_probe(int3472->dev, ret, err_msg);
> > > > > 
> > > > >   return ret;
> > > > > 
> > > > > to
> > > > > 
> > > > >   return dev_err_probe(int3472->dev, ret, err_msg);
> > > > > 
> > > > > Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
> > > > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
> > > > 
> > > > This is a terrible idea because currently Smatch is able to detect about one
> > > > bug per month where someone unintentionally passes the wrong error variable
> > > > to dev_err_probe().
> > 
> > How many cases you know where smatch is false positive about this?
> 
> This check has a very low false positive rate.

Yes, that's my expectation as well.

> There is only one potential
> false positive in the current linux-next.  Let me add Baolin Wang to get his
> take on this.  I never mentioned reported this warning because the code was old
> when I wrote the check.
> 
> drivers/spi/spi-sprd-adi.c
>    550          ret = of_hwspin_lock_get_id(np, 0);
>    551          if (ret > 0 || (IS_ENABLED(CONFIG_HWSPINLOCK) && ret == 0)) {
> 
> Is it possible for the CONFIG_ to not be enabled but ret is zero?
> 
>    552                  sadi->hwlock =
>    553                          devm_hwspin_lock_request_specific(&pdev->dev, ret);
>    554                  if (!sadi->hwlock) {
>    555                          ret = -ENXIO;
>    556                          goto put_ctlr;
>    557                  }
>    558          } else {
>    559                  switch (ret) {
>    560                  case -ENOENT:
>    561                          dev_info(&pdev->dev, "no hardware spinlock supplied\n");
>    562                          break;
>    563                  default:
>    564                          dev_err_probe(&pdev->dev, ret, "failed to find hwlock id\n");
>                                                           ^^^
> 
>    565                          goto put_ctlr;
>    566                  }
>    567          }
> 
> 
> > I believe the number is only a few at most, which means that you may easily
> > detect this still with this change being applied, i.e. "anything that
> > terminates function flow with code 0, passed to dev_err_probe(), is
> > suspicious".
> 
> I think you mean the opposite of what you wrote?  That if we're passing zero to
> dev_err_probe() and it's the last line in a function it's *NOT* suspicious?

Yes, sorry, I meant "...terminates function flow _in the middle_...".

> Otherwise, I don't really understand the heuristic you're proposing.

-- 
With Best Regards,
Andy Shevchenko