Received: by 10.192.165.148 with SMTP id m20csp769756imm; Fri, 4 May 2018 20:49:36 -0700 (PDT) X-Google-Smtp-Source: AB8JxZpdWu564KY6MXw//pGJ7rFjsxS5CdV/sXJ4tgt+4sZn2I3mmIrfTfKe/twVCeMIjGJQoO+i X-Received: by 2002:a63:6b43:: with SMTP id g64-v6mr16786799pgc.337.1525492176042; Fri, 04 May 2018 20:49:36 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1525492176; cv=none; d=google.com; s=arc-20160816; b=jFT2MMZEbF/H5NhHjvzhHtVNplmZ9fexryBHOz46Qo8hOHahpIBdVPRXWN6vcQj7Dp Pp8c2T/gyEvjh3cZLvIXoQRcsTn+R/kT3kd2QIyHqILjfv2IdUu7AbPJfDFy4OrbpTc/ opaR4mSMPTvSwRqArlV+cmLMK8z3yg1hoys5pkc5a+hQ/XKnXT7NzdT3yxicHTOuEnUK d/8p1F6BJ+RJOzeGdKrWObRgWRxmC3AqkNu22Fn8p1VNScOk9x4YpJQVTKloTjshSq0i PDF0gCfBBIiTI9R38iXsqbFd+mYOrndW9WrlSPzZ+i0ssARPKvU9fiQDsv3nl1LZwJSB 6KwQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:user-agent:in-reply-to :content-disposition:mime-version:references:message-id:subject:cc :to:from:date:dkim-signature:arc-authentication-results; bh=8sVRBw2HLtZ+sczFmuhcTkPRui122Vx+L9U8ptHk8AM=; b=b+f2YyAN2ZSmVI7/qa2Y5j0em9WGMILhLIF46kYdIaohHR68rWMzYlIXZbIi4ThI95 UGda1gQjcKDDo9kcGjs6NL+mGuCKDu6byCJVuHqVAn7c8XzJuxfsWjoFFfT+4Z3YKD3q HDeP1EAkhy1L7GRYKzKVmAdQC9nmbz0vrnnjJll6XqRGuRzXmll2N6dcwq+W0GDMntCL YM6cHyBaW3rNE+evuN2Rj5OQNh7PXgHg4WSgfQTZD7J2KF/INGwAp9eOZcdSD4ILiHLx cBrxrOxsZ2N3Ja8p2uA38xm/L7dEf08D0jjgjJUfdTTJKk2NeOcQ+05vMKiRtbpUw2Ng L/Ng== ARC-Authentication-Results: i=1; mx.google.com; dkim=neutral (body hash did not verify) header.i=@infradead.org header.s=bombadil.20170209 header.b=XqYlcs8L; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id f91-v6si4720395plb.510.2018.05.04.20.49.21; Fri, 04 May 2018 20:49:36 -0700 (PDT) Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67; Authentication-Results: mx.google.com; dkim=neutral (body hash did not verify) header.i=@infradead.org header.s=bombadil.20170209 header.b=XqYlcs8L; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752072AbeEEDqx (ORCPT + 99 others); Fri, 4 May 2018 23:46:53 -0400 Received: from bombadil.infradead.org ([198.137.202.133]:34102 "EHLO bombadil.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751652AbeEEDqw (ORCPT ); Fri, 4 May 2018 23:46:52 -0400 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=infradead.org; s=bombadil.20170209; h=In-Reply-To:Content-Type:MIME-Version :References:Message-ID:Subject:Cc:To:From:Date:Sender:Reply-To: Content-Transfer-Encoding:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Id: List-Help:List-Unsubscribe:List-Subscribe:List-Post:List-Owner:List-Archive; bh=eCPYkElxmb6JvL7i02FpmZp88Y40K3hue1lU1c5YeR0=; b=XqYlcs8LiJ36ros6VLAU3vV7J CvXR+DpI4MzzdHARk8XfBYqmBBiIZ2W0/3jiWZPlX+w0lrgslvh4Q/m9hzwv7WhmrlYfgW+ERiRy8 RWZYo1erkCzVvu8Ime7yCYZafiqE45BypjzY17qZuiJ9rVIHaY1X5YrEQ87cHK2xz7nenCMCOSrgt daEHybWAZvvN5PRF24oHJgAfgevpMTOKSLu82mLtLSNu2z8jZQF8PfcdQUQctMPpBkyy261dW2AYl fba/xWw47kjudhvmIHHXaliKMCvMSQbj2s//SWTI5mJV+7oZowKYWBpcVLjcLdK0QWHTthIp48TYl tq4LEokMQ==; Received: from willy by bombadil.infradead.org with local (Exim 4.90_1 #2 (Red Hat Linux)) id 1fEo9n-0002j4-4U; Sat, 05 May 2018 03:46:47 +0000 Date: Fri, 4 May 2018 20:46:46 -0700 From: Matthew Wilcox To: Kees Cook Cc: Matthew Wilcox , Linux-MM , LKML , Rasmus Villemoes Subject: Re: *alloc API changes Message-ID: <20180505034646.GA20495@bombadil.infradead.org> References: 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 Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, May 04, 2018 at 06:08:23PM -0700, Kees Cook wrote: > The number of permutations for our various allocation function is > rather huge. Currently, it is: > > system or wrapper: > kmem_cache_alloc, kmalloc, vmalloc, kvmalloc, devm_kmalloc, > dma_alloc_coherent, pci_alloc_consistent, kmem_alloc, f2fs_kvalloc, > and probably others I haven't found yet. dma_pool_alloc, page_frag_alloc, gen_pool_alloc, __alloc_bootmem_node, cma_alloc, quicklist_alloc (deprecated), mempool_alloc and if you're counting f2fs_*alloc, there's a metric tonne of *alloc wrappers out there. > allocation method (not all available in all APIs): > regular (kmalloc), zeroed (kzalloc), array (kmalloc_array), zeroed > array (kcalloc) ... other initialiser (kmem_cache_alloc) > I wonder if we might be able to rearrange our APIs for the general > case and include a common "kitchen sink" API for the less common > options. I.e. why do we have an entire set of _node() APIs, 2 sets for > zeroing (kzalloc, kcalloc), etc. I'd love it if we had a common pattern for these things. A regular API appeals to me (I blame those RISC people in my formative years). > kmalloc()-family was meant to be a simplification of > kmem_cache_alloc(). That's a little revisionist ;-) We had kmalloc before we had the slab allocator (kernel 1.2, I think?). But I see your point, and that's certainly how it's implemented these days. > vmalloc() duplicated the kmalloc()-family, and > kvmalloc() does too. Then we have "specialty" allocators (devm, dma, > pci, f2fs, xfs's kmem) that have subsets and want to perform other > actions around the base allocators or have their own entirely (e.g. > dma). > > Instead of all the variations, it seems like we just want a per-family > alloc() and alloc_attrs(), where alloc_attrs() could handle the less > common stuff (e.g. gfp, zero, node). > > kmalloc(size, GFP_KERNEL) > becomes a nice: > kmalloc(size) I got shot down for proposing adding #define malloc(x) kmalloc(x, GFP_KERNEL) on the grounds that driver writers will then use malloc in interrupt context. So I think our base version has to be foo_alloc(size, gfp_t). > But this doesn't solve the multiplication overflow case at all, which > is my real goal. Trying to incorporate some of the ideas from other > threads, maybe we could have a multiplication helper that would > saturate and the allocator would see that as a signal to return NULL? > e.g.: > > inline size_t mult(size_t a, size_t b) > { > if (b != 0 && a >= SIZE_MAX / b) > return SIZE_MAX; > return a * b; > } > (really, this kind of helper should be based on Rasmus's helpers which > do correct type handling) Right, I was thinking: static inline size_t mul_ab(size_t a, size_t b) { #if COMPILER_SUPPORTS_OVERFLOW unsigned long c; if (__builtin_mul_overflow(a, b, &c)) return SIZE_MAX; return c; #else if (b != 0 && a >= SIZE_MAX / b) return SIZE_MAX; return a * b; #endif } > void *kmalloc(size_t size) > { > if (size == SIZE_MAX) > return NULL; > kmalloc_attrs(size, GFP_KERNEL, ALLOC_NOZERO, NUMA_NO_NODE); > } You don't need the size check here. We have the size check buried deep in alloc_pages (look for MAX_ORDER), so kmalloc and then alloc_pages will try a bunch of paths all of which fail before returning NULL. > then we get: > var = kmalloc(mult(num, sizeof(*var))); > > we could drop the *calloc(), *zalloc(), and *_array(), leaving only > *alloc() and *alloc_attrs() for all the allocator families. > > I honestly can't tell if this is worse than doing all the family > conversions to *calloc() and *_array() for the 1000ish instances of > 2-factor products used for size arguments in the *alloc() functions. > We could still nest them for the 3-factor ones? > var = kmalloc(multi(row, mult(column, sizeof(*var)))); > > But now we're just pretending to be LISP. I'd rather have a mul_ab(), mul_abc(), mul_ab_add_c(), etc. than nest calls to mult(). > And really, I'd like to keep the nicer *alloc_struct() with all its > type checking. But then do we do *zalloc_struct(), > *alloc_struct_node(), etc, etc? Nono, Linus had the better proposal, struct_size(p, member, n). > Bleh. C sucks for this. Ooh, we could instantiate classes and ... yeah, no, not C++. We *could* abuse the C preprocessor to autogenerate every variant, but I hate that because you can't grep for it. One of the problems with having the single-argument foo_alloc be a static inline for foo_alloc_attrs is that you then have to marshall four arguments for the call instead of just one. I would have two exported symbols for each variant.