On Fri, Jan 31, 2014 at 02:54:53AM +0000, Linux Kernel wrote:
> Commit: 2def2ef2ae5f3990aabdbe8a755911902707d268
>
> ...
>
> - if (get_compat_timespec(&ktspec, timeout))
> + if (compat_get_timespec(&ktspec, timeout))
> return -EFAULT;
>
> datagrams = __sys_recvmmsg(fd, (struct mmsghdr __user *)mmsg, vlen,
> flags | MSG_CMSG_COMPAT, &ktspec);
> - if (datagrams > 0 && put_compat_timespec(&ktspec, timeout))
> + if (datagrams > 0 && compat_put_timespec(&ktspec, timeout))
> datagrams = -EFAULT;
>
Can we rename one of each of those functions ?
It's not really surprising they got mixed up given they look so alike.
It looks like an accident just waiting to happen again.
Dave
On 01/31/2014 09:50 AM, Dave Jones wrote:
> On Fri, Jan 31, 2014 at 02:54:53AM +0000, Linux Kernel wrote:
>
> > Commit: 2def2ef2ae5f3990aabdbe8a755911902707d268
> >
> > ...
> >
> > - if (get_compat_timespec(&ktspec, timeout))
> > + if (compat_get_timespec(&ktspec, timeout))
> > return -EFAULT;
> >
> > datagrams = __sys_recvmmsg(fd, (struct mmsghdr __user *)mmsg, vlen,
> > flags | MSG_CMSG_COMPAT, &ktspec);
> > - if (datagrams > 0 && put_compat_timespec(&ktspec, timeout))
> > + if (datagrams > 0 && compat_put_timespec(&ktspec, timeout))
> > datagrams = -EFAULT;
> >
>
> Can we rename one of each of those functions ?
> It's not really surprising they got mixed up given they look so alike.
>
> It looks like an accident just waiting to happen again.
>
Very much so, I made the same comment.
My feeling is that {get,put}_compat_timespec() should at the very least
have leading underscores to flag it as a low-level function, but better
suggestions would be appreciated.
-hpa
On Fri, Jan 31, 2014 at 10:06 AM, H. Peter Anvin <[email protected]> wrote:
>
> My feeling is that {get,put}_compat_timespec() should at the very least
> have leading underscores to flag it as a low-level function, but better
> suggestions would be appreciated.
Why not just remove it entirely, and change all users to
compat_[get|set]_timespec (same for timeval etc, of course).
After all, compat_*_time*() does fall back cleanly for non-x32 cases.
And sure, maybe that particular code is never *needed* for x32
support, but the overhead is generally zero (since in most cases X32
isn't even configured), or very low anyway. So the upside of having
two subtly incompatible interfaces is very dubious, no?
Linus
On 01/31/2014 10:45 AM, Linus Torvalds wrote:
> On Fri, Jan 31, 2014 at 10:06 AM, H. Peter Anvin <[email protected]> wrote:
>>
>> My feeling is that {get,put}_compat_timespec() should at the very least
>> have leading underscores to flag it as a low-level function, but better
>> suggestions would be appreciated.
>
> Why not just remove it entirely, and change all users to
> compat_[get|set]_timespec (same for timeval etc, of course).
>
> After all, compat_*_time*() does fall back cleanly for non-x32 cases.
> And sure, maybe that particular code is never *needed* for x32
> support, but the overhead is generally zero (since in most cases X32
> isn't even configured), or very low anyway. So the upside of having
> two subtly incompatible interfaces is very dubious, no?
>
As they both seem to be out of line, I would think so. More than half
of the use cases are in kernel/compat.c where we could use a
double-underscore inline version if we really care -- it would probably
be a net win in terms of performance.
There are only 25 call sites in the kernel of
'(get|put)_compat_time(val|spec)' and that includes the ones inside the
larger functions.
-hpa
On 01/31/2014 10:45 AM, Linus Torvalds wrote:
> On Fri, Jan 31, 2014 at 10:06 AM, H. Peter Anvin <[email protected]> wrote:
>>
>> My feeling is that {get,put}_compat_timespec() should at the very least
>> have leading underscores to flag it as a low-level function, but better
>> suggestions would be appreciated.
>
> Why not just remove it entirely, and change all users to
> compat_[get|set]_timespec (same for timeval etc, of course).
>
> After all, compat_*_time*() does fall back cleanly for non-x32 cases.
> And sure, maybe that particular code is never *needed* for x32
> support, but the overhead is generally zero (since in most cases X32
> isn't even configured), or very low anyway. So the upside of having
> two subtly incompatible interfaces is very dubious, no?
>
Hmmm... it ends up being a bit weird even so. Some of the interfaces
ought to be revamped at a higher level.
Consider this bit in ipc/compat.c:
long compat_sys_semtimedop(int semid, struct sembuf __user *tsems,
unsigned nsops, const struct compat_timespec __user
*timeout)
{
struct timespec __user *ts64 = NULL;
if (timeout) {
struct timespec ts;
ts64 = compat_alloc_user_space(sizeof(*ts64));
if (get_compat_timespec(&ts, timeout))
return -EFAULT;
if (copy_to_user(ts64, &ts, sizeof(ts)))
return -EFAULT;
}
return sys_semtimedop(semid, tsems, nsops, ts64);
}
This is most definitely broken if COMPAT_USE_64BIT_TIME, even with
get_compat_timespec() is replaced by compat_get_timespec(). However,
what is *really* going on here is that we want to provide a user space
pointer to a kernel-format timespec, so we could have an interface like
this:
int compat_convert_timespec_user(struct compat_timespec **ts64p,
const struct compat_timespec __user *ts)
{
struct timespec __user *ts64;
struct timespec ts;
/* If the compat timespec is 64 bits, no conversion is needed */
if (!ts || COMPAT_USE_64BIT_TIME) {
*ts64p = (struct timespec __user *)ts;
return 0;
}
*ts64p = ts64 = compat_alloc_user_space(sizeof(*ts64));
if (__get_compat_timespec(&ts, timeout))
return -EFAULT;
if (copy_to_user(ts64, &ts, sizeof(ts)))
return -EFAULT;
return 0;
}
Now one can argue we have a potential problem with type safety here, but
I'm not sure there is any way to avoid that.
-hpa
On 01/31/2014 10:45 AM, Linus Torvalds wrote:
> On Fri, Jan 31, 2014 at 10:06 AM, H. Peter Anvin <[email protected]> wrote:
>>
>> My feeling is that {get,put}_compat_timespec() should at the very least
>> have leading underscores to flag it as a low-level function, but better
>> suggestions would be appreciated.
>
> Why not just remove it entirely, and change all users to
> compat_[get|set]_timespec (same for timeval etc, of course).
>
> After all, compat_*_time*() does fall back cleanly for non-x32 cases.
> And sure, maybe that particular code is never *needed* for x32
> support, but the overhead is generally zero (since in most cases X32
> isn't even configured), or very low anyway. So the upside of having
> two subtly incompatible interfaces is very dubious, no?
>
Here is a patch for comments/review/opinions... it has only been
compile-tested.
-hpa
On Fri, Jan 31, 2014 at 2:37 PM, H. Peter Anvin <[email protected]> wrote:
>
> Here is a patch for comments/review/opinions... it has only been
> compile-tested.
Looks sane to me from a superficial quick read-through.
Linus