Re: sched_setaffinity usability

[Andrew Morton <akpm@osdl.org>]

Ulrich Drepper <drepper@redhat.com> wrote:
>
>  The sched_setaffinity syscall currently has a usability problem.  The
>  size of cpumask_t is not visible outside the kernel and might change
>  from kernel to kernel.  So, if the user uses a large CPU bitset and
>  passes it to the kernel it is not known at all whether all the bits
>  provided in the bitmap are used.  The kernel simply copies the first
>  bytes, enough to fill in the cpumask_t object and ignores the rest.
> 
>  A simple check for a too large bitset is not good.  Programs which are
>  portable (to different kernels) and future safe should use large bitmap
>  sizes.  Instead the user should only be notified about the size problem
>  if any nonzero bit is ignored.

Perhaps the syscall itself should go look for set bits which are beyond the
current number of physical CPUs and fail the syscall if any are found.

Like this, if it was tested:


diff -puN kernel/sched.c~a kernel/sched.c
--- 25/kernel/sched.c~a	2004-03-18 01:25:29.697217008 -0800
+++ 25-akpm/kernel/sched.c	2004-03-18 01:42:32.312755816 -0800
@@ -2736,13 +2736,39 @@ asmlinkage long sys_sched_setaffinity(pi
 	cpumask_t new_mask;
 	int retval;
 	task_t *p;
+	int remainder;
+	unsigned long __user *up;
 
 	if (len < sizeof(new_mask))
 		return -EINVAL;
 
+	/* Avoid spending stupid amounts of time in the kernel */
+	if (len > 16384)
+		return -EINVAL;
+
 	if (copy_from_user(&new_mask, user_mask_ptr, sizeof(new_mask)))
 		return -EFAULT;
 
+	/*
+	 * Check that the user hasn't asked for any impossible CPUs outside
+	 * sizeof(cpumask_t).  set_cpus_allowed() will check for impossible
+	 * cpus inside sizeof(cpumask_t).
+	 */
+	remainder = len - sizeof(new_mask);	/* bytes */
+	up = user_mask_ptr + 1;
+	while (remainder > 0) {
+		unsigned long u;
+		int nr_bits;
+
+		if (get_user(u, up))
+			return -EFAULT;
+		nr_bits = min((int)sizeof(u), remainder);
+		if (find_next_bit(&u, nr_bits, 0) >= nr_bits)
+			return -EINVAL;
+		remainder -= sizeof(u);
+		up++;
+	}
+
 	read_lock(&tasklist_lock);
 
 	p = find_process_by_pid(pid);

_