Re: [PATCH] fix sys cpumap for > 352 NR_CPUS

[Paul Jackson]

> +	BUILD_BUG_ON(NR_CPUS/4 > PAGE_SIZE/2);

Interesting.  This would be the only use in the entire kernel of
BUILD_BUG_ON().

An alternative mechanism would be:

    #if(badthing) ... #error "darn" ... #endif

There are over 300 such constructs in the kernel.  And it has the
advantage of providing an accurate source line number and a specifiable
string.

My preference when checking limits is to check for the exact limit.
Adding fuzz only serves to disguise details.  Whether coding correctly,
or screwing up, best to do so with clarity and precision.

Given all that, how about:

    #if ALIGN(NR_CPUS,32)*9/32 > PAGE_SIZE
    #error "Need 9 bytes space per 32 CPUs in PAGE_SIZE buffer"
    #endif

==

I also could be tempted to remove BUILD_BUG_ON() from kernel.h, and
replace it with a comment:

    /* BUILD_BUG_ON() obsolete - consider using #if ... #error ... #endif */

==

> +	len = cpumask_scnprintf(buf, -1UL, mask);

Why not instead:

> +	len = cpumask_scnprintf(buf, PAGE_SIZE-1, mask);

I see no sense in giving cpumask_scnprintf() license to write past the
end of the buffer, independent of any build-time checks (the -1 is for
the trailing newline).  And since the contract says "PAGE_SIZE", we
should code exactly to that value "PAGE_SIZE", for clarity as to our
understandings.  Once again - I hate fuzz ;).

==

> +	BUG_ON(count > PAGE_SIZE);

Only 'BUG_ON' ??  We have in hand almost certain proof of just
having scrogged kernel memory.  Time to panic, no?

==

What are we going to do about the removal of the node_dev->cpumap field,
and changing this node_read_cpumap() routine to display instead the
value of node_to_cpumask(node_dev->sysdev.id)?

Should I do it, or you?  Should it presume your patch above, or collide
with it, or replace and extend it?

Since I am most impressed with your abilities, since you doubt my
abilities, and since I'm a lazy s.o.b., you're welcome to it.  Or if you
prefer to ask me, that's fine.  Seems to me this should be two patches -
the one discussed above to limit how many bytes cpumask_scnprintf can
posit, and a second to nuke node_dev->cpumap.  The second patch would
depend on the first, for the trivial reason that they collide on some of
the same code.

-- 
                          I won't rest till it's the best ...
                          Programmer, Linux Scalability
                          Paul Jackson <pj@xxxxxxx> 1.650.933.1373
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/