Re: [alsa-devel] [RFC PATCH v2 7/7] sound: core: Avoid using timespec for struct snd_timer_tread

On Thu, 02 Nov 2017 12:06:57 +0100, Baolin Wang wrote:

The struct snd_timer_tread will use 'timespec' type variables to record timestamp, which is not year 2038 safe on 32bits system.

Since the struct snd_timer_tread is passed through read() rather than ioctl(), and the read syscall has no command number that lets us pick between the 32-bit or 64-bit version of this structure.

Thus we introduced one new command SNDRV_TIMER_IOCTL_TREAD64 and new struct snd_timer_tread64 replacing timespec with s64 type to handle 64bit time_t. Also add one struct compat_snd_timer_tread64_x86_32 to handle 64bit time_t with 32bit alignment. That means we will set tu->tread = TREAD_FORMAT_64BIT when user space has a 64bit time_t, then we will copy to user with struct snd_timer_tread64. For x86_32 mode, we will set tu->tread = TREAD_FORMAT_TIME32_X86 to copy struct compat_snd_timer_tread64_x86_32 for user. Otherwise we will use 32bit time_t variables when copying to user.

We should introduce SNDRV_TIMER_IOCTL_USER_PVERSION instead, where user-space can tell which protocol version it understands. If the protocol version is higher than some definition, we can assume it's 64-bit ready. The *_USER_PVERSION is issued from alsa-lib side. In that way, we can extend the ABI more flexibly. A similar trick was already used in PCM ABI. (Ditto for control and rawmidi API; we should have the same mechanism for all relevant APIs).

Moreover, once when the new protocol is used, we can use the standard 64bit monotonic nsecs as a timestamp, so that we don't need to care about 32/64bit compatibility.

thanks,

Takashi

Moreover this patch replaces timespec type with timespec64 type and related y2038 safe APIs.

Signed-off-by: Baolin Wang baolin.wang@linaro.org

include/uapi/sound/asound.h | 11 ++- sound/core/timer.c | 173 ++++++++++++++++++++++++++++++++++--------- sound/core/timer_compat.c | 10 ++- 3 files changed, 157 insertions(+), 37 deletions(-)

diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h index fabb283..4c74f52 100644 --- a/include/uapi/sound/asound.h +++ b/include/uapi/sound/asound.h @@ -760,7 +760,7 @@ struct snd_timer_status {

#define SNDRV_TIMER_IOCTL_PVERSION _IOR('T', 0x00, int) #define SNDRV_TIMER_IOCTL_NEXT_DEVICE _IOWR('T', 0x01, struct snd_timer_id) -#define SNDRV_TIMER_IOCTL_TREAD _IOW('T', 0x02, int) +#define SNDRV_TIMER_IOCTL_TREAD_OLD _IOW('T', 0x02, int) #define SNDRV_TIMER_IOCTL_GINFO _IOWR('T', 0x03, struct snd_timer_ginfo) #define SNDRV_TIMER_IOCTL_GPARAMS _IOW('T', 0x04, struct snd_timer_gparams) #define SNDRV_TIMER_IOCTL_GSTATUS _IOWR('T', 0x05, struct snd_timer_gstatus) @@ -773,6 +773,15 @@ struct snd_timer_status { #define SNDRV_TIMER_IOCTL_STOP _IO('T', 0xa1) #define SNDRV_TIMER_IOCTL_CONTINUE _IO('T', 0xa2) #define SNDRV_TIMER_IOCTL_PAUSE _IO('T', 0xa3) +#define SNDRV_TIMER_IOCTL_TREAD64 _IOW('T', 0xa4, int)

+#if __BITS_PER_LONG == 64 +#define SNDRV_TIMER_IOCTL_TREAD SNDRV_TIMER_IOCTL_TREAD_OLD +#else +#define SNDRV_TIMER_IOCTL_TREAD ((sizeof(__kernel_long_t) > sizeof(time_t)) ? \

• 		 SNDRV_TIMER_IOCTL_TREAD_OLD : \
  

• 		 SNDRV_TIMER_IOCTL_TREAD64)
  

+#endif

struct snd_timer_read { unsigned int resolution; diff --git a/sound/core/timer.c b/sound/core/timer.c index 9168365..ba6c09e 100644 --- a/sound/core/timer.c +++ b/sound/core/timer.c @@ -58,6 +58,30 @@ MODULE_ALIAS_CHARDEV(CONFIG_SND_MAJOR, SNDRV_MINOR_TIMER); MODULE_ALIAS("devname:snd/timer");

+enum timer_tread_format {

• TREAD_FORMAT_NONE = 0,
• TREAD_FORMAT_64BIT,
• TREAD_FORMAT_TIME64,
• TREAD_FORMAT_TIME32_X86,
• TREAD_FORMAT_TIME32,

+};

+struct snd_timer_tread64 {

• int event;
• u32 pad1;
• s64 tstamp_sec;
• s64 tstamp_nsec;
• unsigned int val;
• u32 pad2;

+};

+struct compat_snd_timer_tread64_x86_32 {

• int event;
• s64 tstamp_sec;
• s64 tstamp_nsec;
• unsigned int val;

+} __packed;

struct snd_timer_user { struct snd_timer_instance *timeri; int tread; /* enhanced read with timestamps and events */ @@ -69,7 +93,7 @@ struct snd_timer_user { int queue_size; bool disconnected; struct snd_timer_read *queue;

• struct snd_timer_tread *tqueue;

• struct snd_timer_tread64 *tqueue; spinlock_t qlock; unsigned long last_resolution; unsigned int filter;

@@ -1262,7 +1286,7 @@ static void snd_timer_user_interrupt(struct snd_timer_instance *timeri, }

static void snd_timer_user_append_to_tqueue(struct snd_timer_user *tu,

• 			    struct snd_timer_tread *tread)
  

• 			    struct snd_timer_tread64 *tread)
  

{ if (tu->qused >= tu->queue_size) { tu->overrun++; @@ -1279,7 +1303,7 @@ static void snd_timer_user_ccallback(struct snd_timer_instance *timeri, unsigned long resolution) { struct snd_timer_user *tu = timeri->callback_data;

• struct snd_timer_tread r1;

• struct snd_timer_tread64 r1; unsigned long flags;

  if (event >= SNDRV_TIMER_EVENT_START &&

@@ -1289,7 +1313,8 @@ static void snd_timer_user_ccallback(struct snd_timer_instance *timeri, return; memset(&r1, 0, sizeof(r1)); r1.event = event;

• r1.tstamp = timespec64_to_timespec(*tstamp);

• r1.tstamp_sec = tstamp->tv_sec;
• r1.tstamp_nsec = tstamp->tv_nsec; r1.val = resolution; spin_lock_irqsave(&tu->qlock, flags); snd_timer_user_append_to_tqueue(tu, &r1);

@@ -1311,7 +1336,7 @@ static void snd_timer_user_tinterrupt(struct snd_timer_instance *timeri, unsigned long ticks) { struct snd_timer_user *tu = timeri->callback_data;

• struct snd_timer_tread *r, r1;

• struct snd_timer_tread64 *r, r1; struct timespec64 tstamp; int prev, append = 0;

@@ -1332,7 +1357,8 @@ static void snd_timer_user_tinterrupt(struct snd_timer_instance *timeri, if ((tu->filter & (1 << SNDRV_TIMER_EVENT_RESOLUTION)) && tu->last_resolution != resolution) { r1.event = SNDRV_TIMER_EVENT_RESOLUTION;

• r1.tstamp = timespec64_to_timespec(tstamp);
  

• r1.tstamp_sec = tstamp.tv_sec;
  

• r1.tstamp_nsec = tstamp.tv_nsec;
  

  r1.val = resolution; snd_timer_user_append_to_tqueue(tu, &r1); tu->last_resolution = resolution;

@@ -1346,14 +1372,16 @@ static void snd_timer_user_tinterrupt(struct snd_timer_instance *timeri, prev = tu->qtail == 0 ? tu->queue_size - 1 : tu->qtail - 1; r = &tu->tqueue[prev]; if (r->event == SNDRV_TIMER_EVENT_TICK) {

• 	r->tstamp = timespec64_to_timespec(tstamp);
  

• 	r->tstamp_sec = tstamp.tv_sec;
  

• 	r->tstamp_nsec = tstamp.tv_nsec;
  r->val += ticks;
  append++;
  goto __wake;
  

  } } r1.event = SNDRV_TIMER_EVENT_TICK;

• r1.tstamp = timespec64_to_timespec(tstamp);

• r1.tstamp_sec = tstamp.tv_sec;
• r1.tstamp_nsec = tstamp.tv_nsec; r1.val = ticks; snd_timer_user_append_to_tqueue(tu, &r1); append++;

@@ -1368,7 +1396,7 @@ static void snd_timer_user_tinterrupt(struct snd_timer_instance *timeri, static int realloc_user_queue(struct snd_timer_user *tu, int size) { struct snd_timer_read *queue = NULL;

• struct snd_timer_tread *tqueue = NULL;

• struct snd_timer_tread64 *tqueue = NULL;

  if (tu->tread) { tqueue = kcalloc(size, sizeof(*tqueue), GFP_KERNEL);

@@ -1797,11 +1825,11 @@ static int snd_timer_user_params(struct file *file, tu->qhead = tu->qtail = tu->qused = 0; if (tu->timeri->flags & SNDRV_TIMER_IFLG_EARLY_EVENT) { if (tu->tread) {

• 	struct snd_timer_tread tread;
  

• 	struct snd_timer_tread64 tread;
  memset(&tread, 0, sizeof(tread));
  tread.event = SNDRV_TIMER_EVENT_EARLY;
  

• 	tread.tstamp.tv_sec = 0;
  

• 	tread.tstamp.tv_nsec = 0;
  

• 	tread.tstamp_sec = 0;
  

• 	tread.tstamp_nsec = 0;
  tread.val = 0;
  snd_timer_user_append_to_tqueue(tu, &tread);
  

  } else {

@@ -1919,6 +1947,39 @@ static int snd_timer_user_pause(struct file *file) return (err = snd_timer_pause(tu->timeri)) < 0 ? err : 0; }

+static int snd_timer_user_tread(void __user *argp, struct snd_timer_user *tu,

• 		unsigned int cmd, bool compat)
  

+{

• int __user *p = argp;
• int xarg, old_tread;
• if (tu->timeri) /* too late */

• return -EBUSY;
  

• if (get_user(xarg, p))

• return -EFAULT;
  

• old_tread = tu->tread;
• if (!xarg)

• tu->tread = TREAD_FORMAT_NONE;
  

• else if (!IS_ENABLED(CONFIG_64BITS) && !compat)

• tu->tread = TREAD_FORMAT_64BIT;
  

• else if (cmd == SNDRV_TIMER_IOCTL_TREAD64)

• tu->tread = TREAD_FORMAT_TIME64;
  

• else if (IS_ENABLED(CONFIG_X86))

• tu->tread = TREAD_FORMAT_TIME32_X86;
  

• else

• tu->tread = TREAD_FORMAT_TIME32;
  

• if (tu->tread != old_tread &&

•    realloc_user_queue(tu, tu->queue_size) < 0) {
  

• tu->tread = old_tread;
  

• return -ENOMEM;
  

• }
• return 0;

+}

enum { SNDRV_TIMER_IOCTL_START_OLD = _IO('T', 0x20), SNDRV_TIMER_IOCTL_STOP_OLD = _IO('T', 0x21), @@ -1927,7 +1988,7 @@ enum { };

static long __snd_timer_user_ioctl(struct file *file, unsigned int cmd,

• 		 unsigned long arg)
  

• 		 unsigned long arg, bool compat)
  

{ struct snd_timer_user *tu; void __user *argp = (void __user *)arg; @@ -1939,23 +2000,9 @@ static long __snd_timer_user_ioctl(struct file *file, unsigned int cmd, return put_user(SNDRV_TIMER_VERSION, p) ? -EFAULT : 0; case SNDRV_TIMER_IOCTL_NEXT_DEVICE: return snd_timer_user_next_device(argp);

• case SNDRV_TIMER_IOCTL_TREAD:
• {

• int xarg, old_tread;
  

• if (tu->timeri)	/* too late */
  

• 	return -EBUSY;
  

• if (get_user(xarg, p))
  

• 	return -EFAULT;
  

• old_tread = tu->tread;
  

• tu->tread = xarg ? 1 : 0;
  

• if (tu->tread != old_tread &&
  

•     realloc_user_queue(tu, tu->queue_size) < 0) {
  

• 	tu->tread = old_tread;
  

• 	return -ENOMEM;
  

• }
  

• return 0;
  

• }

• case SNDRV_TIMER_IOCTL_TREAD_OLD:
• case SNDRV_TIMER_IOCTL_TREAD64:

• return snd_timer_user_tread(argp, tu, cmd, compat);
  

  case SNDRV_TIMER_IOCTL_GINFO: return snd_timer_user_ginfo(file, argp); case SNDRV_TIMER_IOCTL_GPARAMS:

@@ -1995,7 +2042,7 @@ static long snd_timer_user_ioctl(struct file *file, unsigned int cmd, long ret;

mutex_lock(&tu->ioctl_lock);

• ret = __snd_timer_user_ioctl(file, cmd, arg);

• ret = __snd_timer_user_ioctl(file, cmd, arg, false); mutex_unlock(&tu->ioctl_lock); return ret;

} @@ -2017,7 +2064,24 @@ static ssize_t snd_timer_user_read(struct file *file, char __user *buffer, int err = 0;

tu = file->private_data;

• unit = tu->tread ? sizeof(struct snd_timer_tread) : sizeof(struct snd_timer_read);

• switch (tu->tread) {
• case TREAD_FORMAT_TIME32_X86:

• unit = sizeof(struct compat_snd_timer_tread64_x86_32);
  

• break;
  

• case TREAD_FORMAT_64BIT:
• case TREAD_FORMAT_TIME64:

• unit = sizeof(struct snd_timer_tread64);
  

• break;
  

• case TREAD_FORMAT_TIME32:

• unit = sizeof(struct snd_timer_tread);
  

• break;
  

• case TREAD_FORMAT_NONE:

• unit = sizeof(struct snd_timer_read);
  

• break;
  

• default:

• return -ENOTSUPP;
  

• }
• mutex_lock(&tu->ioctl_lock); spin_lock_irq(&tu->qlock); while ((long)count - result >= unit) {

@@ -2057,9 +2121,48 @@ static ssize_t snd_timer_user_read(struct file *file, char __user *buffer, spin_unlock_irq(&tu->qlock);

if (tu->tread) {

• 	if (copy_to_user(buffer, &tu->tqueue[qhead],
  

• 			 sizeof(struct snd_timer_tread)))
  

• 		err = -EFAULT;
  

• 	struct snd_timer_tread64 *tread = &tu->tqueue[qhead];
  

• 	struct snd_timer_tread tread32;
  

• 	struct compat_snd_timer_tread64_x86_32 compat_tread64;
  

• 	switch (tu->tread) {
  

• 	case TREAD_FORMAT_TIME32_X86:
  

• 		memset(&compat_tread64, 0, sizeof(compat_tread64));
  

• 		compat_tread64 = (struct compat_snd_timer_tread64_x86_32) {
  

• 			.event = tread->event,
  

• 			.tstamp_sec = tread->tstamp_sec,
  

• 			.tstamp_nsec = tread->tstamp_nsec,
  

• 			.val = tread->val,
  

• 		};
  

• 		if (copy_to_user(buffer, &compat_tread64,
  

• 				 sizeof(compat_tread64)))
  

• 			err = -EFAULT;
  

• 		break;
  

• 	case TREAD_FORMAT_64BIT:
  

• 	case TREAD_FORMAT_TIME64:
  

• 		if (copy_to_user(buffer, tread,
  

• 				 sizeof(struct snd_timer_tread64)))
  

• 			err = -EFAULT;
  

• 		break;
  

• 	case TREAD_FORMAT_TIME32:
  

• 		memset(&tread32, 0, sizeof(tread32));
  

• 		tread32 = (struct snd_timer_tread) {
  

• 			.event = tread->event,
  

• 			.tstamp = {
  

• 				.tv_sec = tread->tstamp_sec,
  

• 				.tv_nsec = tread->tstamp_nsec,
  

• 			},
  

• 			.val = tread->val,
  

• 		};
  

• 		if (copy_to_user(buffer, &tread32, sizeof(tread32)))
  

• 			err = -EFAULT;
  

• 		break;
  

• 	default:
  

• 		err = -ENOTSUPP;
  

• 		break;
  

• 	}
  

  } else { if (copy_to_user(buffer, &tu->queue[qhead], sizeof(struct snd_timer_read)))

diff --git a/sound/core/timer_compat.c b/sound/core/timer_compat.c index 3e09548..866e090 100644 --- a/sound/core/timer_compat.c +++ b/sound/core/timer_compat.c @@ -110,7 +110,15 @@ 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:

• return snd_timer_user_ioctl(file, cmd, (unsigned long)argp);
  

• {

• struct snd_timer_user *tu = file->private_data;
  

• long ret;
  

• mutex_lock(&tu->ioctl_lock);
  

• ret = __snd_timer_user_ioctl(file, cmd, arg, false);
  

• mutex_unlock(&tu->ioctl_lock);
  

• return ret;
  

• } case SNDRV_TIMER_IOCTL_GPARAMS32: return snd_timer_user_gparams_compat(file, argp); case SNDRV_TIMER_IOCTL_INFO32:

-- 1.7.9.5

---

Alsa-devel mailing list Alsa-devel@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel