Re: [PATCH v3 3/3] generic sys_ipc wrapper

[Arnd Bergmann <arnd@arndb.de>]

On Friday 08 January 2010 11:03:47 Christoph Hellwig wrote:
> On Fri, Jan 08, 2010 at 10:50:23AM +0100, Christoph Hellwig wrote:
> > Always return -EINVAL for the iBCS2 special case in SHMAT, and add a
> > prototype to linux/syscalls.h

ok, the changes look good.

> Add a generic implementation of the ipc demultiplexer syscall.  Except for
> s390 and sparc64 all implementations of the sys_ipc are nearly identical.

I think the s390 version is trivial to add as well, like

SYSCALL_DEFINE5(s390_ipc, uint, call, int, first, unsigned long, second,
 		unsigned long, third, void __user *, ptr)
{
	if (call == SEMTIMEDOP)
		return sys_semtimedop(first, (struct sembuf __user *)ptr,
				      (unsigned) second,
				      (const struct timespec __user *)third);

	return sys_ipc(call & 0xffff, first, second, third, ptr, 0);
}

But while going over the code again, I noticed that you broke sign extension
at least on powerpc and s390, possibly on all 64 bit machines:

> +SYSCALL_DEFINE6(ipc, unsigned int, call, int, first, int, second,
> +		unsigned long, third, void __user *, ptr, long, fifth)
> +{
> +	int version, ret;
> +
> +	version = call >> 16; /* hack for backward compatibility */
> +	call &= 0xffff;
> +
> +	switch (call) {
> +	case SEMOP:
> +		return sys_semtimedop(first, (struct sembuf __user *)ptr,
> +				      second, NULL);

                                       (unsigned)second, NULL

> +	case SEMTIMEDOP:
> +		return sys_semtimedop(first, (struct sembuf __user *)ptr,
> +				      second,

                                      (unsigned)second,

> +				      (const struct timespec __user *)fifth);
> +
> +	case SEMGET:
> +		return sys_semget(first, second, third);

                                         (int)second,

> +	case SEMCTL: {
> +		union semun fourth;
> +		if (!ptr)
> +			return -EINVAL;
> +		if (get_user(fourth.__pad, (void __user * __user *) ptr))
> +			return -EFAULT;
> +		return sys_semctl(first, second, third, fourth);

		return sys_semctl(first, (int)second, third, fourth);

> +	case MSGSND:
> +		return sys_msgsnd(first, (struct msgbuf __user *) ptr,
> +				  second, third);

				  (size_t)second, third);

> +	case MSGRCV:
> +		switch (version) {
> +		case 0: {
> +			struct ipc_kludge tmp;
> +			if (!ptr)
> +				return -EINVAL;
> +
> +			if (copy_from_user(&tmp,
> +					   (struct ipc_kludge __user *) ptr,
> +					   sizeof(tmp)))
> +				return -EFAULT;
> +			return sys_msgrcv(first, tmp.msgp, second,

			return sys_msgrcv(first, tmp.msgp, (size_t)second,

> +					   tmp.msgtyp, third);
> +		}
> +		default:
> +			return sys_msgrcv(first,
> +					   (struct msgbuf __user *) ptr,
> +					   second, fifth, third);

					   (size_t)second, fifth, third);

> +		}
> +	case MSGGET:
> +		return sys_msgget((key_t) first, second);

		return sys_msgget((key_t) first, (int)second);

> +	case MSGCTL:
> +		return sys_msgctl(first, second, (struct msqid_ds __user *)ptr);

		return sys_msgctl(first, (int)second, (struct msqid_ds __user *)ptr);

> +	case SHMAT:
> +		switch (version) {
> +		default: {
> +			unsigned long raddr;
> +			ret = do_shmat(first, (char __user *)ptr,
> +				       second, &raddr);

				       (int)second, &raddr);

> +			if (ret)
> +				return ret;
> +			return put_user(raddr, (unsigned long __user *) third);
> +		}
> +		case 1:
> +			/*
> +			 * This was the entry point for kernel-originating calls
> +			 * from iBCS2 in 2.2 days.
> +			 */
> +			return -EINVAL;
> +		}
> +	case SHMDT:
> +		return sys_shmdt((char __user *)ptr);
> +	case SHMGET:
> +		return sys_shmget(first, second, third);

				       (size_t)second, &raddr);

> +	case SHMCTL:
> +		return sys_shmctl(first, second,

		return sys_shmctl(first, (int)second,

> +				   (struct shmid_ds __user *) ptr);
> +	default:
> +		return -ENOSYS;
> +	}
> +}

This is needed to make sure the upper half of the register is filled with
zero-extended or sign-extended correctly and does not contain random garbage
in the native 64 bit case. IIRC, x86_64 does not have this problem and mips64
may have the wrong code already. Alpha, parisc and ia64 don't have a native
sys_ipc and the rest are 32 bit, so they don't care.

	Arnd