linux-kernel - Re: [PATCH] mm: shmem: do not call PageHWPoison on a ERR-page

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] [day] [month] [year] [list]

Message-ID: <CAHbLzkpSHTL=3ditbtS11sfhPe1GirK+eGML0=gidh2PcgzSgg@mail.gmail.com>
Date:   Sat, 13 Nov 2021 19:10:28 -0800
From:   Yang Shi <shy828301@...il.com>
To:     Linus Torvalds <torvalds@...ux-foundation.org>
Cc:     Jens Axboe <axboe@...nel.dk>, Arnd Bergmann <arnd@...db.de>,
        Ajay Garg <ajaygargnsit@...il.com>,
        Hugh Dickins <hughd@...gle.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Linux-MM <linux-mm@...ck.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] mm: shmem: do not call PageHWPoison on a ERR-page

On Sat, Nov 13, 2021 at 2:58 PM Linus Torvalds
<torvalds@...ux-foundation.org> wrote:
>
> On Sat, Nov 13, 2021 at 2:30 PM Yang Shi <shy828301@...il.com> wrote:
> >
> > The above snippet is actually ok since if *pagep returned via
> > shmem_getpage()'s parameter is not NULL, then ret is 0.
>
> That's a random implementation detail, and is not ok to rely on.
>
> It may or may not be true, and is not part of the rules of error handling.
>
> If a function returns an error, you shouldn't be looking at the other
> stuff it returned.
>
> Here's a very recent example of the same kind of problem:
>
>     https://lore.kernel.org/lkml/163663333331.414.639840290224641315.tip-bot2@tip-bot2/
>
> where people didn't actually look properly at the return value of the
> function, and instead looked at the page pointers that the function
> filled in.
>
> See? EXACT same logic. And completely buggy.

Yes, I agree it is too fragile to rely on.

>
> > When  shmem_getpage() returns error code, *pagep is NULL IIUC.
>
> No.
>
> When a function returns an error code, you check for the error code,
> and don't rely on weather the function then filled in other data (or
> left it alone, or whatever).
>
> So the code should
>
>  (a) check and handle error returns properly
>
>  (b) be legible
>
> That (b) basically means that if it's not entirely trivial (and none
> of this was entirely trivial), then when you get an error, you just
> deal with it right away. You return early, and undo anything you need
> to undo.
>
> You don't do "oh, let's keep that error, and then do something else
> that maybe also generates an error".
>
> That "don't handle the error directly" was why
> shmem_read_mapping_page_gfp() was buggy and would cause an oops.
>
> And while the shmem_write_begin() code migth not cause an oops, it had
> the same fundamental bad pattern.
>
> Error handling is where 99% of all problems occur. But that also means
> that you should do the obvious thing wrt error handling, and not have
> some crazy "if function X returned an error, it will have left the
> return array untouched" which may or may not be true.
>
> When a function returns an error code, you do error handling based on
> that code. Not on some random other state.

Thanks a lot for the thorough explanation. Preparing a new patch.

>
>                 Linus