Re: [PATCH 1/3] ib core: Make device counter infrastructure dynamic

[Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 2366 bytes --]

On 05/16/2016 01:04 PM, Christoph Lameter wrote:
> On Mon, 16 May 2016, Doug Ledford wrote:
> 
>>>> Secondly, do *not* use a BUG_ON in this patch.  I saw at least two of
>>>> them.  There is nothing in this patch so serious that we should crash
>>>> the kernel.  Any failure here is something we can work around and keep
>>>> running.
>>>
>>> WARN_ON_ONCE then?
>> Yep.
> 
> BUILD_BUG_ON works even better once we have the enum. Revised patch
> follows:

There's still one BUG_ON.  See below...

> +static struct attribute_group *create_protocol_stats(struct ib_device *device,
> +						     struct kobject *kobj,
> +						     u8 port) {
> +	struct attribute_group *ag;
> +	struct rdma_protocol_stats stats = {0};

We allocated our struct on the stack.  We have a static length counter
array in this struct.

> +	u32 counters;
> +	u32 i;
> +	int ret;
> +
> +	ret = device->get_protocol_stats(device, &stats, port);

We then pass our struct to get_protocol_stats to fill out.

> +
> +	if (ret || !stats.name)
> +		return NULL;
> +
> +	ag = kzalloc(sizeof(*ag), GFP_KERNEL);
> +	if (!ag)
> +		return NULL;
> +
> +	ag->name = stats.dirname;
> +
> +	for (counters = 0; stats.name[counters]; counters++)
> +		;

We count how many names there are...

> +
> +	BUG_ON(counters > MAX_NR_PROTOCOL_STATS);

And we BUG_ON if there are more names than we allocated static counters
in our struct.  However, by now, it's too late.  The device's
get_protocol_stats function has already likely written beyond the end of
our array and smashed our stack.  Let's fix this another way entirely...


> +	/* Emsure we provided all values */

Typo in comment...

> +	BUILD_BUG_ON(NR_COUNTERS != (sizeof(names) / sizeof(char *)) - 1);

Here's where we ant to fix the problem.  We need a second BUILD_BUG_ON
that reads:

	BUILD_BUG_ON(NR_COUNTERS > MAX_NR_PROTOCOL_STATS);

Then we can safely reduce the size of the MAX_NR_PROTOCOL_STATS and know
that if we exceed our allocation, the build will fail and we can
increase it accordingly.  So in all of the get_stats routines we want
both build time checks to make sure that the counter structs are sized
appropriately and the name/enum list lengths match.


-- 
Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
              GPG KeyID: 0E572FDD



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 884 bytes --]