From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756003AbeD3UQQ (ORCPT ); Mon, 30 Apr 2018 16:16:16 -0400 Received: from bombadil.infradead.org ([198.137.202.133]:55126 "EHLO bombadil.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755884AbeD3UQO (ORCPT ); Mon, 30 Apr 2018 16:16:14 -0400 Date: Mon, 30 Apr 2018 13:16:07 -0700 From: Matthew Wilcox To: Kees Cook Cc: Julia Lawall , Andrew Morton , Matthew Wilcox , Linux-MM , LKML , Kernel Hardening , cocci@systeme.lip6.fr, Himanshu Jha Subject: Re: [PATCH 2/2] mm: Add kvmalloc_ab_c and kvzalloc_struct Message-ID: <20180430201607.GA7041@bombadil.infradead.org> References: <20180308025812.GA9082@bombadil.infradead.org> <20180308230512.GD29073@bombadil.infradead.org> <20180313183220.GA21538@bombadil.infradead.org> <20180429203023.GA11891@bombadil.infradead.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.9.2 (2017-12-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, Apr 30, 2018 at 12:02:14PM -0700, Kees Cook wrote: > On Sun, Apr 29, 2018 at 1:30 PM, Matthew Wilcox wrote: > > On Sun, Apr 29, 2018 at 09:59:27AM -0700, Kees Cook wrote: > >> Did this ever happen? > > > > Not yet. I brought it up at LSFMM, and I'll repost the patches soon. > > > >> I'd also like to see kmalloc_array_3d() or > >> something that takes three size arguments. We have a lot of this > >> pattern too: > >> > >> kmalloc(sizeof(foo) * A * B, gfp...) > >> > >> And we could turn that into: > >> > >> kmalloc_array_3d(sizeof(foo), A, B, gfp...) > > > > Are either of A or B constant? Because if so, we could just use > > kmalloc_array. If not, then kmalloc_array_3d becomes a little more > > expensive than kmalloc_array because we have to do a divide at runtime > > instead of compile-time. that's still better than allocating too few > > bytes, of course. > > Yeah, getting the order of the division is nice. Some thoughts below... > > > > > I'm wondering how far down the abc + ab + ac + bc + d rabbit-hole we're > > going to end up going. As far as we have to, I guess. > > Well, the common patterns I've seen so far are: > > a > ab > abc > a + bc > ab + cd > > For any longer multiplications, I've only found[1]: > > drivers/staging/rtl8188eu/os_dep/osdep_service.c: void **a = > kzalloc(h * sizeof(void *) + h * w * size, GFP_KERNEL); That's pretty good, although it's just an atrocious vendor driver and it turns out all of those things are constants, and it'd be far better off with just declaring an array. I bet they used to declare one on the stack ... > At the end of the day, though, I don't really like having all these > different names... > > kmalloc(), kmalloc_array(), kmalloc_ab_c(), kmalloc_array_3d() > > with their "matching" zeroing function: > > kzalloc(), kcalloc(), kzalloc_ab_c(), kmalloc_array_3d(..., gfp | __GFP_ZERO) Yes, it's not very regular. > For the multiplication cases, I wonder if we could just have: > > kmalloc_multN(gfp, a, b, c, ...) > kzalloc_multN(gfp, a, b, c, ...) > > and we can replace all kcalloc() users with kzalloc_mult2(), all > kmalloc_array() users with kmalloc_mult2(), the abc uses with > kmalloc_mult3(). I'm reluctant to do away with kcalloc() as it has the obvious heritage from user-space calloc() with the addition of GFP flags. > That said, I *do* like kmalloc_struct() as it's a very common pattern... Thanks! And way harder to misuse than kmalloc_ab_c(). > Or maybe, just leave the pattern in the name? kmalloc_ab(), > kmalloc_abc(), kmalloc_ab_c(), kmalloc_ab_cd() ? > > Getting the constant ordering right could be part of the macro > definition, maybe? i.e.: > > static inline void *kmalloc_ab(size_t a, size_t b, gfp_t flags) > { > if (__builtin_constant_p(a) && a != 0 && \ > b > SIZE_MAX / a) > return NULL; > else if (__builtin_constant_p(b) && b != 0 && \ > a > SIZE_MAX / b) > return NULL; > > return kmalloc(a * b, flags); > } Ooh, if neither a nor b is constant, it just didn't do a check ;-( This stuff is hard. > (I just wish C had a sensible way to catch overflow...) Every CPU I ever worked with had an "overflow" bit ... do we have a friend on the C standards ctte who might figure out a way to let us write code that checks it? > -Kees > > [1] git grep -E 'alloc\([^,]+[^(]\*[^)][^,]+[^(]\*[^)][^,]+[^(]\*[^)][^,]+,' I'm impressed, but it's not going to catch veryLongPointerNameThatsMeaningfulToMe = kmalloc(initialSize + numberOfEntries * entrySize + someOtherThing * yourMum, GFP_KERNEL);