Re: [PATCH] llist: Don't reinvent the wheel but use existing llist API

[Al Viro <viro@ZenIV.linux.org.uk>]

On Mon, Feb 13, 2017 at 01:10:13PM +0900, Byungchul Park wrote:
> Although llist provides proper APIs, they are not used. Make them used.

> @@ -231,12 +231,10 @@ static void __fput(struct file *file)
>  static void delayed_fput(struct work_struct *unused)
>  {
>  	struct llist_node *node = llist_del_all(&delayed_fput_list);
> -	struct llist_node *next;
> +	struct file *f;
>  
> -	for (; node; node = next) {
> -		next = llist_next(node);
> -		__fput(llist_entry(node, struct file, f_u.fu_llist));
> -	}
> +	llist_for_each_entry(f, node, f_u.fu_llist)
> +		__fput(f);
>  }

#define llist_for_each_entry(pos, node, member)                         \
        for ((pos) = llist_entry((node), typeof(*(pos)), member);       \
             &(pos)->member != NULL;                                    \
             (pos) = llist_entry((pos)->member.next, typeof(*(pos)), member))

Now, think what happens after __fput() frees the damn thing.  In the
step of that loop, that is.

That kind of pattern (find next, do something with the current, proceed to
the next we'd found before) is a strong hint that this "do something"
might remove the thing from the list, or outright destroy it.  Both
file_table.c and namespace.c chunks are breaking exactly that kind of
places.

Please, don't do this kind of conversions blindly.  There _is_ another
iterating primitive for such places, but figuring out which one is right
is not something you can do without understanding what the code is doing.
And no, blind replacement of all such loops with llist_for_each_entry_safe,
just in case, is not a good idea either.