Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220304124416.1181029-1-mailhol.vincent@wanadoo.fr>
 <CAHp75VeT3LbdbSaiwcC2YW40LnA2h8ADtGva-CKU_xh8Edi0nw@mail.gmail.com>
 <20220307105810.1747024-1-alexandr.lobakin@intel.com> <CAK8P3a07uXLTpNz3t1oMwz_imZOTs+1Fw5hhELjUrJ8Zs=8bpQ@mail.gmail.com>
 <CAMZ6RqKaq8GiPYMyL4D1ksUtgketAF905oBgwKuGLtk7BmobZw@mail.gmail.com> <20220307150750.1762040-1-alexandr.lobakin@intel.com>
In-Reply-To: <20220307150750.1762040-1-alexandr.lobakin@intel.com>
From:   Andy Shevchenko <andy.shevchenko@gmail.com>
Date:   Mon, 7 Mar 2022 18:30:53 +0200
Message-ID: <CAHp75VfXeo0rrKB74t7gJOcv4CEnkByucE+Sa1hT2GTap=oQNA@mail.gmail.com>
Subject: Re: [PATCH] linux/bits.h: fix -Wtype-limits warnings in GENMASK_INPUT_CHECK()
To:     Alexander Lobakin <alexandr.lobakin@intel.com>
Cc:     Vincent MAILHOL <mailhol.vincent@wanadoo.fr>,
        Arnd Bergmann <arnd@arndb.de>,
        Rikard Falkeborn <rikard.falkeborn@gmail.com>,
        Andrew Morton <akpm@linux-foundation.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Kees Cook <keescook@chromium.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Mon, Mar 7, 2022 at 5:10 PM Alexander Lobakin
<alexandr.lobakin@intel.com> wrote:
> From: Vincent MAILHOL <mailhol.vincent@wanadoo.fr>
> Date: Mon, 7 Mar 2022 22:50:56 +0900

...

> For example, people tend to make the following mistake:
>
>         unsigned int i;
>
>         for (i = 0; i ...) {
>                 ret = setup_something(array[i]);
>                 if (ret)
>                         goto unroll;
>         }
>
> unroll:
>         while (--i)
>                 unroll_something(array[i]);
>
> The loop will never end as `i` was declared as unsigned.
> -Wtype-limits catches this.

This looks like a wrapping value issue, not sure if the type limits
makes logical sense. What I'm saying is that the waning is
controversial. It may help or it may make noise.

> Not speaking of checking unsigned variables on < 0:
>
>         unsigned int num;
>
>         /* calculate_something() returns the number of something
>          * or -ERRNO in case of an error
>          */
>         num = calculate_something();
>         if (num < 0)
>                 ...

Depends on the context. Here is a mistake, but there are plenty of
cases when it's okay to do so. And in the above the variable name is
misleading with its semantics, The proper code should be

  unsigned int num;
  int ret;

  ret = ...
  if (ret < 0)
    ...
  num = ret;

Again, the warning is controversial in my opinion.

-- 
With Best Regards,
Andy Shevchenko