This is my first attempt to convert sound subsystem to year 2038 safe. In these series patches I focus on the timer.
When check the time relative code in timer of sound subsystem, I feel that I could easy split 64bit time_xxx type in kernel and in userspace (__kernel_time_xxx) according to arnd's approach[1] in comparison with other parts of sound subsys(e.g. pcm). Whether I should follow the same approach in the whole sound subsystem is an open issue for me.
On the other hand, there are difference approaches for dealing with the code in userspace. It seems that snd_timer_read is the only api for other parts of alsa. It share the same code no matter tread is true or false.
The fisrt approach is hide __kernel_time_xxx inside snd_timer_read, although the code may be a little bit ugly.
The second approach is that force userspace migration to 64bit time on all 32bit(including compat) system by re-definition the following types in alsa-lib/include/global.h: typedef struct __kernel_timespec snd_htimestamp_t; This approach will not affect the 64bit application.
[1] http://git.kernel.org/cgit/linux/kernel/git/arnd/playground.git/log/?h=y2038...
Bamvor Zhang Jian (2): y2038: sound: convert timer to y2038 safe y2038: sound: convert compat ioctl of timer to year 2038 safe
include/sound/timer.h | 8 +++++++- include/uapi/sound/asound.h | 9 ++++++--- sound/core/timer.c | 49 +++++++++++++++++++++++++++++++-------------- sound/core/timer_compat.c | 4 +++- 4 files changed, 50 insertions(+), 20 deletions(-)
There are two parts in this patch. I put them together in order to compile pass.
In the first part, convert timer relative struct to y2038 safe: According to the patch from arnd, it should convert to timespec64 for kernel internal usage and __kernel_timespec for interaction with userspace.
In the second part, convert the timer relative function to y2038 safe. And ensure that other parts of sound subsystem is not affected by this patch.
When arnd's patch[1] is applied and CONFIG_COMPAT_TIME is defined, the 64bit time is used between kernel and userspace. Without arnd's patch[1] or CONFIG_COMPAT_TIME is not defined, timespec will be used between kernel and userspace. That is 64bit time for 64bit application and 32bit time for 32bit application.
[1] y2038: introduce struct __kernel_timespec
Signed-off-by: Bamvor Zhang Jian bamvor.zhangjian@linaro.org --- include/sound/timer.h | 8 +++++++- include/uapi/sound/asound.h | 9 ++++++--- sound/core/timer.c | 49 +++++++++++++++++++++++++++++++-------------- 3 files changed, 47 insertions(+), 19 deletions(-)
diff --git a/include/sound/timer.h b/include/sound/timer.h index 7990469..2cfee32 100644 --- a/include/sound/timer.h +++ b/include/sound/timer.h @@ -102,7 +102,7 @@ struct snd_timer_instance { unsigned long ticks, unsigned long resolution); void (*ccallback) (struct snd_timer_instance * timeri, int event, - struct timespec * tstamp, + struct timespec64 * tstamp, unsigned long resolution); void *callback_data; unsigned long ticks; /* auto-load ticks when expired */ @@ -120,6 +120,12 @@ struct snd_timer_instance { struct snd_timer_instance *master; };
+struct snd_timer_tread { + int event; + struct timespec64 tstamp; + unsigned int val; +}; + /* * Registering */ diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h index a45be6b..f7e3793 100644 --- a/include/uapi/sound/asound.h +++ b/include/uapi/sound/asound.h @@ -29,6 +29,9 @@ #include <stdlib.h> #endif
+#ifndef CONFIG_COMPAT_TIME +# define __kernel_timespec timespec +#endif /* * protocol version */ @@ -739,7 +742,7 @@ struct snd_timer_params { };
struct snd_timer_status { - struct timespec tstamp; /* Timestamp - last update */ + struct __kernel_timespec tstamp;/* Timestamp - last update */ unsigned int resolution; /* current period resolution in ns */ unsigned int lost; /* counter of master tick lost */ unsigned int overrun; /* count of read queue overruns */ @@ -787,9 +790,9 @@ enum { SNDRV_TIMER_EVENT_MRESUME = SNDRV_TIMER_EVENT_RESUME + 10, };
-struct snd_timer_tread { +struct __kernel_snd_timer_tread { int event; - struct timespec tstamp; + struct __kernel_timespec tstamp; unsigned int val; };
diff --git a/sound/core/timer.c b/sound/core/timer.c index 31f40f0..1b6e1ef 100644 --- a/sound/core/timer.c +++ b/sound/core/timer.c @@ -70,7 +70,7 @@ struct snd_timer_user { spinlock_t qlock; unsigned long last_resolution; unsigned int filter; - struct timespec tstamp; /* trigger tstamp */ + struct timespec64 tstamp; /* trigger tstamp */ wait_queue_head_t qchange_sleep; struct fasync_struct *fasync; struct mutex tread_sem; @@ -387,12 +387,12 @@ static void snd_timer_notify1(struct snd_timer_instance *ti, int event) unsigned long flags; unsigned long resolution = 0; struct snd_timer_instance *ts; - struct timespec tstamp; + struct timespec64 tstamp;
if (timer_tstamp_monotonic) - ktime_get_ts(&tstamp); + ktime_get_ts64(&tstamp); else - getnstimeofday(&tstamp); + getnstimeofday64(&tstamp); if (snd_BUG_ON(event < SNDRV_TIMER_EVENT_START || event > SNDRV_TIMER_EVENT_PAUSE)) return; @@ -883,7 +883,8 @@ static int snd_timer_dev_disconnect(struct snd_device *device) return 0; }
-void snd_timer_notify(struct snd_timer *timer, int event, struct timespec *tstamp) +static void snd_timer_notify64(struct snd_timer *timer, int event, + struct timespec64 *tstamp64) { unsigned long flags; unsigned long resolution = 0; @@ -905,14 +906,23 @@ void snd_timer_notify(struct snd_timer *timer, int event, struct timespec *tstam } list_for_each_entry(ti, &timer->active_list_head, active_list) { if (ti->ccallback) - ti->ccallback(ti, event, tstamp, resolution); + ti->ccallback(ti, event, tstamp64, resolution); list_for_each_entry(ts, &ti->slave_active_head, active_list) if (ts->ccallback) - ts->ccallback(ts, event, tstamp, resolution); + ts->ccallback(ts, event, tstamp64, resolution); } spin_unlock_irqrestore(&timer->lock, flags); }
+void snd_timer_notify(struct snd_timer *timer, int event, struct timespec *tstamp) +{ + struct timespec64 tstamp64; + + tstamp64.tv_sec = tstamp->tv_sec; + tstamp64.tv_nsec = tstamp->tv_nsec; + snd_timer_notify64(timer, event, &tstamp64); +} + /* * exported functions for global timers */ @@ -1159,7 +1169,7 @@ static void snd_timer_user_append_to_tqueue(struct snd_timer_user *tu,
static void snd_timer_user_ccallback(struct snd_timer_instance *timeri, int event, - struct timespec *tstamp, + struct timespec64 *tstamp, unsigned long resolution) { struct snd_timer_user *tu = timeri->callback_data; @@ -1187,7 +1197,7 @@ static void snd_timer_user_tinterrupt(struct snd_timer_instance *timeri, { struct snd_timer_user *tu = timeri->callback_data; struct snd_timer_tread *r, r1; - struct timespec tstamp; + struct timespec64 tstamp; int prev, append = 0;
memset(&tstamp, 0, sizeof(tstamp)); @@ -1199,9 +1209,9 @@ static void snd_timer_user_tinterrupt(struct snd_timer_instance *timeri, } if (tu->last_resolution != resolution || ticks > 0) { if (timer_tstamp_monotonic) - ktime_get_ts(&tstamp); + ktime_get_ts64(&tstamp); else - getnstimeofday(&tstamp); + getnstimeofday64(&tstamp); } if ((tu->filter & (1 << SNDRV_TIMER_EVENT_RESOLUTION)) && tu->last_resolution != resolution) { @@ -1702,7 +1712,8 @@ static int snd_timer_user_status(struct file *file, if (!tu->timeri) return -EBADFD; memset(&status, 0, sizeof(status)); - status.tstamp = tu->tstamp; + status.tstamp.tv_sec = tu->tstamp.tv_sec; + status.tstamp.tv_nsec = tu->tstamp.tv_nsec; status.resolution = snd_timer_resolution(tu->timeri); status.lost = tu->timeri->lost; status.overrun = tu->overrun; @@ -1843,9 +1854,12 @@ static ssize_t snd_timer_user_read(struct file *file, char __user *buffer, struct snd_timer_user *tu; long result = 0, unit; int err = 0; + struct __kernel_snd_timer_tread kttr; + struct snd_timer_tread *ttrp;
tu = file->private_data; - unit = tu->tread ? sizeof(struct snd_timer_tread) : sizeof(struct snd_timer_read); + unit = tu->tread ? sizeof(struct __kernel_snd_timer_tread) : + sizeof(struct snd_timer_read); spin_lock_irq(&tu->qlock); while ((long)count - result >= unit) { while (!tu->qused) { @@ -1877,8 +1891,13 @@ static ssize_t snd_timer_user_read(struct file *file, char __user *buffer, goto _error;
if (tu->tread) { - if (copy_to_user(buffer, &tu->tqueue[tu->qhead++], - sizeof(struct snd_timer_tread))) { + ttrp = &tu->tqueue[tu->qhead++]; + kttr.event = ttrp->event; + kttr.tstamp.tv_sec = ttrp->tstamp.tv_sec; + kttr.tstamp.tv_nsec = ttrp->tstamp.tv_nsec; + kttr.val = ttrp->val; + if (copy_to_user(buffer, &kttr, + sizeof(struct __kernel_snd_timer_tread))) { err = -EFAULT; goto _error; }
On Friday 17 July 2015 15:21:07 Bamvor Zhang Jian wrote:
diff --git a/include/sound/timer.h b/include/sound/timer.h index 7990469..2cfee32 100644 --- a/include/sound/timer.h +++ b/include/sound/timer.h @@ -120,6 +120,12 @@ struct snd_timer_instance { struct snd_timer_instance *master; }; +struct snd_timer_tread {
- int event;
- struct timespec64 tstamp;
- unsigned int val;
+};
/*
- Registering
*/ diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h index a45be6b..f7e3793 100644 --- a/include/uapi/sound/asound.h +++ b/include/uapi/sound/asound.h @@ -29,6 +29,9 @@ #include <stdlib.h> #endif +#ifndef CONFIG_COMPAT_TIME +# define __kernel_timespec timespec +#endif
CONFIG_COMPAT_TIME cannot be used in a uapi header: whether user space uses a 64-bit or 32-bit time_t is independent of what gets implemented in the kernel.
- protocol version
*/ @@ -739,7 +742,7 @@ struct snd_timer_params { }; struct snd_timer_status {
- struct timespec tstamp; /* Timestamp - last update */
- struct __kernel_timespec tstamp;/* Timestamp - last update */ unsigned int resolution; /* current period resolution in ns */ unsigned int lost; /* counter of master tick lost */ unsigned int overrun; /* count of read queue overruns */
@@ -787,9 +790,9 @@ enum { SNDRV_TIMER_EVENT_MRESUME = SNDRV_TIMER_EVENT_RESUME + 10, }; -struct snd_timer_tread { +struct __kernel_snd_timer_tread { int event;
- struct timespec tstamp;
- struct __kernel_timespec tstamp; unsigned int val;
};
Also, __kernel_timespec is defined to be always 64-bit wide. This means if we do this change (assuming we drop the #define above), then user space will always see the new definition of this structure, and programs compiled against the latest header will no longer work on older kernels.
Is this what you had in mind?
We could decide to do it like this, and we have historically done changes to the ioctl interface this way, but I'm not sure if we want to do it for all ioctls.
The alternative is to leave the 'timespec' visible here for user space, so user programs will see either the old or the new definition of struct depending on their timespec definition, and only programs built with 64-bit time_t will require new kernels.
+void snd_timer_notify(struct snd_timer *timer, int event, struct timespec *tstamp) +{
- struct timespec64 tstamp64;
- tstamp64.tv_sec = tstamp->tv_sec;
- tstamp64.tv_nsec = tstamp->tv_nsec;
- snd_timer_notify64(timer, event, &tstamp64);
+}
This works, but I'd leave it up to Mark if he'd prefer to do the conversion bit by bit and change over all users of snd_timer_notify to use snd_timer_notify64, or to move them all at once and leave the function name unchanged.
I can see six callers of snd_timer_notify, but they are all in the same file, so I'd expect it to be possible to convert them all together, e.g. by adding a patch that changes the prototype in all these callers after changing the ccallback prototype.
@@ -1702,7 +1712,8 @@ static int snd_timer_user_status(struct file *file, if (!tu->timeri) return -EBADFD; memset(&status, 0, sizeof(status));
- status.tstamp = tu->tstamp;
- status.tstamp.tv_sec = tu->tstamp.tv_sec;
- status.tstamp.tv_nsec = tu->tstamp.tv_nsec; status.resolution = snd_timer_resolution(tu->timeri); status.lost = tu->timeri->lost; status.overrun = tu->overrun;
With the change to the structure definition, this will now only handle the new structure size on patched kernels, but not work with old user space on native 32-bit kernels any more.
Your patch 2 fixes the case of handling both old compat 32-bit user space on 64-bit kernels as well as new compat 32-bit user space with 64-bit time_t, but I think you are missing the case of handling old 32-bit user space.
Note that we cannot use compat_ioctl() for native 32-bit kernels, so snd_timer_user_ioctl will now have to be changed to handle both cases.
@@ -1843,9 +1854,12 @@ static ssize_t snd_timer_user_read(struct file *file, char __user *buffer, struct snd_timer_user *tu; long result = 0, unit; int err = 0;
- struct __kernel_snd_timer_tread kttr;
- struct snd_timer_tread *ttrp;
tu = file->private_data;
- unit = tu->tread ? sizeof(struct snd_timer_tread) : sizeof(struct snd_timer_read);
- unit = tu->tread ? sizeof(struct __kernel_snd_timer_tread) :
sizeof(struct snd_timer_read);
Now this is the part that gets really tricky: Instead of two cases (read and tread), we now have to handle three cases. Any user space that is compiled with 64-bit time_t needs to get the new structure, while old user space needs to get the old structure.
It looks like we already get this wrong for existing compat user space: running a 32-bit program on a 64-bit kernel will currently get the 64-bit version of struct snd_timer_tread and misinterpret that. We can probably fix both issues at the same time after introducing turning the tread flag into a three-way enum (or something along that lines).
I would recommend separating the tread changes from the user_status changes, as both of them are getting more complex now.
Arnd
On Friday 17 July 2015 14:59:48 Arnd Bergmann wrote:
Also, __kernel_timespec is defined to be always 64-bit wide. This means if we do this change (assuming we drop the #define above), then user space will always see the new definition of this structure, and programs compiled against the latest header will no longer work on older kernels.
Is this what you had in mind?
We could decide to do it like this, and we have historically done changes to the ioctl interface this way, but I'm not sure if we want to do it for all ioctls.
The alternative is to leave the 'timespec' visible here for user space, so user programs will see either the old or the new definition of struct depending on their timespec definition, and only programs built with 64-bit time_t will require new kernels.
For clarification: The approach of changing the existing data structure definition will work in this particular case, but a lot of other ioctl commands just pass a plain time_t / timespec / timeval.
For those commands, we absolutely have to make sure that kernel and user space agree on the command code for old and new argument format, and that will involve an #ifdef like the example I gave. Aside from ioctl, we need the same thing for a couple of other interfaces like setsockopts.
Arnd
Hi, Arnd
On 07/17/2015 08:59 PM, Arnd Bergmann wrote:
On Friday 17 July 2015 15:21:07 Bamvor Zhang Jian wrote:
diff --git a/include/sound/timer.h b/include/sound/timer.h index 7990469..2cfee32 100644 --- a/include/sound/timer.h +++ b/include/sound/timer.h @@ -120,6 +120,12 @@ struct snd_timer_instance { struct snd_timer_instance *master; }; +struct snd_timer_tread {
- int event;
- struct timespec64 tstamp;
- unsigned int val;
+};
/*
- Registering
*/ diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h index a45be6b..f7e3793 100644 --- a/include/uapi/sound/asound.h +++ b/include/uapi/sound/asound.h @@ -29,6 +29,9 @@ #include <stdlib.h> #endif +#ifndef CONFIG_COMPAT_TIME +# define __kernel_timespec timespec +#endif
CONFIG_COMPAT_TIME cannot be used in a uapi header: whether user space uses a 64-bit or 32-bit time_t is independent of what gets implemented in the kernel.
Oh, sorry for doing this again. Originally, I want to follow your approach in "include/linux/time64.h" in patch[1] in order to work with(for y2038 safe) and without(for keeping abi unchaged) your patch[1]. On the other hand, when I write this commit, I found that there are some headers include the <linux/time.h>(most them are drivers): include/uapi/linux/videodev2.h, include/uapi/linux/input.h, include/uapi/linux/timex.h, include/uapi/linux/elfcore.h, include/uapi/linux/resource.h, include/uapi/linux/dvb/dmx.h, include/uapi/linux/dvb/video.h, include/uapi/linux/coda.h. CONFIG_COMPAT_TIME is used indirectly in uapi files. Do we need to fix these issues?
- protocol version
*/ @@ -739,7 +742,7 @@ struct snd_timer_params { }; struct snd_timer_status {
- struct timespec tstamp; /* Timestamp - last update */
- struct __kernel_timespec tstamp;/* Timestamp - last update */ unsigned int resolution; /* current period resolution in ns */ unsigned int lost; /* counter of master tick lost */ unsigned int overrun; /* count of read queue overruns */
@@ -787,9 +790,9 @@ enum { SNDRV_TIMER_EVENT_MRESUME = SNDRV_TIMER_EVENT_RESUME + 10, }; -struct snd_timer_tread { +struct __kernel_snd_timer_tread { int event;
- struct timespec tstamp;
- struct __kernel_timespec tstamp; unsigned int val;
};
Also, __kernel_timespec is defined to be always 64-bit wide. This means if we do this change (assuming we drop the #define above), then user space will always see the new definition of this structure, and programs compiled against the latest header will no longer work on older kernels.
Is this what you had in mind?
Yes. It was what in my head. But I do not whether it ought to be.
We could decide to do it like this, and we have historically done changes to the ioctl interface this way, but I'm not sure if we want to do it for all ioctls.
The alternative is to leave the 'timespec' visible here for user space, so user programs will see either the old or the new definition of struct depending on their timespec definition, and only programs built with 64-bit time_t will require new kernels.
Do you mean we leave snd_timer_tread unchanged and introduce a new struct? (assuming we drop the #define above). struct snd_timer_tread64 { int event; struct timespec tstamp; struct __kernel_timespec tstamp; unsigned int val; };
#if BITS_PER_TIME_T == BITS_PER_LONG #define SNDRV_TIMER_IOCTL_TREAD _IOW('T', 0x02, int) #else #define SNDRV_TIMER_IOCTL_TREAD64 _IOW('T', 0x15, int) #endif
snd_timer_tread64 is used by SNDRV_TIMER_IOCTL_TREAD64.
IIUC, we should define SNDRV_TIMER_IOCTL_TREAD64 instead keep the same name for differenct ioctl, otherwise it is hard to support the old and new application binary in new kernel.
+void snd_timer_notify(struct snd_timer *timer, int event, struct timespec *tstamp) +{
- struct timespec64 tstamp64;
- tstamp64.tv_sec = tstamp->tv_sec;
- tstamp64.tv_nsec = tstamp->tv_nsec;
- snd_timer_notify64(timer, event, &tstamp64);
+}
This works, but I'd leave it up to Mark if he'd prefer to do the conversion bit by bit and change over all users of snd_timer_notify to use snd_timer_notify64, or to move them all at once and leave the function name unchanged.
Yes, understand. I want to keep the function name unchanged too. I guess I will do it when pcm part of y2038 patches is ready. Otherwise, the sound subsystem is broken.
I can see six callers of snd_timer_notify, but they are all in the same file, so I'd expect it to be possible to convert them all together, e.g. by adding a patch that changes the prototype in all these callers after changing the ccallback prototype.
Got you.
@@ -1702,7 +1712,8 @@ static int snd_timer_user_status(struct file *file, if (!tu->timeri) return -EBADFD; memset(&status, 0, sizeof(status));
- status.tstamp = tu->tstamp;
- status.tstamp.tv_sec = tu->tstamp.tv_sec;
- status.tstamp.tv_nsec = tu->tstamp.tv_nsec; status.resolution = snd_timer_resolution(tu->timeri); status.lost = tu->timeri->lost; status.overrun = tu->overrun;
With the change to the structure definition, this will now only handle the new structure size on patched kernels, but not work with old user space on native 32-bit kernels any more.
Your patch 2 fixes the case of handling both old compat 32-bit user space on 64-bit kernels as well as new compat 32-bit user space with 64-bit time_t, but I think you are missing the case of handling old 32-bit user space.
Note that we cannot use compat_ioctl() for native 32-bit kernels, so snd_timer_user_ioctl will now have to be changed to handle both cases.
Oh, I miss it, I will try to add it in my next version.
@@ -1843,9 +1854,12 @@ static ssize_t snd_timer_user_read(struct file *file, char __user *buffer, struct snd_timer_user *tu; long result = 0, unit; int err = 0;
- struct __kernel_snd_timer_tread kttr;
- struct snd_timer_tread *ttrp;
tu = file->private_data;
- unit = tu->tread ? sizeof(struct snd_timer_tread) : sizeof(struct snd_timer_read);
- unit = tu->tread ? sizeof(struct __kernel_snd_timer_tread) :
sizeof(struct snd_timer_read);Now this is the part that gets really tricky: Instead of two cases (read and tread), we now have to handle three cases. Any user space that is compiled with 64-bit time_t needs to get the new structure, while old user space needs to get the old structure.
It looks like we already get this wrong for existing compat user space: running a 32-bit program on a 64-bit kernel will currently get the 64-bit version of struct snd_timer_tread and misinterpret that. We can probably fix both issues at the same time after introducing turning the tread flag into a three-way enum (or something along that lines).
It seems that the current tread flag select the read or tread. How about choose the following you mentioned to deal with it? if (tu->tread) if (BITS_PER_TIME_T == BITS_PER_LONG) unit = sizeof(struct snd_timer_tread); else unit = sizeof(struct snd_timer_tread64); else unit = sizeof(struct snd_timer_read);
I would recommend separating the tread changes from the user_status changes, as both of them are getting more complex now.
Ok.
Arnd
regards
bamvor [1] http://git.kernel.org/cgit/linux/kernel/git/arnd/playground.git/commit/?h=y2...
Add SNDRV_TIMER_IOCTL_STATUS in compat_ioctl functions for the compat application which want to migrate to year 2038 safe. In the other hand, convert the time stamp between 64bit in kernel and 32bit in userspace for the old SNDRV_TIMER_IOCTL_STATUS32.
Signed-off-by: Bamvor Zhang Jian bamvor.zhangjian@linaro.org --- sound/core/timer_compat.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/sound/core/timer_compat.c b/sound/core/timer_compat.c index e05802a..36f5989 100644 --- a/sound/core/timer_compat.c +++ b/sound/core/timer_compat.c @@ -76,7 +76,8 @@ static int snd_timer_user_status_compat(struct file *file, if (snd_BUG_ON(!tu->timeri)) return -ENXIO; memset(&status, 0, sizeof(status)); - status.tstamp = tu->tstamp; + status.tstamp.tv_sec = (compat_time_t)(tu->tstamp.tv_sec & 0xffffffff); + status.tstamp.tv_nsec = (s32)(tu->tstamp.tv_nsec & 0xffffffff); status.resolution = snd_timer_resolution(tu->timeri); status.lost = tu->timeri->lost; status.overrun = tu->overrun; @@ -117,6 +118,7 @@ static long snd_timer_user_ioctl_compat(struct file *file, unsigned int cmd, uns case SNDRV_TIMER_IOCTL_PAUSE: case SNDRV_TIMER_IOCTL_PAUSE_OLD: case SNDRV_TIMER_IOCTL_NEXT_DEVICE: + case SNDRV_TIMER_IOCTL_STATUS: return snd_timer_user_ioctl(file, cmd, (unsigned long)argp); case SNDRV_TIMER_IOCTL_INFO32: return snd_timer_user_info_compat(file, argp);
-
Arnd Bergmann -
Bamvor Zhang Jian