netdev - Re: [PATCH 1/2] bpf: do not use KMALLOC_SHIFT

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]

Date:   Sat, 17 Dec 2016 00:39:17 +0100
From:   Michal Hocko <mhocko@...nel.org>
To:     Alexei Starovoitov <alexei.starovoitov@...il.com>
Cc:     linux-mm@...ck.org, Cristopher Lameter <cl@...ux.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Alexei Starovoitov <ast@...nel.org>, netdev@...r.kernel.org,
        Daniel Borkmann <daniel@...earbox.net>
Subject: Re: [PATCH 1/2] bpf: do not use KMALLOC_SHIFT_MAX

On Fri 16-12-16 15:23:42, Alexei Starovoitov wrote:
> On Fri, Dec 16, 2016 at 11:02:35PM +0100, Michal Hocko wrote:
> > On Fri 16-12-16 10:02:10, Alexei Starovoitov wrote:
> > > On Thu, Dec 15, 2016 at 05:47:21PM +0100, Michal Hocko wrote:
> > > > From: Michal Hocko <mhocko@...e.com>
> > > > 
> > > > 01b3f52157ff ("bpf: fix allocation warnings in bpf maps and integer
> > > > overflow") has added checks for the maximum allocateable size. It
> > > > (ab)used KMALLOC_SHIFT_MAX for that purpose. While this is not incorrect
> > > > it is not very clean because we already have KMALLOC_MAX_SIZE for this
> > > > very reason so let's change both checks to use KMALLOC_MAX_SIZE instead.
> > > > 
> > > > Cc: Alexei Starovoitov <ast@...nel.org>
> > > > Signed-off-by: Michal Hocko <mhocko@...e.com>
> > > 
> > > Nack until the patches 1 and 2 are reversed.
> > 
> > I do not insist on ordering. The thing is that it shouldn't matter all
> > that much. Or are you worried about bisectability?
> 
> This patch 1 strongly depends on patch 2 !
> Therefore order matters.
> The patch 1 by itself is broken.
> The commit log is saying
> '(ab)used KMALLOC_SHIFT_MAX for that purpose .. use KMALLOC_MAX_SIZE instead'
> that is also incorrect. We cannot do that until KMALLOC_MAX_SIZE is fixed.
> So please change the order

Yes, I agree that using KMALLOC_MAX_SIZE could lead to a warning with
the current ordering. Why that matters all that much is less clear to
me. The allocation would simply fail and you would return ENOMEM rather
than E2BIG. Does this really matter?

Anyway, as I've said, I do not really insist on the current ordering and
the will ask Andrew to reorder them. I am just really wondering about
such a strong pushback about something that barely matters. Or maybe I
am just missing your point and checking KMALLOC_MAX_SIZE without an
update would lead to a wrong behavior, user space breakage, crash or
anything similar.

> and fix the commit log to say that KMALLOC_MAX_SIZE
> is actually valid limit now.

KMALLOC_MAX_SIZE has always been the right limit. It's value has been
incorrect but that is to be fixed now. Using KMALLOC_SHIFT_MAX is simply
abusing an internal constant. So I am not sure what should be fixed in
the changelog.

-- 
Michal Hocko
SUSE Labs