Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210428180109.293606-1-axelrasmussen@google.com>
 <20210428183943.GH6584@xz-x1> <alpine.LSU.2.11.2104281338380.9009@eggly.anvils>
 <20210428212430.GA6462@xz-x1>
In-Reply-To: <20210428212430.GA6462@xz-x1>
From:   Axel Rasmussen <axelrasmussen@google.com>
Date:   Wed, 28 Apr 2021 14:26:03 -0700
Message-ID: <CAJHvVcj43SPX-fZ3y3brKQ3xkW2omOR=b_61M_GoGv8Dy+5G1g@mail.gmail.com>
Subject: Re: [PATCH] userfaultfd: release page in error path to avoid BUG_ON
To:     Peter Xu <peterx@redhat.com>
Cc:     Hugh Dickins <hughd@google.com>,
        Andrea Arcangeli <aarcange@redhat.com>,
        Andrew Morton <akpm@linux-foundation.org>,
        Lokesh Gidra <lokeshgidra@google.com>,
        Linux MM <linux-mm@kvack.org>,
        LKML <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Wed, Apr 28, 2021 at 2:24 PM Peter Xu <peterx@redhat.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
>