linux-kernel - Re: [PATCH] userfaultfd: release page in error path to avoid BUG

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: <CAJHvVcj43SPX-fZ3y3brKQ3xkW2omOR=b_61M_GoGv8Dy+5G1g@mail.gmail.com>
Date:   Wed, 28 Apr 2021 14:26:03 -0700
From:   Axel Rasmussen <axelrasmussen@...gle.com>
To:     Peter Xu <peterx@...hat.com>
Cc:     Hugh Dickins <hughd@...gle.com>,
        Andrea Arcangeli <aarcange@...hat.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Lokesh Gidra <lokeshgidra@...gle.com>,
        Linux MM <linux-mm@...ck.org>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] userfaultfd: release page in error path to avoid BUG_ON

On Wed, Apr 28, 2021 at 2:24 PM Peter Xu <peterx@...hat.com> wrote:
>
> On Wed, Apr 28, 2021 at 02:03:05PM -0700, Hugh Dickins wrote:
>
> [...]
>
> > > > +                 put_page(*pagep);
> > > > +                 *pagep = NULL;
> > > > +         }
> > > >           goto out;
> > >
> > > All "goto out" in this functions looks weird as it returns directly... so if
> > > you're touching this after all, I suggest we do "return -ENOMEM" directly and
> > > drop the "ret = -ENOMEM".
> >
> > No strong feeling either way from me on that: whichever looks best
> > to you.  But I suspect the "ret = -ENOMEM" cannot be dropped,
> > because it's relied on further down too?
>
> Ah sorry I just noticed Axel didn't really touch that line.. :) So yeah please
> also feel free to keep it as is.
>
> If to drop it, "ret = -ENOMEM" can go as well, I think.. since all later errors
> should always reset variable "ret".

Although I can see a refactor which simplifies the error handling a
bit, my inclination is to leave it alone in this patch, since it's
trying to be a simple fix and especially considering we may need to
backport it as far back as 4.14.

But, I'll keep this feedback in mind and try to apply it as much as
possible to my other series which is significantly refactoring this
function already, so the end state ought to be as simple + consistent
error handling as possible.

>
> Thanks,
>
> --
> Peter Xu
>