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  PHC 
Open Source and information security mailing list archives
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Sun, 6 Mar 2022 13:19:09 +0100
From:   Jakob Koschel <>
To:     Linus Torvalds <>
Cc:     Xiaomeng Tong <>,
        Arnd Bergmann <>,
        Greg Kroah-Hartman <>,
        Jann Horn <>,
        Kees Cook <>,
        Linux Kbuild mailing list <>,
        Linux Kernel Mailing List <>,
        Linux-MM <>, Netdev <>
Subject: Re: [PATCH 2/6] list: add new MACROs to make iterator invisiable
 outside the loop

> On 6. Mar 2022, at 01:35, Linus Torvalds <> wrote:
> On Sat, Mar 5, 2022 at 1:09 PM Linus Torvalds
> <> wrote:
>> Now, I'd love for the list head entry itself to "declare the type",
>> and solve it that way. That would in many ways be the optimal
>> situation, in that when a structure has that
>>        struct list_head xyz;
>> entry, it would be lovely to declare *there* what the list entry type
>> is - and have 'list_for_each_entry()' just pick it up that way.
>> It would be doable in theory - with some preprocessor trickery [...]
> Ok, I decided to look at how that theory looks in real life.
> The attached patch does actually work for me. I'm not saying this is
> *beautiful*, but I made the changes to kernel/exit.c to show how this
> can be used, and while the preprocessor tricks and the odd "unnamed
> union with a special member to give the target type" is all kinds of
> hacky, the actual use case code looks quite nice.
> In particular, look at the "good case" list_for_each_entry() transformation:
>   static int do_wait_thread(struct wait_opts *wo, struct task_struct *tsk)
>   {
>  -     struct task_struct *p;
>  -
>  -     list_for_each_entry(p, &tsk->children, sibling) {
>  +     list_traverse(p, &tsk->children, sibling) {
> IOW, it avoided the need to declare 'p' entirely, and it avoids the
> need for a type, because the macro now *knows* the type of that
> 'tsk->children' list and picks it out automatically.
> So 'list_traverse()' is basically a simplified version of
> 'list_for_each_entry()'.
> That patch also has - as another example - the "use outside the loop"
> case in mm_update_next_owner(). That is more of a "rewrite the loop
> cleanly using list_traverse() thing, but it's also quite simple and
> natural.
> One nice part of this approach is that it allows for incremental changes.
> In fact, the patch very much is meant to demonstrate exactly that:
> yes, it converts the uses in kernel/exit.c, but it does *not* convert
> the code in kernel/fork.c, which still does that old-style traversal:
>                list_for_each_entry(child, &parent->children, sibling) {
> and the kernel/fork.c code continues to work as well as it ever did.
> So that new 'list_traverse()' function allows for people to say "ok, I
> will now declare that list head with that list_traversal_head() macro,
> and then I can convert 'list_for_each_entry()' users one by one to
> this simpler syntax that also doesn't allow the list iterator to be
> used outside the list.
> What do people think? Is this clever and useful, or just too subtle
> and odd to exist?
> NOTE! I decided to add that "name of the target head in the target
> type" to the list_traversal_head() macro, but it's not actually used
> as is. It's more of a wishful "maybe we could add some sanity checking
> of the target list entries later".
> Comments?

I guess we could apply this to list_for_each_entry() as well
once all the uses after the loop are fixed?

I feel like this simply introduces a new set of macros
(we would also need list_traverse_reverse(), list_traverse_continue_reverse()
etc) and end up with a second set of macros that do pretty much
the same as the first one.

I like the way of using list_traversal_head() to only remember the type.
The interface of list_traverse() is the same as list_for_each_entry() so
we could just do this with a simple coccinelle script once 'pos' is no
longer used after the loop:

-struct some_struct *pos;
+list_traversal_head(struct some_struct, pos, target_member);

although there are *some* cases where 'pos' is also used separately
in the function which would need to change, e.g.:

struct some_struct *pos = some_variable;

if (pos)
	// do one thing
	list_for_each_entry(pos, ..., ...)

(I've fixed ~440/450 cases now and I'm chunking it into patch sets right now.
The once left over are non-obvious code I would need some input on)

Personally I guess I also prefer the name list_for_each_entry() over list_traverse()
and not having two types of iterators for the same thing at the same time.

>                   Linus
> <patch.diff>


Powered by blists - more mailing lists