[v5,6/8] media: v4l2-core: fix v4l2_buffer handling for time64 ABI

Message ID 20191126161824.337724-7-arnd@arndb.de
State Superseded
Headers show
Series
  • [v5,1/8] media: documentation: fix video_event description
Related show

Commit Message

Arnd Bergmann Nov. 26, 2019, 4:18 p.m.
The v4l2_buffer structure contains a 'struct timeval' member that is
defined by the user space C library, creating an ABI incompatibility
when that gets updated to a 64-bit time_t.

As in v4l2_event, handle this with a special case in video_put_user()
and video_get_user() to replace the memcpy there.

Since the structure also contains a pointer, there are now two
native versions (on 32-bit systems) as well as two compat versions
(on 64-bit systems), which unfortunately complicates the compat
handler quite a bit.

Duplicating the existing handlers for the new types is a safe
conversion for now, but unfortunately this may turn into a
maintenance burden later. A larger-scale rework of the
compat code might be a better alternative, but is out of scope
of the y2038 work.

Sparc64 needs a special case because of their special suseconds_t
definition.

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

---
 drivers/media/v4l2-core/v4l2-ioctl.c | 73 ++++++++++++++++++++++++++--
 include/media/v4l2-ioctl.h           | 30 ++++++++++++
 include/uapi/linux/videodev2.h       | 23 +++++++++
 3 files changed, 122 insertions(+), 4 deletions(-)

-- 
2.20.0

Comments

Hans Verkuil Dec. 13, 2019, 3:32 p.m. | #1
On 12/13/19 4:08 PM, Arnd Bergmann wrote:
> On Thu, Dec 12, 2019 at 4:43 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:

>>

>> On 11/26/19 5:18 PM, Arnd Bergmann wrote:

>>>

>>>       switch (cmd) {

>>> +#ifdef COMPAT_32BIT_TIME

>>

>> COMPAT_32BIT_TIME -> CONFIG_COMPAT_32BIT_TIME

> 

> Fixed.

> 

>>> +             *vb = (struct v4l2_buffer) {

>>> +                     .index          = vb32.index,

>>> +                     .type           = vb32.type,

>>> +                     .bytesused      = vb32.bytesused,

>>> +                     .flags          = vb32.flags,

>>> +                     .field          = vb32.field,

>>> +                     .timestamp.tv_sec       = vb32.timestamp.tv_sec,

>>> +                     .timestamp.tv_usec      = vb32.timestamp.tv_usec,

>>> +                     .timecode       = vb32.timecode,

>>

>> You forgot to copy sequence.

>>

>>> +                     .memory         = vb32.memory,

>>> +                     .m.userptr      = vb32.m.usercopy,

>>

>> usercopy -> userptr

> 

> Fixed.

> 

>>> +                     .length         = vb32.length,

>>> +                     .request_fd     = vb32.request_fd,

>>> +             };

>>> +

>>> +             if (cmd == VIDIOC_QUERYBUF_TIME32)

>>> +                     memset(&vb->length, 0, sizeof(*vb) -

>>> +                            offsetof(struct v4l2_buffer, length));

>>

>> It's from the field AFTER vb->length that this needs to be zeroed. It's best to

>> use the CLEAR_AFTER_FIELD macro here.

> 

> I'm a bit lost about this one: the fields that are not explicitly

> uninitialized here are already set to zero by the assignment

> above. Should this simply be a

> 

>                 if (cmd == VIDIOC_QUERYBUF_TIME32)

>                        vb->request_fd = 0;


Yes, you are correct. That's much simpler.

> 

> then? I don't remember where that memset() originally came

> from or why request_fd has to be cleared here.

> 

>>> @@ -3100,6 +3141,30 @@ static int video_put_user(void __user *arg, void *parg, unsigned int cmd)

>>>                       return -EFAULT;

>>>               break;

>>>       }

>>> +     case VIDIOC_QUERYBUF_TIME32:

>>> +     case VIDIOC_QBUF_TIME32:

>>> +     case VIDIOC_DQBUF_TIME32:

>>> +     case VIDIOC_PREPARE_BUF_TIME32: {

>>> +             struct v4l2_buffer *vb = parg;

>>> +             struct v4l2_buffer_time32 vb32 = {

>>> +                     .index          = vb->index,

>>> +                     .type           = vb->type,

>>> +                     .bytesused      = vb->bytesused,

>>> +                     .flags          = vb->flags,

>>> +                     .field          = vb->field,

>>> +                     .timestamp.tv_sec       = vb->timestamp.tv_sec,

>>> +                     .timestamp.tv_usec      = vb->timestamp.tv_usec,

>>> +                     .timecode       = vb->timecode,

>>

>> You forgot to copy sequence.

> 

> Fixed.

> 

>> With these changes this patch series passed both the 64 and 32 bit compliance

>> tests (in fact, all the issues mentioned above were found with these compliance

>> tests).

> 

> Yay compliance tests!

> 

>> I am unable to test with musl since v4l2-ctl and v4l2-compliance are C++ programs,

>> and there doesn't appear to be an easy way to compile a C++ program with musl.

>>

>> If you happen to have a test environment where you can compile C++ with musl,

>> then let me know and I can give instructions on how to run the compliance tests.

>>

>> If you can't test that, then I can merge this regardless, and hope for the best

>> once the Y2038 fixes end up in glibc. But ideally I'd like to have this tested.

> 

> I've heard good things about the prebuilt toolchains from http://musl.cc/.

> These seems to come with a libstdc++, but I have not tried that myself.


I'll see if I can give those a spin, but if I can't get it to work quickly,
then I don't plan on spending much time on it.

Regards,

	Hans

> 

> I've folded the change below into this patch in my y2038-v4l2-v6 branch

> but have not been able to update the copy on git.kernel.org yet because of

> server-side issues today.

> 

>           Arnd

> 

> 8<-----

> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c

> b/drivers/media/v4l2-core/v4l2-ioctl.c

> index c416870a3166..667225712343 100644

> --- a/drivers/media/v4l2-core/v4l2-ioctl.c

> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c

> @@ -3055,7 +3055,7 @@ static int video_get_user(void __user *arg, void

> *parg, unsigned int cmd,

>         }

> 

>         switch (cmd) {

> -#ifdef COMPAT_32BIT_TIME

> +#ifdef CONFIG_COMPAT_32BIT_TIME

>         case VIDIOC_QUERYBUF_TIME32:

>         case VIDIOC_QBUF_TIME32:

>         case VIDIOC_DQBUF_TIME32:

> @@ -3075,15 +3075,15 @@ static int video_get_user(void __user *arg,

> void *parg, unsigned int cmd,

>                         .timestamp.tv_sec       = vb32.timestamp.tv_sec,

>                         .timestamp.tv_usec      = vb32.timestamp.tv_usec,

>                         .timecode       = vb32.timecode,

> +                       .sequence       = vb32.sequence,

>                         .memory         = vb32.memory,

> -                       .m.userptr      = vb32.m.usercopy,

> +                       .m.userptr      = vb32.m.userptr,

>                         .length         = vb32.length,

>                         .request_fd     = vb32.request_fd,

>                 };

> 

>                 if (cmd == VIDIOC_QUERYBUF_TIME32)

> -                       memset(&vb->length, 0, sizeof(*vb) -

> -                              offsetof(struct v4l2_buffer, length));

> +                       vb->request_fd = 0;

> 

>                 break;

>         }

> @@ -3155,6 +3155,7 @@ static int video_put_user(void __user *arg, void

> *parg, unsigned int cmd)

>                         .timestamp.tv_sec       = vb->timestamp.tv_sec,

>                         .timestamp.tv_usec      = vb->timestamp.tv_usec,

>                         .timecode       = vb->timecode,

> +                       .sequence       = vb->sequence,

>                         .memory         = vb->memory,

>                         .m.userptr      = vb->m.userptr,

>                         .length         = vb->length,

>
Arnd Bergmann Dec. 14, 2019, 9:44 p.m. | #2
On Sat, Dec 14, 2019 at 12:27 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>

> On 12/13/19 4:32 PM, Hans Verkuil wrote:

> >>> I am unable to test with musl since v4l2-ctl and v4l2-compliance are C++ programs,

> >>> and there doesn't appear to be an easy way to compile a C++ program with musl.

> >>>

> >>> If you happen to have a test environment where you can compile C++ with musl,

> >>> then let me know and I can give instructions on how to run the compliance tests.

> >>>

> >>> If you can't test that, then I can merge this regardless, and hope for the best

> >>> once the Y2038 fixes end up in glibc. But ideally I'd like to have this tested.

> >>

> >> I've heard good things about the prebuilt toolchains from http://musl.cc/.

> >> These seems to come with a libstdc++, but I have not tried that myself.

> >

> > I'll see if I can give those a spin, but if I can't get it to work quickly,

> > then I don't plan on spending much time on it.

>

> I managed to build v4l2-ctl/compliance with those toolchains, but they seem to be

> still using a 32-bit time_t.

>

> Do I need to get a specific version or do something special?


My mistake: only musl-1.2.0 and up have 64-bit time_t, but this isn't released
yet. According to https://wiki.musl-libc.org/roadmap.html, the release
was planned
for last month, no idea how long it will take.

It appears that a snapshot build at
http://more.musl.cc/7.5.0/x86_64-linux-musl/i686-linux-musl-native.tgz
is new enough to have 64-bit time_t (according to include/bits/alltypes.h),
but this is a month old as well, so it may have known bugs.

Adding Zach to Cc here, maybe he already has plans for another build with
the latest version.

       Arnd
Hans Verkuil Dec. 15, 2019, 5:26 p.m. | #3
On 12/14/19 10:44 PM, Arnd Bergmann wrote:
> On Sat, Dec 14, 2019 at 12:27 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:

>>

>> On 12/13/19 4:32 PM, Hans Verkuil wrote:

>>>>> I am unable to test with musl since v4l2-ctl and v4l2-compliance are C++ programs,

>>>>> and there doesn't appear to be an easy way to compile a C++ program with musl.

>>>>>

>>>>> If you happen to have a test environment where you can compile C++ with musl,

>>>>> then let me know and I can give instructions on how to run the compliance tests.

>>>>>

>>>>> If you can't test that, then I can merge this regardless, and hope for the best

>>>>> once the Y2038 fixes end up in glibc. But ideally I'd like to have this tested.

>>>>

>>>> I've heard good things about the prebuilt toolchains from http://musl.cc/.

>>>> These seems to come with a libstdc++, but I have not tried that myself.

>>>

>>> I'll see if I can give those a spin, but if I can't get it to work quickly,

>>> then I don't plan on spending much time on it.

>>

>> I managed to build v4l2-ctl/compliance with those toolchains, but they seem to be

>> still using a 32-bit time_t.

>>

>> Do I need to get a specific version or do something special?

> 

> My mistake: only musl-1.2.0 and up have 64-bit time_t, but this isn't released

> yet. According to https://wiki.musl-libc.org/roadmap.html, the release

> was planned

> for last month, no idea how long it will take.

> 

> It appears that a snapshot build at

> http://more.musl.cc/7.5.0/x86_64-linux-musl/i686-linux-musl-native.tgz

> is new enough to have 64-bit time_t (according to include/bits/alltypes.h),

> but this is a month old as well, so it may have known bugs.


Ah, great, that worked, after applying the patch below.

Both struct v4l2_buffer32 and v4l2_event32 need to be packed, otherwise you would
get an additional 4 bytes since the 64 bit compiler wants to align the 8 byte tv_secs
to an 8 byte boundary. But that's not what the i686 compiler does.

If I remember correctly, packed is only needed for CONFIG_X86_64.

With these changes (plus a bunch of fixes for v4l-utils) I was able to do full
compliance tests for 64-bit, 32-bit time32 under x86_64, 32-bit time64 under x86_64,
time32 under i686 and time64 under i686.

Arnd, if you can post a v6 with the previous fixes and this fix included, then
I'll make a pull request for this for kernel 5.6.

Regards,

	Hans


Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>

---
diff --git a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
index 3bbf47d950e0..c01492cf6160 100644
--- a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
+++ b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
@@ -492,7 +492,11 @@ struct v4l2_buffer32 {
 	__u32			length;
 	__u32			reserved2;
 	__s32			request_fd;
+#ifdef CONFIG_X86_64
+} __attribute__ ((packed));
+#else
 };
+#endif

 struct v4l2_buffer32_time32 {
 	__u32			index;
@@ -1280,7 +1284,7 @@ struct v4l2_event32 {
 	struct __kernel_timespec	timestamp;
 	__u32				id;
 	__u32				reserved[8];
-};
+} __attribute__ ((packed));

 struct v4l2_event32_time32 {
 	__u32				type;
Arnd Bergmann Dec. 16, 2019, 9:29 a.m. | #4
On Sun, Dec 15, 2019 at 6:26 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>

> Ah, great, that worked, after applying the patch below.

>

> Both struct v4l2_buffer32 and v4l2_event32 need to be packed, otherwise you would

> get an additional 4 bytes since the 64 bit compiler wants to align the 8 byte tv_secs

> to an 8 byte boundary. But that's not what the i686 compiler does.


Thanks so much for the testing and finding this issue. It would be much more
embarrassing to find it later, given that I explained how it's supposed to work
in the comment above v4l2_event32 and in the documentation I just submitted
but got it wrong anyway ;-)

> If I remember correctly, packed is only needed for CONFIG_X86_64.


Correct.

> diff --git a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c

> index 3bbf47d950e0..c01492cf6160 100644

> --- a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c

> +++ b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c

> @@ -492,7 +492,11 @@ struct v4l2_buffer32 {

>         __u32                   length;

>         __u32                   reserved2;

>         __s32                   request_fd;

> +#ifdef CONFIG_X86_64

> +} __attribute__ ((packed));

> +#else

>  };

> +#endif


I would prefer to write it like this instead to avoid the #ifdef, the
effect should
be the same:

--- a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
+++ b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
@@ -475,8 +475,8 @@ struct v4l2_buffer32 {
        __u32                   flags;
        __u32                   field;  /* enum v4l2_field */
        struct {
-               long long       tv_sec;
-               long long       tv_usec;
+               compat_s64      tv_sec;
+               compat_s64      tv_usec;
        }                       timestamp;
        struct v4l2_timecode    timecode;
        __u32                   sequence;
--- a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
+++ b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
@@ -1277,7 +1277,10 @@ struct v4l2_event32 {
        } u;
        __u32                           pending;
        __u32                           sequence;
-       struct __kernel_timespec        timestamp;
+       struct {
+               compat_s64              tv_sec;
+               compat_s64              tv_usec;
+       } timestamp;
        __u32                           id;
        __u32                           reserved[8];
 };

If you agree, I'll push out a modified branch with that version and send out
that series to the list again.

There is one more complication that I just noticed: The "struct v4l2_buffer32"
definition has always been defined in a way that works for i386 user space
but is broken for x32 user space. The version I used accidentally fixed x32
while breaking i386. With the change above, it's back to missing x32 support
(so nothing changed).

There is no way to fix the uapi definition of v4l2_buffer to have x32 and i386
use the same format, because applications may be using old headers, but
I suppose I could add yet another version of the struct to correctly deal with
x32, or just add a comment like

--- a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
+++ b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
@@ -468,6 +468,10 @@ struct v4l2_plane32 {
        __u32                   reserved[11];
 };

+/*
+ * This is correct for all architectures including i386, but not x32,
+ * which has different alignment requirements for timestamp
+ */
 struct v4l2_buffer32 {
        __u32                   index;
        __u32                   type;   /* enum v4l2_buf_type */


      Arnd

Patch

diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
index 96aafb659783..4d611a847462 100644
--- a/drivers/media/v4l2-core/v4l2-ioctl.c
+++ b/drivers/media/v4l2-core/v4l2-ioctl.c
@@ -474,10 +474,10 @@  static void v4l_print_buffer(const void *arg, bool write_only)
 	const struct v4l2_plane *plane;
 	int i;
 
-	pr_cont("%02ld:%02d:%02d.%08ld index=%d, type=%s, request_fd=%d, flags=0x%08x, field=%s, sequence=%d, memory=%s",
-			p->timestamp.tv_sec / 3600,
-			(int)(p->timestamp.tv_sec / 60) % 60,
-			(int)(p->timestamp.tv_sec % 60),
+	pr_cont("%02d:%02d:%02d.%09ld index=%d, type=%s, request_fd=%d, flags=0x%08x, field=%s, sequence=%d, memory=%s",
+			(int)p->timestamp.tv_sec / 3600,
+			((int)p->timestamp.tv_sec / 60) % 60,
+			((int)p->timestamp.tv_sec % 60),
 			(long)p->timestamp.tv_usec,
 			p->index,
 			prt_names(p->type, v4l2_type_names), p->request_fd,
@@ -3029,6 +3029,14 @@  static unsigned int video_translate_cmd(unsigned int cmd)
 #ifdef CONFIG_COMPAT_32BIT_TIME
 	case VIDIOC_DQEVENT_TIME32:
 		return VIDIOC_DQEVENT;
+	case VIDIOC_QUERYBUF_TIME32:
+		return VIDIOC_QUERYBUF;
+	case VIDIOC_QBUF_TIME32:
+		return VIDIOC_QBUF;
+	case VIDIOC_DQBUF_TIME32:
+		return VIDIOC_DQBUF;
+	case VIDIOC_PREPARE_BUF_TIME32:
+		return VIDIOC_PREPARE_BUF;
 #endif
 	}
 
@@ -3047,6 +3055,39 @@  static int video_get_user(void __user *arg, void *parg, unsigned int cmd,
 	}
 
 	switch (cmd) {
+#ifdef COMPAT_32BIT_TIME
+	case VIDIOC_QUERYBUF_TIME32:
+	case VIDIOC_QBUF_TIME32:
+	case VIDIOC_DQBUF_TIME32:
+	case VIDIOC_PREPARE_BUF_TIME32: {
+		struct v4l2_buffer_time32 vb32;
+		struct v4l2_buffer *vb = parg;
+
+		if (copy_from_user(&vb32, arg, sizeof(vb32)))
+			return -EFAULT;
+
+		*vb = (struct v4l2_buffer) {
+			.index		= vb32.index,
+			.type		= vb32.type,
+			.bytesused	= vb32.bytesused,
+			.flags		= vb32.flags,
+			.field		= vb32.field,
+			.timestamp.tv_sec	= vb32.timestamp.tv_sec,
+			.timestamp.tv_usec	= vb32.timestamp.tv_usec,
+			.timecode	= vb32.timecode,
+			.memory		= vb32.memory,
+			.m.userptr	= vb32.m.usercopy,
+			.length		= vb32.length,
+			.request_fd	= vb32.request_fd,
+		};
+
+		if (cmd == VIDIOC_QUERYBUF_TIME32)
+			memset(&vb->length, 0, sizeof(*vb) -
+			       offsetof(struct v4l2_buffer, length));
+
+		break;
+	}
+#endif
 	default:
 		/*
 		 * In some cases, only a few fields are used as input,
@@ -3100,6 +3141,30 @@  static int video_put_user(void __user *arg, void *parg, unsigned int cmd)
 			return -EFAULT;
 		break;
 	}
+	case VIDIOC_QUERYBUF_TIME32:
+	case VIDIOC_QBUF_TIME32:
+	case VIDIOC_DQBUF_TIME32:
+	case VIDIOC_PREPARE_BUF_TIME32: {
+		struct v4l2_buffer *vb = parg;
+		struct v4l2_buffer_time32 vb32 = {
+			.index		= vb->index,
+			.type		= vb->type,
+			.bytesused	= vb->bytesused,
+			.flags		= vb->flags,
+			.field		= vb->field,
+			.timestamp.tv_sec	= vb->timestamp.tv_sec,
+			.timestamp.tv_usec	= vb->timestamp.tv_usec,
+			.timecode	= vb->timecode,
+			.memory		= vb->memory,
+			.m.userptr	= vb->m.userptr,
+			.length		= vb->length,
+			.request_fd	= vb->request_fd,
+		};
+
+		if (copy_to_user(arg, &vb32, sizeof(vb32)))
+			return -EFAULT;
+		break;
+	}
 #endif
 	default:
 		/*  Copy results into user buffer  */
diff --git a/include/media/v4l2-ioctl.h b/include/media/v4l2-ioctl.h
index 05c1ec93a911..86878fba332b 100644
--- a/include/media/v4l2-ioctl.h
+++ b/include/media/v4l2-ioctl.h
@@ -749,4 +749,34 @@  struct v4l2_event_time32 {
 
 #define	VIDIOC_DQEVENT_TIME32	 _IOR('V', 89, struct v4l2_event_time32)
 
+struct v4l2_buffer_time32 {
+	__u32			index;
+	__u32			type;
+	__u32			bytesused;
+	__u32			flags;
+	__u32			field;
+	struct old_timeval32	timestamp;
+	struct v4l2_timecode	timecode;
+	__u32			sequence;
+
+	/* memory location */
+	__u32			memory;
+	union {
+		__u32           offset;
+		unsigned long   userptr;
+		struct v4l2_plane *planes;
+		__s32		fd;
+	} m;
+	__u32			length;
+	__u32			reserved2;
+	union {
+		__s32		request_fd;
+		__u32		reserved;
+	};
+};
+#define VIDIOC_QUERYBUF_TIME32	_IOWR('V',  9, struct v4l2_buffer_time32)
+#define VIDIOC_QBUF_TIME32	_IOWR('V', 15, struct v4l2_buffer_time32)
+#define VIDIOC_DQBUF_TIME32	_IOWR('V', 17, struct v4l2_buffer_time32)
+#define VIDIOC_PREPARE_BUF_TIME32 _IOWR('V', 93, struct v4l2_buffer_time32)
+
 #endif /* _V4L2_IOCTL_H */
diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
index caf156d45842..5f9357dcb060 100644
--- a/include/uapi/linux/videodev2.h
+++ b/include/uapi/linux/videodev2.h
@@ -912,6 +912,25 @@  struct v4l2_jpegcompression {
 /*
  *	M E M O R Y - M A P P I N G   B U F F E R S
  */
+
+#ifdef __KERNEL__
+/*
+ * This corresponds to the user space version of timeval
+ * for 64-bit time_t. sparc64 is different from everyone
+ * else, using the microseconds in the wrong half of the
+ * second 64-bit word.
+ */
+struct __kernel_v4l2_timeval {
+	long long	tv_sec;
+#if defined(__sparc__) && defined(__arch64__)
+	int		tv_usec;
+	int		__pad;
+#else
+	long long	tv_usec;
+#endif
+};
+#endif
+
 struct v4l2_requestbuffers {
 	__u32			count;
 	__u32			type;		/* enum v4l2_buf_type */
@@ -997,7 +1016,11 @@  struct v4l2_buffer {
 	__u32			bytesused;
 	__u32			flags;
 	__u32			field;
+#ifdef __KERNEL__
+	struct __kernel_v4l2_timeval timestamp;
+#else
 	struct timeval		timestamp;
+#endif
 	struct v4l2_timecode	timecode;
 	__u32			sequence;