On Fri, May 4, 2018 at 8:35 AM, Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx> wrote: > On Fri, May 4, 2018 at 3:14 AM Matthew Wilcox <willy@xxxxxxxxxxxxx> wrote: > >> > In fact, the conversion I saw was buggy. You can *not* convert a > GFP_ATOMIC >> > user of kmalloc() to use kvmalloc. > >> Not sure which conversion you're referring to; not one of mine, I hope? > > I was thinking of the coccinelle patch in this thread, but just realized > that that actually only did it for GFP_KERNEL, so I guess it would work, > apart from the "oops, now it doesn't enforce the kmalloc limits any more" > issue. Just to be clear: the Coccinelle scripts I'm building aren't doing a kmalloc -> kvmalloc conversion. I'm just removing all the 2-factor multiplication and replacing it with the appropriate calls to the allocator family's *calloc or *alloc_array(). This will get us to the place where we can do all the sanity-checking in the allocator functions (whatever that checking ends up being). As it turns out, though, we have kind of a lot of allocator families. Some are wrappers, like devm_*alloc(), etc. All that said, the overwhelming majority of *alloc() multiplications are just "count * sizeof()". It really feels like everything should just be using a new *alloc_struct() which can do the type checking, etc, etc, but we can get there. The remaining "count * size" are a minority and could be dealt with some other way. >> > - that divide is really really expensive on many architectures. > >> 'c' and 'size' are _supposed_ to be constant and get evaluated at >> compile-time. ie you should get something like this on x86: > > I guess that willalways be true of the 'kvzalloc_struct() version that > will always use a sizeof(). I was more thinking of any bare kvalloc_ab_c() > cases, but maybe we'd discourage that to ever be used as such? Yeah, bare *alloc_ab_c() is not great. Perhaps a leading "__" can hint to that? > Because we definitely have things like that, ie a quick grep finds > > f = kmalloc (sizeof (*f) + size*num, GFP_KERNEL); > > where 'size' is not obviously a constant. There may be others, but I didn't > really try to grep any further. > > Maybe they aren't common, and maybe the occasional divide doesn't matter, > but particularly if we use scripting to then catch and convert users, I > really hate the idea of "let's introduce something that is potentially much > more expensive than it needs to be". Yup: I'm not after that either. I just want to be able to get at the multiplication factors before they're multiplied. :) > (And the automated coccinelle scripting it's also something where we must > very much avoid then subtly lifting allocation size limits) Agreed. I think most cases are already getting lifted to size_t due to the sizeof(). It's the "two variables" cases I want to double-check. Another completely insane idea would be to have a macro that did type size checking and would DTRT, but with all the alloc families, it looks nasty. This is all RFC stage, as far as I'm concerned. Fun example: devm_kzalloc(dev, sizeof(...) * num, gfp...) $ git grep 'devm_kzalloc([^,]*, *sizeof([^,]*,' | egrep '\* *sizeof|\) *\*' | wc -l 88 some are constants: drivers/video/fbdev/au1100fb.c: devm_kzalloc(&dev->dev, sizeof(u32) * 16, GFP_KERNEL); but many aren't: sound/soc/generic/audio-graph-card.c: dai_link = devm_kzalloc(dev, sizeof(*dai_link) * num, GFP_KERNEL); While type-checking on the non-sizeof factor would let us know if it was safe, so would the division, and most of those could happen at compile time. It's the size_t variables that we want to catch. So, mainly I'm just trying to get the arguments reordered (for a compile-time division) into the correct helpers so the existing logic can do the right thing, and only for 2-factor products. After that, then I'm hoping to tackle the multi-factor products, of which the *alloc_struct() helper seems to cover the vast majority of the remaining cases. -Kees -- Kees Cook Pixel Security
