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

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:   Mon, 13 Feb 2017 05:43:26 +0000
From:   Al Viro <viro@...IV.linux.org.uk>
To:     Byungchul Park <byungchul.park@....com>
Cc:     peterz@...radead.org, mingo@...nel.org, koverstreet@...gle.com,
        neilb@...e.de, nab@...ux-iscsi.org, ying.huang@...el.com,
        oleg@...hat.com, asias@...hat.com, shli@...nel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] llist: Don't reinvent the wheel but use existing llist
 API

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.