On Thu, 2021-03-04 at 10:35 +0100, Philippe Gerum wrote:
> [email protected] <[email protected]> writes:
>
> > On Sat, 2021-02-20 at 16:18 +0100, Philippe Gerum via Xenomai wrote:
> > [snip]
> > > diff --git a/kernel/cobalt/posix/sem.c b/kernel/cobalt/posix/sem.c
> > > index 05a861dfe..467a9b7dd 100644
> > > --- a/kernel/cobalt/posix/sem.c
> > > +++ b/kernel/cobalt/posix/sem.c
> > > @@ -267,7 +267,7 @@ out:
> > > return ret;
> > > }
> > >
> > >
> > >
> > >
> > > -static inline int sem_fetch_timeout(struct timespec *ts,
> > > +static inline int sem_fetch_timeout(struct timespec64 *ts,
> > > const void __user *u_ts)
> >
> > Handle the following with care, maybe my understanding of
> > CONFIG_XENO_ARCH_SYS3264 is wrong. See below...
> >
> > My understanding is that this is a breaking change for 32 bit kernels.
> > The broken part is hidden here, but sem_fetch_timeout() now assumes
> > that the user provided timespec64, which is not be the case for 32 bit
> > applications running on a 32 bit kernel.
> >
>
> The user-provided memory is referred to by u_ts, not ts, which receives
> the timestamp fetch_timeout() is supposed to return. The change should
> only affect the return type, not the source one. sys32_fetch_timeout()
> should still do the right thing via sys32_get_timespec().
Adding the full code:
static inline int sem_fetch_timeout(struct timespec64 *ts,
const void __user *u_ts)
{
return u_ts == NULL ? -EFAULT :
cobalt_copy_from_user(ts, u_ts, sizeof(*ts));
}
Copying sizeof(*ts) is to much if the application provided
"__old_time_t" (sizeof(time_t) =4) only. Isn't it? I would expect the
result (in ts) to be garbage. Due to different padding the "sec" field
(in ts) would now contain the nsec value from u_ts as well.
I will come up with a proposal to fix that. We need some discussion
anyway...
>
> Did you observe a manifest breakage with 32_64 setups?
>
> >
> > > {
> > > return u_ts == NULL ? -EFAULT :
> > > @@ -276,10 +276,10 @@ static inline int sem_fetch_timeout(struct timespec
> > > *ts,
> > >
> > >
> > >
> > >
> > > int __cobalt_sem_timedwait(struct cobalt_sem_shadow __user *u_sem,
> > > const void __user *u_ts,
> > > - int (*fetch_timeout)(struct timespec *ts,
> > > + int (*fetch_timeout)(struct timespec64 *ts,
> > > const void __user *u_ts))
> > > {
> > > - struct timespec ts = { .tv_sec = 0, .tv_nsec = 0 };
> > > + struct timespec64 ts = { .tv_sec = 0, .tv_nsec = 0 };
> > > int pull_ts = 1, ret, info;
> > > struct cobalt_sem *sem;
> > > xnhandle_t handle;
> > > @@ -434,7 +434,7 @@ COBALT_SYSCALL(sem_wait, primary,
> > >
> > >
> > >
> > >
> > > COBALT_SYSCALL(sem_timedwait, primary,
> > > (struct cobalt_sem_shadow __user *u_sem,
> > > - struct timespec __user *u_ts))
> > > + struct __user_old_timespec __user *u_ts))
> >
> > This change is still correct, but this is where the journey begins. If
> > my understanding of CONFIG_XENO_ARCH_SYS3264 is correct, this syscall
> > is being used on 32 bit kernels as well. Is that understanding correct?
>
> COBALT_SYSCALL(sem_timedwait, ...) is only used in native (e.g. mere
> 64bit, or 32bit) mode. The following one is used in 32bit over 64bit
> mode specifically:
>
> COBALT_SYSCALL32emu(sem_timedwait, primary,
> (struct cobalt_sem_shadow __user *u_sem,
> struct old_timespec32 __user *u_ts))
>
>
