mbox series

[v5,0/8] y2038 safety in v4l2

Message ID 20191126161824.337724-1-arnd@arndb.de
Headers show
Series y2038 safety in v4l2 | expand

Message

Arnd Bergmann Nov. 26, 2019, 4:18 p.m. UTC
I'm in the process of finishing up the last bits on y2038-unsafe
code in the kernel, this series is for v4l2, which has no problem
with overflow, but has multiple ioctls that break with user space
built against a new 32-bit libc.

I posted similar patches as part of a series back in 2015, the
new version was rewritten from scratch and I double-checked with
the old version to make sure I did not miss anything I had already
taken care of before.

Hans Verkuil worked on a different patch set in 2017, but this
also did not get to the point of being merged.

My new version contains compat-ioctl support, which the old one
did not and should be complete, but given its size likely contains
bugs. I did randconfig build tests, but no runtime test, so
careful review as well as testing would be much appreciated.

With this version, the newly added code takes care of the existing
ABI, while the existing code got moved to the 64-bit time_t
interface and is used internally. This means that testing with
existing binaries should exercise most of the modifications
and if that works and doesn't get shot down in review, we can
probably live without testing the new ABI explicitly.

I'm not entirely happy with the compat-ioctl implementation that
adds quite a bit of code duplication, but I hope this is
acceptable anyway, as a better implementation would likely
require a larger refactoring of the compat-ioctl file, while
my approach simply adds support for the additional data structure
variants.

I uploaded git branch on top of the v4l2/dvb branch to [2].

      Arnd

Changes since v4:

- Move non-public contents out of uapi header
- split out __kernel_v4l2_timeval into separate struct
- use compound initializers for v4l2_event_time32 and
  v4l2_buffer_time32 conversion
- add comment for v4l2_buffer_get_timestamp()

Changes since v3:

- This is a minor update compared to version 3 of this series,
  with bugfixes for small mistakes that I found or that were
  reported by automated build bots.


[1] https://lwn.net/Articles/657754/
[2] https://git.kernel.org/pub/scm/linux/kernel/git/arnd/playground.git/log/?h=y2038-v4l2-v5

Arnd Bergmann (8):
  media: documentation: fix video_event description
  media: v4l2: abstract timeval handling in v4l2_buffer
  media: v4l2-core: compat: ignore native command codes
  media: v4l2-core: split out data copy from video_usercopy
  media: v4l2-core: fix VIDIOC_DQEVENT for time64 ABI
  media: v4l2-core: fix v4l2_buffer handling for time64 ABI
  media: v4l2-core: fix compat VIDIOC_DQEVENT for time64 ABI
  media: v4l2-core: fix compat v4l2_buffer handling for time64 ABI

 .../media/uapi/dvb/video-get-event.rst        |   2 +-
 Documentation/media/uapi/dvb/video_types.rst  |   2 +-
 .../media/common/videobuf2/videobuf2-v4l2.c   |   4 +-
 drivers/media/pci/meye/meye.c                 |   4 +-
 drivers/media/usb/cpia2/cpia2_v4l.c           |   4 +-
 drivers/media/usb/stkwebcam/stk-webcam.c      |   2 +-
 drivers/media/usb/usbvision/usbvision-video.c |   4 +-
 drivers/media/v4l2-core/v4l2-compat-ioctl32.c | 470 +++++++++++++++---
 drivers/media/v4l2-core/v4l2-event.c          |   5 +-
 drivers/media/v4l2-core/v4l2-ioctl.c          | 209 ++++++--
 drivers/media/v4l2-core/v4l2-subdev.c         |  26 +-
 drivers/media/v4l2-core/videobuf-core.c       |   5 +-
 include/media/v4l2-common.h                   |  21 +
 include/media/v4l2-ioctl.h                    |  55 ++
 include/trace/events/v4l2.h                   |   2 +-
 include/uapi/linux/videodev2.h                |  29 ++
 16 files changed, 709 insertions(+), 135 deletions(-)

-- 
2.20.0

Comments

Hans Verkuil Dec. 12, 2019, 3:43 p.m. UTC | #1
On 11/26/19 5:18 PM, Arnd Bergmann wrote:
> 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(-)

> 

> 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


COMPAT_32BIT_TIME -> CONFIG_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,


You forgot to copy sequence.

> +			.memory		= vb32.memory,

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


usercopy -> 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));


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.

> +

> +		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,


You forgot to copy sequence.

> +			.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;

>  

> 


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).

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.

Regards,

	Hans
Arnd Bergmann Dec. 13, 2019, 3:08 p.m. UTC | #2
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;

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'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. 13, 2019, 3:38 p.m. UTC | #3
On Fri, Dec 13, 2019 at 4:33 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
> 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:

> >

> > 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.


Ok, sounds good. The way the series is structured, I tried to have the
time64 ioctls use the existing code, while adding new time32 ioctls
to ensure that we catch the bugs in the new time32 version through
testing, and have fewer bugs to start with in the time64 version.

     Arnd
Hans Verkuil Dec. 14, 2019, 11:27 a.m. UTC | #4
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?

Regards,

	Hans
Hans Verkuil Dec. 16, 2019, 10:28 a.m. UTC | #5
On 12/16/19 10:29 AM, Arnd Bergmann wrote:
> 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.


That's fine. I did a quick test with this and it looks fine.

> 

> 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

> 


Go with a comment. We've never tested with x32 to be honest. There were discussions
about a year ago of dropping x32 altogether, but that hasn't happened yet.

Regards,

	Hans
Zach van Rijn Dec. 19, 2019, 11:26 p.m. UTC | #6
On Sat, 2019-12-14 at 22:44 +0100, 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'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.


Yes, that's correct. The current (as of 2019-12-19) GCC 9.2.1
offering is based on musl 1.1.24, though the 7.5.0 release is
using a more recent git tag only due to timing/availability.

Within reason, I'm happy to bump versions with justification.
Rebuilding takes about a full day but no time on my end.

Rich sent out a message [1] just today suggesting there is still
some time64 work to be done, so once he pushes those patches
I'll build and release new toolchains for 9.2, 8.3, and 7.5.


ZV

[1]: https://www.openwall.com/lists/musl/2019/12/19/6