Prev: Grant Compensation
Next: signalfd: fill in ssi_int for posix timers and message queues
From: Arnd Bergmann on 3 Jul 2010 15:20
On Friday 02 July 2010 17:45:18 Chris Metcalf wrote:
>
> This change reflects some feedback from Arnd Bergmann, and also
> fixes a compat issue by removing the requirement that the cpumask
> pointer passed to the ioctl point to memory whose size is a
> multiple of sizeof(long), since that is awkward when userspace
> has a different sizeof(long).
>
> The compat_ptr() declaration was fixed and used to pass the
> compat_ioctl argument to the normal ioctl. So far we limit compat
> code to 2GB, so the difference between zero-extend and sign-extend
> (the latter being correct, eventually) had been overlooked.
>
> Remove the file_to_hardwall() abstractions since they're not
> really needed.

Looks good.

> In addition, use <linux/list_types.h> to simplify hardwall code.
> Instead of using a bogus hardwall_list type, we can now use
> the proper list_head type directly.

It sounds like this is now ending up in linux/types.h,
so you'll have to change that again.

> Signed-off-by: Chris Metcalf <cmetcalf(a)tilera.com>

Acked-by: Arnd Bergmann <arnd(a)arndb.de>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Paul Mundt on 4 Jul 2010 00:00
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()?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Chris Metcalf on 4 Jul 2010 08:00
On 7/3/2010 11:57 PM, Paul Mundt wrote:
> 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.
>

Good point, thanks. Done.

>> static struct hardwall_info *hardwall_create(
>> - size_t size, const unsigned long __user *bits)
>> + size_t size, const unsigned char __user *bits)
>> {
>> [...]
>>
>>
> 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.
>

Yes, it does make sense; see get_nodes() in mm/mempolicy.c for a similar
API philosophy. The idea is that you don't want to tie userspace code
to the particular NR_CPUS that you happened to build your kernel with.
So you let userspace use any reasonable value for its bitmask, and as
long as it's passing zeroes for the >NR_CPUS positions, that's OK.

cpumask_parse_user() won't help here, since we're not parsing an ASCII
string input. The main flow is just a copy_from_user() for the bits in
the mask, then an optional memset() if the user specified fewer than
NR_CPUS cpus, or a scan to check for set bits if the user specified more
than NR_CPUS cpus.

--
Chris Metcalf, Tilera Corp.
http://www.tilera.com

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
 | 
Pages: 1
Prev: Grant Compensation
Next: signalfd: fill in ssi_int for posix timers and message queues