diff mbox series

[v6,2/8] ALSA: Avoid using timespec for struct snd_timer_status

Message ID 20191112151642.680072-3-arnd@arndb.de
State Superseded
Headers show
Series Fix year 2038 issue for sound subsystem | expand

Commit Message

Arnd Bergmann Nov. 12, 2019, 3:16 p.m. UTC 
From: Baolin Wang <baolin.wang@linaro.org>


struct snd_timer_status uses 'timespec' type variables to record
timestamp, which will be changed to an incompatible layout with
updated user space using 64-bit time_t.

To handle both the old and the new layout on 32-bit architectures,
this patch introduces 'struct snd_timer_status32' and 'struct snd_timer_status64'
to handle 32bit time_t and 64bit time_t in native mode and compat mode,
which replaces timespec with s64 type.

When glibc changes time_t to 64-bit, any recompiled program will issue
ioctl commands that the kernel does not understand without this patch.

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

Signed-off-by: Arnd Bergmann <arnd@arndb.de>

---
 include/uapi/sound/asound.h |  2 ++
 sound/core/timer.c          | 62 +++++++++++++++++++++++++++++++++----
 sound/core/timer_compat.c   | 57 ++++------------------------------
 3 files changed, 64 insertions(+), 57 deletions(-)

-- 
2.20.0

Comments

Takashi Iwai Nov. 12, 2019, 3:42 p.m. UTC | #1 
On Tue, 12 Nov 2019 16:16:36 +0100,
Arnd Bergmann wrote:
> 

> From: Baolin Wang <baolin.wang@linaro.org>

> 

> struct snd_timer_status uses 'timespec' type variables to record

> timestamp, which will be changed to an incompatible layout with

> updated user space using 64-bit time_t.

> 

> To handle both the old and the new layout on 32-bit architectures,

> this patch introduces 'struct snd_timer_status32' and 'struct snd_timer_status64'

> to handle 32bit time_t and 64bit time_t in native mode and compat mode,

> which replaces timespec with s64 type.

> 

> When glibc changes time_t to 64-bit, any recompiled program will issue

> ioctl commands that the kernel does not understand without this patch.

> 

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

> Signed-off-by: Arnd Bergmann <arnd@arndb.de>

> ---

>  include/uapi/sound/asound.h |  2 ++

>  sound/core/timer.c          | 62 +++++++++++++++++++++++++++++++++----

>  sound/core/timer_compat.c   | 57 ++++------------------------------

>  3 files changed, 64 insertions(+), 57 deletions(-)

> 

> diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h

> index df1153cea0b7..930854f67fd3 100644

> --- a/include/uapi/sound/asound.h

> +++ b/include/uapi/sound/asound.h

> @@ -761,6 +761,7 @@ struct snd_timer_params {

>  	unsigned char reserved[60];	/* reserved */

>  };

>  

> +#ifndef __KERNEL__

>  struct snd_timer_status {

>  	struct timespec tstamp;		/* Timestamp - last update */

>  	unsigned int resolution;	/* current period resolution in ns */


Do we need this ifndef?  Is it for stopping the reference of struct
snd_timer_status from the kernel code but only 32 and 64 variants?


thanks,

Takashi
Arnd Bergmann Nov. 12, 2019, 8:08 p.m. UTC | #2 
On Tue, Nov 12, 2019 at 4:42 PM Takashi Iwai <tiwai@suse.de> wrote:

> > @@ -761,6 +761,7 @@ struct snd_timer_params {

> >       unsigned char reserved[60];     /* reserved */

> >  };

> >

> > +#ifndef __KERNEL__

> >  struct snd_timer_status {

> >       struct timespec tstamp;         /* Timestamp - last update */

> >       unsigned int resolution;        /* current period resolution in ns */

>

> Do we need this ifndef?  Is it for stopping the reference of struct

> snd_timer_status from the kernel code but only 32 and 64 variants?


Well spotted, this is indeed a very recent change I did to the patch.
The idea here is to hide any use of 'time_t', 'timespec' and 'timeval'
from kernel compilation. These types are now defined in an incompatible
way by libc, so we have to remove them from the kernel's uapi headers.
I would prefer to remove them completely from the kernel (rather than
moving them from uapi to internal headers) to make it harder to write
y2038-incompatible code, and with the 90 patches I sent this week,
all users are gone from the kernel (this series was the last part).

Interestingly, hiding snd_timer_status from the drivers /also/ caught
a but in a file when I had missed a reference that needed to be converted
to snd_timer_status64.

     Arnd
Takashi Iwai Nov. 12, 2019, 8:28 p.m. UTC | #3 
On Tue, 12 Nov 2019 21:08:17 +0100,
Arnd Bergmann wrote:
> 

> On Tue, Nov 12, 2019 at 4:42 PM Takashi Iwai <tiwai@suse.de> wrote:

> 

> > > @@ -761,6 +761,7 @@ struct snd_timer_params {

> > >       unsigned char reserved[60];     /* reserved */

> > >  };

> > >

> > > +#ifndef __KERNEL__

> > >  struct snd_timer_status {

> > >       struct timespec tstamp;         /* Timestamp - last update */

> > >       unsigned int resolution;        /* current period resolution in ns */

> >

> > Do we need this ifndef?  Is it for stopping the reference of struct

> > snd_timer_status from the kernel code but only 32 and 64 variants?

> 

> Well spotted, this is indeed a very recent change I did to the patch.

> The idea here is to hide any use of 'time_t', 'timespec' and 'timeval'

> from kernel compilation. These types are now defined in an incompatible

> way by libc, so we have to remove them from the kernel's uapi headers.

> I would prefer to remove them completely from the kernel (rather than

> moving them from uapi to internal headers) to make it harder to write

> y2038-incompatible code, and with the 90 patches I sent this week,

> all users are gone from the kernel (this series was the last part).


Could you put this trick in the changelog, too?

> Interestingly, hiding snd_timer_status from the drivers /also/ caught

> a but in a file when I had missed a reference that needed to be converted

> to snd_timer_status64.


Heh, that's no surprising, proving the usefulness :)


thanks,

Takashi
Arnd Bergmann Nov. 13, 2019, 3:09 p.m. UTC | #4 
On Tue, Nov 12, 2019 at 9:28 PM Takashi Iwai <tiwai@suse.de> wrote:

> > Well spotted, this is indeed a very recent change I did to the patch.

> > The idea here is to hide any use of 'time_t', 'timespec' and 'timeval'

> > from kernel compilation. These types are now defined in an incompatible

> > way by libc, so we have to remove them from the kernel's uapi headers.

> > I would prefer to remove them completely from the kernel (rather than

> > moving them from uapi to internal headers) to make it harder to write

> > y2038-incompatible code, and with the 90 patches I sent this week,

> > all users are gone from the kernel (this series was the last part).

>

> Could you put this trick in the changelog, too?


Done.

      Arnd
diff mbox series

Patch

diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h
index df1153cea0b7..930854f67fd3 100644
--- a/include/uapi/sound/asound.h
+++ b/include/uapi/sound/asound.h
@@ -761,6 +761,7 @@  struct snd_timer_params {
 	unsigned char reserved[60];	/* reserved */
 };
 
+#ifndef __KERNEL__
 struct snd_timer_status {
 	struct timespec tstamp;		/* Timestamp - last update */
 	unsigned int resolution;	/* current period resolution in ns */
@@ -769,6 +770,7 @@  struct snd_timer_status {
 	unsigned int queue;		/* used queue size */
 	unsigned char reserved[64];	/* reserved */
 };
+#endif
 
 #define SNDRV_TIMER_IOCTL_PVERSION	_IOR('T', 0x00, int)
 #define SNDRV_TIMER_IOCTL_NEXT_DEVICE	_IOWR('T', 0x01, struct snd_timer_id)
diff --git a/sound/core/timer.c b/sound/core/timer.c
index 29b99cfb5215..921f4ae2f9bf 100644
--- a/sound/core/timer.c
+++ b/sound/core/timer.c
@@ -65,6 +65,30 @@  struct snd_timer_user {
 	struct mutex ioctl_lock;
 };
 
+struct snd_timer_status32 {
+	s32 tstamp_sec;			/* Timestamp - last update */
+	s32 tstamp_nsec;
+	unsigned int resolution;	/* current period resolution in ns */
+	unsigned int lost;		/* counter of master tick lost */
+	unsigned int overrun;		/* count of read queue overruns */
+	unsigned int queue;		/* used queue size */
+	unsigned char reserved[64];	/* reserved */
+};
+
+#define SNDRV_TIMER_IOCTL_STATUS32	_IOR('T', 0x14, struct snd_timer_status32)
+
+struct snd_timer_status64 {
+	s64 tstamp_sec;			/* Timestamp - last update */
+	s64 tstamp_nsec;
+	unsigned int resolution;	/* current period resolution in ns */
+	unsigned int lost;		/* counter of master tick lost */
+	unsigned int overrun;		/* count of read queue overruns */
+	unsigned int queue;		/* used queue size */
+	unsigned char reserved[64];	/* reserved */
+};
+
+#define SNDRV_TIMER_IOCTL_STATUS64	_IOR('T', 0x14, struct snd_timer_status64)
+
 /* list of timers */
 static LIST_HEAD(snd_timer_list);
 
@@ -1861,17 +1885,41 @@  static int snd_timer_user_params(struct file *file,
 	return err;
 }
 
-static int snd_timer_user_status(struct file *file,
-				 struct snd_timer_status __user *_status)
+static int snd_timer_user_status32(struct file *file,
+				   struct snd_timer_status32 __user *_status)
+ {
+	struct snd_timer_user *tu;
+	struct snd_timer_status32 status;
+
+	tu = file->private_data;
+	if (!tu->timeri)
+		return -EBADFD;
+	memset(&status, 0, sizeof(status));
+	status.tstamp_sec = tu->tstamp.tv_sec;
+	status.tstamp_nsec = tu->tstamp.tv_nsec;
+	status.resolution = snd_timer_resolution(tu->timeri);
+	status.lost = tu->timeri->lost;
+	status.overrun = tu->overrun;
+	spin_lock_irq(&tu->qlock);
+	status.queue = tu->qused;
+	spin_unlock_irq(&tu->qlock);
+	if (copy_to_user(_status, &status, sizeof(status)))
+		return -EFAULT;
+	return 0;
+}
+
+static int snd_timer_user_status64(struct file *file,
+				   struct snd_timer_status64 __user *_status)
 {
 	struct snd_timer_user *tu;
-	struct snd_timer_status status;
+	struct snd_timer_status64 status;
 
 	tu = file->private_data;
 	if (!tu->timeri)
 		return -EBADFD;
 	memset(&status, 0, sizeof(status));
-	status.tstamp = timespec64_to_timespec(tu->tstamp);
+	status.tstamp_sec = tu->tstamp.tv_sec;
+	status.tstamp_nsec = tu->tstamp.tv_nsec;
 	status.resolution = snd_timer_resolution(tu->timeri);
 	status.lost = tu->timeri->lost;
 	status.overrun = tu->overrun;
@@ -1995,8 +2043,10 @@  static long __snd_timer_user_ioctl(struct file *file, unsigned int cmd,
 		return snd_timer_user_info(file, argp);
 	case SNDRV_TIMER_IOCTL_PARAMS:
 		return snd_timer_user_params(file, argp);
-	case SNDRV_TIMER_IOCTL_STATUS:
-		return snd_timer_user_status(file, argp);
+	case SNDRV_TIMER_IOCTL_STATUS32:
+		return snd_timer_user_status32(file, argp);
+	case SNDRV_TIMER_IOCTL_STATUS64:
+		return snd_timer_user_status64(file, argp);
 	case SNDRV_TIMER_IOCTL_START:
 	case SNDRV_TIMER_IOCTL_START_OLD:
 		return snd_timer_user_start(file);
diff --git a/sound/core/timer_compat.c b/sound/core/timer_compat.c
index bb6be484dfd3..20eef5bc304b 100644
--- a/sound/core/timer_compat.c
+++ b/sound/core/timer_compat.c
@@ -69,54 +69,11 @@  static int snd_timer_user_info_compat(struct file *file,
 	return 0;
 }
 
-struct snd_timer_status32 {
-	struct compat_timespec tstamp;
-	u32 resolution;
-	u32 lost;
-	u32 overrun;
-	u32 queue;
-	unsigned char reserved[64];
-};
-
-static int snd_timer_user_status_compat(struct file *file,
-					struct snd_timer_status32 __user *_status)
-{
-	struct snd_timer_user *tu;
-	struct snd_timer_status32 status;
-	
-	tu = file->private_data;
-	if (!tu->timeri)
-		return -EBADFD;
-	memset(&status, 0, sizeof(status));
-	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;
-	spin_lock_irq(&tu->qlock);
-	status.queue = tu->qused;
-	spin_unlock_irq(&tu->qlock);
-	if (copy_to_user(_status, &status, sizeof(status)))
-		return -EFAULT;
-	return 0;
-}
-
-#ifdef CONFIG_X86_X32
-/* X32 ABI has the same struct as x86-64 */
-#define snd_timer_user_status_x32(file, s) \
-	snd_timer_user_status(file, s)
-#endif /* CONFIG_X86_X32 */
-
-/*
- */
-
 enum {
 	SNDRV_TIMER_IOCTL_GPARAMS32 = _IOW('T', 0x04, struct snd_timer_gparams32),
 	SNDRV_TIMER_IOCTL_INFO32 = _IOR('T', 0x11, struct snd_timer_info32),
-	SNDRV_TIMER_IOCTL_STATUS32 = _IOW('T', 0x14, struct snd_timer_status32),
-#ifdef CONFIG_X86_X32
-	SNDRV_TIMER_IOCTL_STATUS_X32 = _IOW('T', 0x14, struct snd_timer_status),
-#endif /* CONFIG_X86_X32 */
+	SNDRV_TIMER_IOCTL_STATUS_COMPAT32 = _IOW('T', 0x14, struct snd_timer_status32),
+	SNDRV_TIMER_IOCTL_STATUS_COMPAT64 = _IOW('T', 0x14, struct snd_timer_status64),
 };
 
 static long __snd_timer_user_ioctl_compat(struct file *file, unsigned int cmd,
@@ -145,12 +102,10 @@  static long __snd_timer_user_ioctl_compat(struct file *file, unsigned int cmd,
 		return snd_timer_user_gparams_compat(file, argp);
 	case SNDRV_TIMER_IOCTL_INFO32:
 		return snd_timer_user_info_compat(file, argp);
-	case SNDRV_TIMER_IOCTL_STATUS32:
-		return snd_timer_user_status_compat(file, argp);
-#ifdef CONFIG_X86_X32
-	case SNDRV_TIMER_IOCTL_STATUS_X32:
-		return snd_timer_user_status_x32(file, argp);
-#endif /* CONFIG_X86_X32 */
+	case SNDRV_TIMER_IOCTL_STATUS_COMPAT32:
+		return snd_timer_user_status32(file, argp);
+	case SNDRV_TIMER_IOCTL_STATUS_COMPAT64:
+		return snd_timer_user_status64(file, argp);
 	}
 	return -ENOIOCTLCMD;
 }