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

[Yang Shi <shy828301@gmail.com>]

On Sat, Nov 13, 2021 at 2:58 PM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> On Sat, Nov 13, 2021 at 2:30 PM Yang Shi <shy828301@gmail.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