Date:   Sun, 05 Nov 2017 11:29:32 +0100
From:   Takashi Iwai <tiwai@...e.de>
To:     Baolin Wang <baolin.wang@...aro.org>
Cc:     perex@...ex.cz, arnd@...db.de, fabf@...net.be,
        arvind.yadav.cs@...il.com, linux-kernel@...r.kernel.org,
        alsa-devel@...a-project.org, vinod.koul@...el.com,
        hardik.t.shah@...el.com, guneshwor.o.singh@...el.com,
        lgirdwood@...il.com, elfring@...rs.sourceforge.net,
        gudishax.kranthikumar@...el.com, broonie@...nel.org,
        bhumirks@...il.com, naveen.m@...el.com, jeeja.kp@...el.com,
        o-takashi@...amocchi.jp, subhransu.s.prusty@...el.com,
        mingo@...nel.org, dan.carpenter@...cle.com
Subject: 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@...aro.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@...a-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
> 

Powered by blists - more mailing lists