[v2,2/8] mm: Add kvmalloc_ab_c and kvzalloc_struct

Commit Message

Matthew Wilcox Feb. 15, 2018, 3:40 a.m. UTC

On Wed, Feb 14, 2018 at 03:58:33PM -0800, Andrew Morton wrote:
> On Wed, 14 Feb 2018 13:12:03 -0800 Matthew Wilcox <[e-mail]> wrote:
> > If C macros had decent introspection, I'd like it to be:
> > 
> > 	sev = kvzalloc_struct(elems, GFP_KERNEL);
> > 
> > and have the macro examine the structure pointed to by 'sev', check
> > the last element was an array, calculate the size of the array element,
> > and call kvzalloc_ab_c.  But we don't live in that world, so I have to
> > get the programmer to tell me the structure and the name of the last
> > element in it.
> 
> hm, bikeshedding fun.

Heck, yeah!  Fun!

> struct foo {
> 	whatevs;
> 	struct bar[0];
> }
> 
> 
> 	struct foo *a_foo;
> 
> 	a_foo = kvzalloc_struct_buf(foo, bar, nr_bars);
> 
> and macro magic will insert the `struct' keyword.  This will help to
> force a miscompile if inappropriate types are used for foo and bar.
> 
> Problem is, foo may be a union(?) and bar may be a scalar type.  So
> 
> 	a_foo = kvzalloc_struct_buf(struct foo, struct bar, nr_bars);
> 
> or, of course.
> 
> 	a_foo = kvzalloc_struct_buf(typeof(*a_foo), typeof(a_foo->bar[0]),
> 				    nr_bars);
> 
> or whatever.

I think that's actually *less* checking than the option I presented here.
My version has the compiler check:
1. You assigned the pointer to the allocated memory
2. ... to a pointer of compatible type with p
3. p is a pointer
4. member is a member of the type p points to
5. member is an array type

Your version doesn't check point 4, and it'd be easy to get it wrong like this:

struct quux {
	int n;
	struct foo foos[];
} *p = kvmalloc_struct(struct quux, struct foo *, n);

or vice-versa:

struct quux {
	int n;
	struct foo *foos[];
} *p = kvmalloc_struct(struct quux, struct foo, n);

What is it that you don't like about my version?  Is it passing the
uninitialised pointer to a macro that looks like a function?  Because
we do this all the time:

	struct foo *p = kmalloc(sizeof(*p), GFP_KERNEL);

> The basic idea is to use the wrapper macros to force compile errors if
> these things are misused.

Right.  Although passing the pointer in lets us work this magic on an
unnamed struct.  Like this mess...


> > +static inline __must_check
> > +void *kvmalloc_ab_c(size_t n, size_t size, size_t c, gfp_t gfp)
> > +{
> > +	if (size != 0 && n > (SIZE_MAX - c) / size)
> > +		return NULL;
> > +
> > +	return kvmalloc(n * size + c, gfp);
> > +}
> 
> Can we please avoid the single-char identifiers?
> 
> void *kvmalloc_ab_c(size_t n_elems, size_t elem_size, size_t header_size,
> 		    gfp_t gfp);
> 
> makes the code so much more readable.

I was naming for consistency:

static inline void *kvmalloc_array(size_t n, size_t size, gfp_t flags)
{
        if (size != 0 && n > SIZE_MAX / size)
                return NULL;

I'll happily change 'c' to 'hdr_size' though.

Comments

Matthew Wilcox Feb. 15, 2018, 5:36 p.m. UTC | #1

On Wed, Feb 14, 2018 at 07:40:50PM -0800, Matthew Wilcox wrote:
> > 	a_foo = kvzalloc_struct_buf(struct foo, struct bar, nr_bars);
> > 
> > or, of course.
> > 
> > 	a_foo = kvzalloc_struct_buf(typeof(*a_foo), typeof(a_foo->bar[0]),
> > 				    nr_bars);
> > 
> > or whatever.

This version works, although it's more typing in the callers:

-               attr_group = kvzalloc_struct(attr_group, attrs, i + 1,
+               attr_group = kvzalloc_struct(typeof(*attr_group), attrs, i + 1,
                                                                GFP_KERNEL);
...
-       dev_dax = kvzalloc_struct(dev_dax, res, count, GFP_KERNEL);
+       dev_dax = kvzalloc_struct(struct dev_dax, res, count, GFP_KERNEL);
...
-#define kvzalloc_struct(p, member, n, gfp)                             \
-       (typeof(p))kvzalloc_ab_c(n,                                     \
-               sizeof(*(p)->member) + __must_be_array((p)->member),    \
-               offsetof(typeof(*(p)), member), gfp)
+#define kvzalloc_struct(s, member, n, gfp) ({                          \
+       s *__p;                                                         \
+       (s *)kvzalloc_ab_c(n,                                           \
+               sizeof(*(__p)->member) + __must_be_array((__p)->member),\
+               offsetof(s, member), gfp);                              \
+})

Gives all the same checking as the current version, and doesn't involve
passing an uninitialised pointer to the macro.

It also looks pretty similar:

	p = kvzalloc(sizeof(*p), GFP_KERNEL);
	p = kvzalloc_struct(typeof(*p), array, count, GFP_KERNEL);
	p = kvzalloc_array(sizeof(*p), count);

Patch

diff --git a/arch/x86/events/intel/uncore.c b/arch/x86/events/intel/uncore.c
index 7874c980d569..5cd3e127bea8 100644
--- a/arch/x86/events/intel/uncore.c
+++ b/arch/x86/events/intel/uncore.c
@@ -792,7 +792,7 @@  static void uncore_type_exit(struct intel_uncore_type *type)
 		kfree(type->pmus);
 		type->pmus = NULL;
 	}
-	kfree(type->events_group);
+	kvfree(type->events_group);
 	type->events_group = NULL;
 }
 
@@ -805,8 +805,6 @@  static void uncore_types_exit(struct intel_uncore_type **types)
 static int __init uncore_type_init(struct intel_uncore_type *type, bool setid)
 {
 	struct intel_uncore_pmu *pmus;
-	struct attribute_group *attr_group;
-	struct attribute **attrs;
 	size_t size;
 	int i, j;
 
@@ -831,21 +829,24 @@  static int __init uncore_type_init(struct intel_uncore_type *type, bool setid)
 				0, type->num_counters, 0, 0);
 
 	if (type->event_descs) {
+		struct {
+			struct attribute_group group;
+			struct attribute *attrs[];
+		} *attr_group;
 		for (i = 0; type->event_descs[i].attr.attr.name; i++);
 
-		attr_group = kzalloc(sizeof(struct attribute *) * (i + 1) +
-					sizeof(*attr_group), GFP_KERNEL);
+		attr_group = kvzalloc_struct(attr_group, attrs, i + 1,
+								GFP_KERNEL);
 		if (!attr_group)
 			goto err;
 
-		attrs = (struct attribute **)(attr_group + 1);
-		attr_group->name = "events";
-		attr_group->attrs = attrs;
+		attr_group->group.name = "events";
+		attr_group->group.attrs = attr_group->attrs;
 
 		for (j = 0; j < i; j++)
-			attrs[j] = &type->event_descs[j].attr.attr;
+			attr_group->attrs[j] = &type->event_descs[j].attr.attr;
 
-		type->events_group = attr_group;
+		type->events_group = &attr_group->group;
 	}
 
 	type->pmu_group = &uncore_pmu_attr_group;