Re: [PATCH] arch/tile: updates to hardwall code from community feedback.

[Paul Mundt <lethal@linux-sh.org>]

On Fri, Jul 02, 2010 at 11:45:18AM -0400, Chris Metcalf wrote:
>  #define for_each_hardwall_task_safe(pos, n, head) \
> -	hardwall_for_each_entry_safe(pos, n, head, thread.hardwall_list)
> +	list_for_each_entry_safe(pos, n, head, thread.hardwall_list)
>  
If you've killed off the rest of the wrappers due to them not really
having to do anything special, you could do the same for this one, too.

>  static struct hardwall_info *hardwall_create(
> -	size_t size, const unsigned long __user *bits)
> +	size_t size, const unsigned char __user *bits)
>  {
>  	struct hardwall_info *iter, *rect;
>  	struct cpumask mask;
>  	unsigned long flags;
> -	int rc, cpu, maxcpus = smp_height * smp_width;
> +	int rc;
>  
> -	/* If "size" is not a multiple of long, it's invalid. */
> -	if (size % sizeof(long))
> +	/* Reject crazy sizes out of hand, a la sys_mbind(). */
> +	if (size > PAGE_SIZE)
>  		return ERR_PTR(-EINVAL);
>  
Does it even make any sense to accept > sizeof(struct cpumask) ? Your
case below obviously handles this by making sure the rest of the bits are
zeroed, but that still seems a bit excessive. If anything, the
sizeof(struct cpumask) should be your worst case (or just rejected out of
hand), and you could use cpumask_size() for everything else.

> -	/* Validate that all the bits we are given fit in our coordinates. */
> -	cpumask_clear(&mask);
> -	for (cpu = 0; size > 0; ++bits, size -= sizeof(long)) {
> -		int i;
> -		unsigned long m;
> -		if (get_user(m, bits) != 0)
> -			return ERR_PTR(-EFAULT);
> -		for (i = 0; i < BITS_PER_LONG; ++i, ++cpu)
> -			if (m & (1UL << i)) {
> -				if (cpu >= maxcpus)
> -					return ERR_PTR(-EINVAL);
> -				cpumask_set_cpu(cpu, &mask);
> -			}
> +	/* Copy whatever fits into a cpumask. */
> +	if (copy_from_user(&mask, bits, min(sizeof(struct cpumask), size)))
> +		return ERR_PTR(-EFAULT);
> +
> +	/*
> +	 * If the size was short, clear the rest of the mask;
> +	 * otherwise validate that the rest of the user mask was zero
> +	 * (we don't try hard to be efficient when validating huge masks).
> +	 */
> +	if (size < sizeof(struct cpumask)) {
> +		memset((char *)&mask + size, 0, sizeof(struct cpumask) - size);
> +	} else if (size > sizeof(struct cpumask)) {
> +		size_t i;
> +		for (i = sizeof(struct cpumask); i < size; ++i) {
> +			char c;
> +			if (get_user(c, &bits[i]))
> +				return ERR_PTR(-EFAULT);
> +			if (c)
> +				return ERR_PTR(-EINVAL);
> +		}
>  	}
>  
Surely you can just replace all of this with cpumask_parse_user()?