2004-09-04 13:41:26

by Andi Kleen

[permalink] [raw]
Subject: Re: [PATCH] Fix argument checking in sched_setaffinity

On Wed, Sep 01, 2004 at 11:59:22AM +1000, Anton Blanchard wrote:
>
> > I notice that you didn't bother with the fractional byte that is handled
> > by 'endmask' in mm/mempolicy.c:get_nodes(). But I really don't give a
> > hoot - either way is fine by me.
> >
> > I've written a couple of code snippets that manage to intuit the size of
> > the kernel's cpumask dynamically from user space, by probing with
> > various sched_getaffinity() calls. But since your patch only changes
> > the errors generated by sched_setaffinity() [that's "set", not "get"], I
> > will not experience any grief from this subtle change in the kernel's
> > API.
> >
> > Should you lock hotplug before calling get_user_cpu_mask(), since
> > get_user_cpu_mask() depends on cpu_online_mask()?
>
> FYI the NUMA API and affinity code is broken on 64bit big endian. We
> really need a get/set compat bitmap and use it. How does this look?
> Not well tested yet...

Looks good from a quick review. But there is nothing to call it?

-Andi


2004-09-05 14:30:29

by Anton Blanchard

[permalink] [raw]
Subject: Re: [PATCH] Fix argument checking in sched_setaffinity


> Looks good from a quick review. But there is nothing to call it?

Heres one :) Unfortunately we have to frob 32bit userspace bitmaps to
64bit ones on big endian platforms. This version does extra copies but
is simple, avoids set_fs tricks and gets things working for me on ppc64.

Anton

diff -puN mm/mempolicy.c~numa_api mm/mempolicy.c
--- gr_work/mm/mempolicy.c~numa_api 2004-09-04 21:14:44.595414365 -0500
+++ gr_work-anton/mm/mempolicy.c 2004-09-05 09:12:18.899685327 -0500
@@ -525,20 +525,82 @@ asmlinkage long sys_get_mempolicy(int __
}

#ifdef CONFIG_COMPAT
-/* The other functions are compatible */
+
asmlinkage long compat_get_mempolicy(int __user *policy,
- unsigned __user *nmask, unsigned maxnode,
- unsigned addr, unsigned flags)
+ compat_ulong_t __user *nmask,
+ compat_ulong_t maxnode,
+ compat_ulong_t addr, compat_ulong_t flags)
{
long err;
unsigned long __user *nm = NULL;
+ unsigned long nr_bits, alloc_size;
+ DECLARE_BITMAP(bm, MAX_NUMNODES);
+
+ nr_bits = min(maxnode-1, MAX_NUMNODES);
+ alloc_size = ALIGN(nr_bits, BITS_PER_LONG) / 8;
+
if (nmask)
- nm = compat_alloc_user_space(ALIGN(maxnode-1, 64) / 8);
- err = sys_get_mempolicy(policy, nm, maxnode, addr, flags);
- if (!err && copy_in_user(nmask, nm, ALIGN(maxnode-1, 32)/8))
- err = -EFAULT;
+ nm = compat_alloc_user_space(alloc_size);
+
+ err = sys_get_mempolicy(policy, nm, nr_bits+1, addr, flags);
+
+ if (!err && nmask) {
+ err = copy_from_user(bm, nm, alloc_size);
+ /* ensure entire bitmap is zeroed */
+ err |= clear_user(nmask, ALIGN(maxnode-1, 8) / 8);
+ err |= compat_put_bitmap(nmask, bm, nr_bits);
+ }
+
return err;
}
+
+asmlinkage long compat_set_mempolicy(int mode, compat_ulong_t __user *nmask,
+ compat_ulong_t maxnode)
+{
+ long err;
+ unsigned long __user *nm = NULL;
+ unsigned long nr_bits, alloc_size;
+ DECLARE_BITMAP(bm, MAX_NUMNODES);
+
+ nr_bits = min(maxnode-1, MAX_NUMNODES);
+ alloc_size = ALIGN(nr_bits, BITS_PER_LONG) / 8;
+
+ if (nmask) {
+ err = compat_get_bitmap(bm, nmask, nr_bits);
+ nm = compat_alloc_user_space(alloc_size);
+ err |= copy_to_user(nm, bm, alloc_size);
+ }
+
+ if (err)
+ return -EFAULT;
+
+ return sys_set_mempolicy(mode, nm, nr_bits+1);
+}
+
+asmlinkage long compat_mbind(compat_ulong_t start, compat_ulong_t len,
+ compat_ulong_t mode, compat_ulong_t __user *nmask,
+ compat_ulong_t maxnode, compat_ulong_t flags)
+{
+ long err;
+ unsigned long __user *nm = NULL;
+ unsigned long nr_bits, alloc_size;
+ DECLARE_BITMAP(bm, MAX_NUMNODES);
+
+ nr_bits = min(maxnode-1, MAX_NUMNODES);
+ alloc_size = ALIGN(nr_bits, BITS_PER_LONG) / 8;
+
+ if (nmask) {
+ err = compat_get_bitmap(bm, nmask, nr_bits);
+ nm = compat_alloc_user_space(alloc_size);
+ err |= copy_to_user(nm, bm, alloc_size);
+ }
+
+ if (err)
+ return -EFAULT;
+
+ return sys_mbind(start, len, mode, nm, nr_bits+1, flags);
+}
+
#endif

/* Return effective policy for a VMA */
@@ -900,7 +962,7 @@ mpol_shared_policy_lookup(struct shared_

static void sp_delete(struct shared_policy *sp, struct sp_node *n)
{
- PDprintk("deleting %lx-l%x\n", n->start, n->end);
+ PDprintk("deleting %lx-%lx\n", n->start, n->end);
rb_erase(&n->nd, &sp->root);
mpol_free(n->policy);
kmem_cache_free(sn_cache, n);