LKML Archive on
help / color / mirror / Atom feed
From: Matthew Wilcox <>
To: Kees Cook <>
Cc: Matthew Wilcox <>,
	Linux-MM <>,
	LKML <>,
	Rasmus Villemoes <>
Subject: Re: *alloc API changes
Date: Fri, 4 May 2018 20:46:46 -0700	[thread overview]
Message-ID: <> (raw)
In-Reply-To: <>

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)
	unsigned long c;
	if (__builtin_mul_overflow(a, b, &c))
		return SIZE_MAX;
	return c;
	if (b != 0 && a >= SIZE_MAX / b)
		return SIZE_MAX;
	return a * b;

> 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.

  reply	other threads:[~2018-05-05  3:46 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-05-05  1:08 Kees Cook
2018-05-05  3:46 ` Matthew Wilcox [this message]
2018-05-05  4:24   ` Kees Cook
2018-05-07 11:39     ` Matthew Wilcox
2018-05-07 16:03       ` Kees Cook
2018-05-07 20:19         ` Matthew Wilcox
2018-05-07 20:27           ` Kees Cook
2018-05-07 20:49             ` Matthew Wilcox
2018-05-07 21:15               ` Kees Cook
2018-05-07 21:48             ` John Johansen
2018-05-07 21:41     ` Rasmus Villemoes
2018-05-07 22:56       ` Kees Cook
2018-05-05  4:30   ` Matthew Wilcox

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \ \ \ \ \ \ \ \
    --subject='Re: *alloc API changes' \

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).