linux-kernel - Re: [PATCH] [suggestion] mm/gup: avoid IS_ERR_OR

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZGiEEPXdAMnKqkqx@nvidia.com>
Date:   Sat, 20 May 2023 05:25:52 -0300
From:   Jason Gunthorpe <jgg@...dia.com>
To:     Lorenzo Stoakes <lstoakes@...il.com>
Cc:     Arnd Bergmann <arnd@...nel.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Arnd Bergmann <arnd@...db.de>,
        Catalin Marinas <catalin.marinas@....com>,
        Will Deacon <will@...nel.org>,
        Peter Zijlstra <peterz@...radead.org>,
        Ingo Molnar <mingo@...hat.com>,
        Arnaldo Carvalho de Melo <acme@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
        Jiri Olsa <jolsa@...nel.org>,
        Namhyung Kim <namhyung@...nel.org>,
        Ian Rogers <irogers@...gle.com>,
        Adrian Hunter <adrian.hunter@...el.com>,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
        linux-mm@...ck.org, linux-perf-users@...r.kernel.org
Subject: Re: [PATCH] [suggestion] mm/gup: avoid IS_ERR_OR_NULL

On Sat, May 20, 2023 at 06:19:37AM +0100, Lorenzo Stoakes wrote:
> On Fri, May 19, 2023 at 07:17:41PM -0300, Jason Gunthorpe wrote:
> > On Fri, May 19, 2023 at 03:51:51PM +0100, Lorenzo Stoakes wrote:
> > > Given you are sharply criticising the code I authored here, is it too much
> > > to ask for you to cc- me, the author on commentaries like this? Thanks.
> > >
> > > On Fri, May 19, 2023 at 11:39:13AM +0200, Arnd Bergmann wrote:
> > > > From: Arnd Bergmann <arnd@...db.de>
> > > >
> > > > While looking at an unused-variable warning, I noticed a new interface coming
> > > > in that requires the use of IS_ERR_OR_NULL(), which tends to indicate bad
> > > > interface design and is usually surprising to users.
> > >
> > > I am not sure I understand your reasoning, why does it 'tend to indicate
> > > bad interface design'? You say that as if it is an obvious truth. Not
> > > obvious to me at all.
> > >
> > > There are 3 possible outcomes from the function - an error, the function
> > > failing to pin a page, or it succeeding in doing so. For some of the
> > > callers that results in an error, for others it is not an error.
> >
> > No, there really isn't.
> >
> > Either it pins the page or it doesn't. Returning "NULL" to mean a
> > specific kind of failure was encountered is crazy.. Especially if we
> > don't document what that specific failure even was.
> >
> 
> It's not a specific kind of failure, it's literally "I didn't pin any
> pages" which a caller may or may not choose to interpret as a failure.

Any time gup fails it didn't pin any pages, that is the whole
point. All that is happening is some ill defined subset of gup errors
are returning 0 instead of an error code.

If we want to enable callers to ignore certain errors then we need to
return error codes with well defined meanings, have documentation what
errors are included and actually make it sane.

> That can be a reason for gup returning 0 but also if it you look at the
> main loop in __get_user_pages_locked(), if it can't find the VMA it will
> bail early, OR if the VMA flags are not as expected it'll bail early.

And how does that make any sense? Missing VMA should be EFAULT.
 
> caller differentiates between an error and not being able to pin -
> uprobe_write_opcode() - which treats failure to pin as a non-error state.

That looks like a bug since the function returns 0 on success but it
clearly didn't succeed.

> Also if we decided at some point to return -EIO as an error suddenly we
> would be treating an error state as not an error state in the proposed code
> which sounds like a foot gun.

No, this returning 0 on failure is a foot gun. Failing to pin a single
page is always an error, the only question is what sub reason caused
the error to happen. There is no third case where it is not an error.

Jason