linux-kernel - Re: [GIT PULL] iomap: new code for 5.4

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]

Message-ID: <20190919170148.GA8908@lst.de>
Date:   Thu, 19 Sep 2019 19:01:48 +0200
From:   Christoph Hellwig <hch@....de>
To:     Linus Torvalds <torvalds@...ux-foundation.org>
Cc:     "Darrick J. Wong" <djwong@...nel.org>,
        linux-fsdevel <linux-fsdevel@...r.kernel.org>,
        linux-xfs <linux-xfs@...r.kernel.org>,
        Dave Chinner <david@...morbit.com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Eric Sandeen <sandeen@...deen.net>,
        Christoph Hellwig <hch@....de>,
        Andreas Gruenbacher <agruenba@...hat.com>,
        Bob Peterson <rpeterso@...hat.com>,
        cluster-devel <cluster-devel@...hat.com>
Subject: Re: [GIT PULL] iomap: new code for 5.4

On Wed, Sep 18, 2019 at 06:31:29PM -0700, Linus Torvalds wrote:
> It seems to have come from "list_empty()", but the difference is that
> it actually makes sense to check for emptiness of a list outside
> whatever lock that protects the list. It can be one of those very
> useful optimizations where you don't even bother taking the lock if
> you can optimistically check that the list is empty.
> 
> But the same is _not_ true of an operation like "list_pop()". By
> definition, the list you pop something off has to be stable, so the
> READ_ONCE() makes no sense here.

Indeed.

> Anyway, if that was the only issue, I wouldn't care. But looking
> closer, the whole thing is just completely wrong.
> 
> All the users seem to do some version of this:
> 
>         struct list_head tmp;
> 
>         list_replace_init(&ioend->io_list, &tmp);
>         iomap_finish_ioend(ioend, error);
>         while ((ioend = list_pop_entry(&tmp, struct iomap_ioend, io_list)))
>                 iomap_finish_ioend(ioend, error);
> 
> which is completely wrong and pointless.
> 
> Why would anybody use that odd "list_pop()" thing in a loop, when what
> it really seems to just want is that bog-standard
> "list_for_each_entry_safe()"
> 
>         struct list_head tmp;
>         struct iomap_ioend *next;
> 
>         list_replace_init(&ioend->io_list, &tmp);
>         iomap_finish_ioend(ioend, error);
>         list_for_each_entry_safe(struct iomap_ioend, next, &tmp, io_list)
>                 iomap_finish_ioend(ioend, error);
> 
> which is not only the common pattern, it's more efficient and doesn't
> pointlessly re-write the list for each entry, it just walks it (and
> the "_safe()" part is because it looks up the next entry early, so
> that the entry that it's walking can be deleted).

That might be true for the current two cases that operate on a temporary
local list, but in general we have lots of cases where we operate on
lists that are not just local and where have to delete all the entries.

Sure, we could somehow let them dangle and then just do a INIT_LIST_HEAD
on the list later, but that is just asking for trouble down the road
when people actually use list_empty in the functions called in the loop.