linux-kernel - Re: [RFC PATCH 04/13] vfio/mdev: remove the usage of the list iterator after the loop

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:   Wed, 23 Feb 2022 23:08:53 +0100
From:   Jakob <jakobkoschel@...il.com>
To:     Linus Torvalds <torvalds@...ux-foundation.org>
Cc:     Jason Gunthorpe <jgg@...pe.ca>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        linux-arch <linux-arch@...r.kernel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Arnd Bergman <arnd@...db.de>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Kees Cook <keescook@...omium.org>,
        Mike Rapoport <rppt@...nel.org>,
        "Gustavo A. R. Silva" <gustavo@...eddedor.com>,
        Brian Johannesmeyer <bjohannesmeyer@...il.com>,
        Cristiano Giuffrida <c.giuffrida@...nl>,
        "Bos, H.J." <h.j.bos@...nl>
Subject: Re: [RFC PATCH 04/13] vfio/mdev: remove the usage of the list
 iterator after the loop



> On 23. Feb 2022, at 21:22, Linus Torvalds <torvalds@...ux-foundation.org> wrote:
> 
> On Wed, Feb 23, 2022 at 12:15 PM Jakob <jakobkoschel@...il.com> wrote:
>> 
>> in such a case you would still have to set the iterator value to
>> NULL when reaching the terminating condition or am I missing something?
> 
> No.
> 
> Make the rule be "you never use the iterator outside the loop".
> 
> IOW, the code sequence is
> 
>        some_struct *ptr, *iter;

with C99 iter would be defined within the loop instead right?

> 
>        ptr = NULL;
>        list_for_each_entry(iter, ...) {
>                if (iter_matches_condition(iter)) {
>                        ptr = iter;
>                        break;
>                }
>        }
> 
>        .. never use 'iter' here - you use 'ptr' and check it for NULL ..
> 
> See? Same number of variables as using a separate 'bool found' flag,
> but simpler code, and it matches the rule of 'don't use iter outside
> the loop'.

ah yes this does make sense. I missed the part of using a separate
'ptr' variable. Thanks for clarifying.
I think this is a great idea.

There are cases where pos->member is used (the only legitimate way to
use it right now). I suppose those turn into something like this
(this example is inspired by dev_add_offload() (net/core/gro.c:38)):

       some_struct *ptr, *iter;
       list_head *list_ptr;

       ptr = NULL;
       list_for_each_entry(iter, head, list) {
               if (iter_matches_condition(iter)) {
                       ptr = iter;
                       break;
               }
       }
       

       if (ptr)
               list_ptr = head->prev;
       else
               list_ptr = iter->list.prev;
       list_add(..., list_ptr);

before it was simply
       list_add(..., iter->list.prev);


The other possibility I suppose would be:

       if (!ptr)
               ptr = container_of(head, typeof(*ptr), list)
       list_add(..., ptr->list.prev);

which leaves you with the same type confusion as before, being far from
ideal.

> This is how you'd have to do it anyway if we start using a C99 style
> 'declare iter _in_ the loop' model.
> 
> And as mentioned, it actually tends to lead to better code, since the
> code outside the loop only has one variable live, not two.
> 
> Of course, compilers can do a lot of optimizations, so a 'found'
> variable can be made to generate good code too - if the compiler just
> tracks it and notices, and turns the 'break' into a 'goto found', and
> the fallthrough into the 'goto not_found'.
> 
> So 'better code generation' is debatable, but even if the compiler can
> do as good a job with a separate 'bool' variable and some cleverness,
> I think we should strive for code where we make it easy for the
> compiler to DTRT - and where the generated code is easier to match up
> with what we wrote.
> 
>                  Linus

If there is interest, I'm happy to send a new patch set once the fixes are clear.

	Jakob