2004-04-09 10:35:38

by Jakub Jelinek

[permalink] [raw]
Subject: Two minor nits about mq patches

Hi!

kernel/signal.c has:
case __SI_RT: /* This is not generated by the kernel as of now. */
case __SI_MESGQ: /* But this is */
err |= __put_user(from->si_pid, &to->si_pid);
err |= __put_user(from->si_uid, &to->si_uid);
err |= __put_user(from->si_int, &to->si_int);
err |= __put_user(from->si_ptr, &to->si_ptr);
break;
but si_int and si_ptr are union members, so it is enough
to __put_user si_ptr. On big-endian we have a bad problem
in 32-bit compatibility when translating 64-bit sigval_t to 32-bit,
whether to choose high or low 32-bits of si_ptr but without
union sigval { struct { int _pad; int _sival_int; } _u; void *sival_ptr; }
like definition for BE 64-bit arches (which I'm not sure POSIX would allow)
I'm afraid there is nothing to do about it.

In
http://www.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.5/2.6.5-mc3/broken-out/compat_mq.patch
there is:
+asmlinkage long compat_sys_mq_open(const char __user *u_name,
+ int oflag, compat_mode_t mode,
+ struct compat_mq_attr __user *u_attr)
+{
+ struct mq_attr attr;
+ mm_segment_t oldfs;
+ char *name;
+ long ret;
+
+ if (!u_attr)
+ return sys_mq_open(u_name, oflag, mode, 0);

which is incorrect. If oflag does not have O_CREAT set in oflag,
u_attr might contain complete garbage, and thus return -EFAULT
even when it must not or doing kernel copies of name/u_attr
unnecessarily.
So the above if should be either:
if ((oflag & O_CREAT) == 0 || !u_attr)
instead, or sys_mq_open could be split into do_mq_open
which would only deal with kernel pointers and sys_mq_open
and compat_sys_mq_open wrappers around it.

Another problem in compat-mq.patch is that __SI_MESGQ should be
handled in all 32-bit compat layers.

Jakub