linux-kernel - Re: [RFC PATCH] Add bridge driver to connect sensors to CIO2 device via software nodes on ACPI platforms

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:   Thu, 17 Sep 2020 16:15:39 +0300
From:   Dan Carpenter <dan.carpenter@...cle.com>
To:     Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Cc:     devel@...verdev.osuosl.org, robh@...nel.org, mchehab@...nel.org,
        jorhand@...ux.microsoft.com, gregkh@...uxfoundation.org,
        linux-kernel@...r.kernel.org, kieran.bingham@...asonboard.com,
        Daniel Scally <djrscally@...il.com>, yong.zhi@...el.com,
        Sakari Ailus <sakari.ailus@...ux.intel.com>,
        bingbu.cao@...el.com, kitakar@...il.com, davem@...emloft.net,
        tian.shu.qiu@...el.com, linux-media@...r.kernel.org
Subject: Re: [RFC PATCH] Add bridge driver to connect sensors to CIO2 device
 via software nodes on ACPI platforms

On Thu, Sep 17, 2020 at 03:25:29PM +0300, Andy Shevchenko wrote:
> On Thu, Sep 17, 2020 at 01:49:41PM +0300, Dan Carpenter wrote:
> > On Thu, Sep 17, 2020 at 01:33:43PM +0300, Sakari Ailus wrote:
> 
> > > > +	int i, ret;
> > > 
> > > unsigned int i
> > > 
> > 
> > Why?
> > 
> > For list iterators then "int i;" is best...  For sizes then unsigned is
> > sometimes best.  Or if it's part of the hardware spec or network spec
> > unsigned is best.  Otherwise unsigned variables cause a ton of bugs.
> > They're not as intuitive as signed variables.  Imagine if there is an
> > error in this loop and you want to unwind.  With a signed variable you
> > can do:
> > 
> > 	while (--i >= 0)
> > 		cleanup(&bridge.sensors[i]);
> 
> Ha-ha. It's actually a counter argument to your stuff because above is the same as
> 
> 	while (i--)
> 		cleanup(&bridge.sensors[i]);
> 
> with pretty much unsigned int i.

With vanilla "int i;" you can't go wrong because both styles work as
expected.  I was just giving examples of real life bugs that I have seen
involving unsigned iterators.

54313503f9a3 ("drm/amdgpu: signedness bug in amdgpu_cs_parser_init()")

Here are a couple more bugs involving unsigned iterators...

d72cf01f410a ("drm/mipi-dbi: fix a loop in debugfs code")
ce6c1cd2c324 ("pinctrl: nsp-gpio: forever loop in nsp_gpio_get_strength()")

I've change a lot of variables unsigned as well.  For min_t() then it
should *always* be an unsigned type.

It's pretty rare to iterate over 2 billion times in the kernel, but
there are times when you might want to do that.  Normally you would
want to declare the iterator as an unsigned ong in that case.  But most
of the time iterators should just be "int i;" to prevent bugs.

regards,
dan carpenter