linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v4 0/8] y2038 safety in v4l2
@ 2019-11-11 20:38 Arnd Bergmann
  2019-11-11 20:38 ` [PATCH v4 1/8] media: documentation: fix video_event description Arnd Bergmann
                   ` (8 more replies)
  0 siblings, 9 replies; 25+ messages in thread
From: Arnd Bergmann @ 2019-11-11 20:38 UTC (permalink / raw)
  To: Mauro Carvalho Chehab, linux-media
  Cc: Hans Verkuil, linux-kernel, y2038, Arnd Bergmann

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.

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. I updated the tree at [2]
to this version now.

      Arnd

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

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          | 188 +++++--
 drivers/media/v4l2-core/v4l2-subdev.c         |  20 +-
 drivers/media/v4l2-core/videobuf-core.c       |   4 +-
 include/linux/videodev2.h                     |  17 +-
 include/trace/events/v4l2.h                   |   2 +-
 include/uapi/linux/videodev2.h                |  77 +++
 15 files changed, 669 insertions(+), 136 deletions(-)

-- 
2.20.0

See below for the changes compared to v3:

|diff --git a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
|index a13e4849df0c..3bbf47d950e0 100644
|--- a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
|+++ b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
|@@ -500,7 +500,7 @@ struct v4l2_buffer32_time32 {
|        __u32                   bytesused;
|        __u32                   flags;
|        __u32                   field;  /* enum v4l2_field */
|-       struct compat_timeval   timestamp;
|+       struct old_timeval32    timestamp;
|        struct v4l2_timecode    timecode;
|        __u32                   sequence;
| 
|@@ -1290,7 +1290,7 @@ struct v4l2_event32_time32 {
|        } u;
|        __u32                           pending;
|        __u32                           sequence;
|-       struct compat_timespec          timestamp;
|+       struct old_timespec32           timestamp;
|        __u32                           id;
|        __u32                           reserved[8];
| };
|@@ -1482,8 +1482,8 @@ static long do_video_ioctl(struct file *file, unsigned int cmd, unsigned long ar
|        case VIDIOC_S_EXT_CTRLS32: ncmd = VIDIOC_S_EXT_CTRLS; break;
|        case VIDIOC_TRY_EXT_CTRLS32: ncmd = VIDIOC_TRY_EXT_CTRLS; break;
| #ifdef CONFIG_X86_64
|-       case VIDIOC_DQEVENT32: cmd = VIDIOC_DQEVENT; break;
|-       case VIDIOC_DQEVENT32_TIME32: cmd = VIDIOC_DQEVENT_TIME32; break;
|+       case VIDIOC_DQEVENT32: ncmd = VIDIOC_DQEVENT; break;
|+       case VIDIOC_DQEVENT32_TIME32: ncmd = VIDIOC_DQEVENT_TIME32; break;
| #endif
|        case VIDIOC_OVERLAY32: ncmd = VIDIOC_OVERLAY; break;
|        case VIDIOC_STREAMON32: ncmd = VIDIOC_STREAMON; break;
|diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
|index cd9a80960289..ad125cd4eb41 100644
|--- a/drivers/media/v4l2-core/v4l2-ioctl.c
|+++ b/drivers/media/v4l2-core/v4l2-ioctl.c
|@@ -474,7 +474,7 @@ static void v4l_print_buffer(const void *arg, bool write_only)
|        const struct v4l2_plane *plane;
|        int i;
| 
|-       pr_cont("%02ld:%02d:%02d.%09ld index=%d, type=%s, request_fd=%d, flags=0x%08x, field=%s, sequence=%d, memory=%s",
|+       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),
|@@ -821,7 +821,7 @@ static void v4l_print_event(const void *arg, bool write_only)
|        const struct v4l2_event *p = arg;
|        const struct v4l2_event_ctrl *c;
| 
|-       pr_cont("type=0x%x, pending=%u, sequence=%u, id=%u, timestamp=%lu.%9.9lu\n",
|+       pr_cont("type=0x%x, pending=%u, sequence=%u, id=%u, timestamp=%llu.%9.9llu\n",
|                        p->type, p->pending, p->sequence, p->id,
|                        p->timestamp.tv_sec, p->timestamp.tv_nsec);
|        switch (p->type) {
|diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h
|index 481ee3013b50..4086036e37d5 100644
|--- a/include/linux/videodev2.h
|+++ b/include/linux/videodev2.h
|@@ -62,7 +62,7 @@
| static inline u64 v4l2_buffer_get_timestamp(const struct v4l2_buffer *buf)
| {
|        return buf->timestamp.tv_sec * NSEC_PER_SEC +
|-              buf->timestamp.tv_usec * NSEC_PER_USEC;
|+              (u32)buf->timestamp.tv_usec * NSEC_PER_USEC;
| }
| 
| static inline void v4l2_buffer_set_timestamp(struct v4l2_buffer *buf,
|diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
|index 4275d6e92dae..ca10828328a5 100644
|--- a/include/uapi/linux/videodev2.h
|+++ b/include/uapi/linux/videodev2.h
|@@ -1001,7 +1001,12 @@ struct v4l2_buffer {
|        /* match glibc timeval64 format */
|        struct {
|                long long       tv_sec;
|+# if defined(__sparc__) && defined(__arch64__)
|+               int             tv_usec;
|+               int             __pad;
|+# else
|                long long       tv_usec;
|+# endif
|        } timestamp;
| #else
|        struct timeval          timestamp;
|

^ permalink raw reply	[flat|nested] 25+ messages in thread

* [PATCH v4 1/8] media: documentation: fix video_event description
  2019-11-11 20:38 [PATCH v4 0/8] y2038 safety in v4l2 Arnd Bergmann
@ 2019-11-11 20:38 ` Arnd Bergmann
  2019-11-11 20:38 ` [PATCH v4 2/8] media: v4l2: abstract timeval handling in v4l2_buffer Arnd Bergmann
                   ` (7 subsequent siblings)
  8 siblings, 0 replies; 25+ messages in thread
From: Arnd Bergmann @ 2019-11-11 20:38 UTC (permalink / raw)
  To: Mauro Carvalho Chehab, linux-media
  Cc: Hans Verkuil, linux-kernel, y2038, Arnd Bergmann

The type for the timestamp in video_event was changed to
'long' a long time ago, change the documentation to match.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 Documentation/media/uapi/dvb/video-get-event.rst | 2 +-
 Documentation/media/uapi/dvb/video_types.rst     | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/Documentation/media/uapi/dvb/video-get-event.rst b/Documentation/media/uapi/dvb/video-get-event.rst
index def6c40db601..7f03fbe3d3b0 100644
--- a/Documentation/media/uapi/dvb/video-get-event.rst
+++ b/Documentation/media/uapi/dvb/video-get-event.rst
@@ -81,7 +81,7 @@ for this ioctl call.
 	#define VIDEO_EVENT_FRAME_RATE_CHANGED	2
 	#define VIDEO_EVENT_DECODER_STOPPED 	3
 	#define VIDEO_EVENT_VSYNC 		4
-		__kernel_time_t timestamp;
+		long timestamp;
 		union {
 			video_size_t size;
 			unsigned int frame_rate;	/* in frames per 1000sec */
diff --git a/Documentation/media/uapi/dvb/video_types.rst b/Documentation/media/uapi/dvb/video_types.rst
index 479942ce6fb8..2697400ccf62 100644
--- a/Documentation/media/uapi/dvb/video_types.rst
+++ b/Documentation/media/uapi/dvb/video_types.rst
@@ -170,7 +170,7 @@ VIDEO_GET_EVENT call.
     #define VIDEO_EVENT_FRAME_RATE_CHANGED  2
     #define VIDEO_EVENT_DECODER_STOPPED     3
     #define VIDEO_EVENT_VSYNC       4
-	__kernel_time_t timestamp;
+	long timestamp;
 	union {
 	    video_size_t size;
 	    unsigned int frame_rate;    /* in frames per 1000sec */
-- 
2.20.0


^ permalink raw reply related	[flat|nested] 25+ messages in thread

* [PATCH v4 2/8] media: v4l2: abstract timeval handling in v4l2_buffer
  2019-11-11 20:38 [PATCH v4 0/8] y2038 safety in v4l2 Arnd Bergmann
  2019-11-11 20:38 ` [PATCH v4 1/8] media: documentation: fix video_event description Arnd Bergmann
@ 2019-11-11 20:38 ` Arnd Bergmann
  2019-11-25 15:52   ` Hans Verkuil
  2019-11-11 20:38 ` [PATCH v4 3/8] media: v4l2-core: compat: ignore native command codes Arnd Bergmann
                   ` (6 subsequent siblings)
  8 siblings, 1 reply; 25+ messages in thread
From: Arnd Bergmann @ 2019-11-11 20:38 UTC (permalink / raw)
  To: Mauro Carvalho Chehab, linux-media
  Cc: Hans Verkuil, linux-kernel, y2038, Arnd Bergmann

As a preparation for adding 64-bit time_t support in the uapi,
change the drivers to no longer care about the format of the
timestamp field in struct v4l2_buffer.

The v4l2_timeval_to_ns() function is no longer needed in the
kernel after this, but there may be userspace code relying on
it because it is part of the uapi header.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/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/videobuf-core.c         |  4 ++--
 include/linux/videodev2.h                       | 17 ++++++++++++++++-
 include/trace/events/v4l2.h                     |  2 +-
 include/uapi/linux/videodev2.h                  |  2 ++
 9 files changed, 30 insertions(+), 13 deletions(-)

diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c
index 5a9ba3846f0a..9ec710878db6 100644
--- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
+++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
@@ -143,7 +143,7 @@ static void __copy_timestamp(struct vb2_buffer *vb, const void *pb)
 		 * and the timecode field and flag if needed.
 		 */
 		if (q->copy_timestamp)
-			vb->timestamp = v4l2_timeval_to_ns(&b->timestamp);
+			vb->timestamp = v4l2_buffer_get_timestamp(b);
 		vbuf->flags |= b->flags & V4L2_BUF_FLAG_TIMECODE;
 		if (b->flags & V4L2_BUF_FLAG_TIMECODE)
 			vbuf->timecode = b->timecode;
@@ -476,7 +476,7 @@ static void __fill_v4l2_buffer(struct vb2_buffer *vb, void *pb)
 
 	b->flags = vbuf->flags;
 	b->field = vbuf->field;
-	b->timestamp = ns_to_timeval(vb->timestamp);
+	v4l2_buffer_set_timestamp(b, vb->timestamp);
 	b->timecode = vbuf->timecode;
 	b->sequence = vbuf->sequence;
 	b->reserved2 = 0;
diff --git a/drivers/media/pci/meye/meye.c b/drivers/media/pci/meye/meye.c
index 0e61c81356ef..3a4c29bc0ba5 100644
--- a/drivers/media/pci/meye/meye.c
+++ b/drivers/media/pci/meye/meye.c
@@ -1266,7 +1266,7 @@ static int vidioc_querybuf(struct file *file, void *fh, struct v4l2_buffer *buf)
 		buf->flags |= V4L2_BUF_FLAG_DONE;
 
 	buf->field = V4L2_FIELD_NONE;
-	buf->timestamp = ns_to_timeval(meye.grab_buffer[index].ts);
+	v4l2_buffer_set_timestamp(buf, meye.grab_buffer[index].ts);
 	buf->sequence = meye.grab_buffer[index].sequence;
 	buf->memory = V4L2_MEMORY_MMAP;
 	buf->m.offset = index * gbufsize;
@@ -1332,7 +1332,7 @@ static int vidioc_dqbuf(struct file *file, void *fh, struct v4l2_buffer *buf)
 	buf->bytesused = meye.grab_buffer[reqnr].size;
 	buf->flags = V4L2_BUF_FLAG_MAPPED | V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
 	buf->field = V4L2_FIELD_NONE;
-	buf->timestamp = ns_to_timeval(meye.grab_buffer[reqnr].ts);
+	v4l2_buffer_set_timestamp(buf, meye.grab_buffer[reqnr].ts);
 	buf->sequence = meye.grab_buffer[reqnr].sequence;
 	buf->memory = V4L2_MEMORY_MMAP;
 	buf->m.offset = reqnr * gbufsize;
diff --git a/drivers/media/usb/cpia2/cpia2_v4l.c b/drivers/media/usb/cpia2/cpia2_v4l.c
index 626264a56517..9d3d05125d7b 100644
--- a/drivers/media/usb/cpia2/cpia2_v4l.c
+++ b/drivers/media/usb/cpia2/cpia2_v4l.c
@@ -800,7 +800,7 @@ static int cpia2_querybuf(struct file *file, void *fh, struct v4l2_buffer *buf)
 		break;
 	case FRAME_READY:
 		buf->bytesused = cam->buffers[buf->index].length;
-		buf->timestamp = ns_to_timeval(cam->buffers[buf->index].ts);
+		v4l2_buffer_set_timestamp(buf, cam->buffers[buf->index].ts);
 		buf->sequence = cam->buffers[buf->index].seq;
 		buf->flags = V4L2_BUF_FLAG_DONE;
 		break;
@@ -907,7 +907,7 @@ static int cpia2_dqbuf(struct file *file, void *fh, struct v4l2_buffer *buf)
 	buf->flags = V4L2_BUF_FLAG_MAPPED | V4L2_BUF_FLAG_DONE
 		| V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
 	buf->field = V4L2_FIELD_NONE;
-	buf->timestamp = ns_to_timeval(cam->buffers[buf->index].ts);
+	v4l2_buffer_set_timestamp(buf, cam->buffers[buf->index].ts);
 	buf->sequence = cam->buffers[buf->index].seq;
 	buf->m.offset = cam->buffers[buf->index].data - cam->frame_buffer;
 	buf->length = cam->frame_size;
diff --git a/drivers/media/usb/stkwebcam/stk-webcam.c b/drivers/media/usb/stkwebcam/stk-webcam.c
index 21f90a887485..b22501f76b78 100644
--- a/drivers/media/usb/stkwebcam/stk-webcam.c
+++ b/drivers/media/usb/stkwebcam/stk-webcam.c
@@ -1125,7 +1125,7 @@ static int stk_vidioc_dqbuf(struct file *filp,
 	sbuf->v4lbuf.flags &= ~V4L2_BUF_FLAG_QUEUED;
 	sbuf->v4lbuf.flags |= V4L2_BUF_FLAG_DONE;
 	sbuf->v4lbuf.sequence = ++dev->sequence;
-	sbuf->v4lbuf.timestamp = ns_to_timeval(ktime_get_ns());
+	v4l2_buffer_set_timestamp(&sbuf->v4lbuf, ktime_get_ns());
 
 	*buf = sbuf->v4lbuf;
 	return 0;
diff --git a/drivers/media/usb/usbvision/usbvision-video.c b/drivers/media/usb/usbvision/usbvision-video.c
index cdc66adda755..15a423c5deb7 100644
--- a/drivers/media/usb/usbvision/usbvision-video.c
+++ b/drivers/media/usb/usbvision/usbvision-video.c
@@ -687,7 +687,7 @@ static int vidioc_querybuf(struct file *file,
 	vb->length = usbvision->curwidth *
 		usbvision->curheight *
 		usbvision->palette.bytes_per_pixel;
-	vb->timestamp = ns_to_timeval(usbvision->frame[vb->index].ts);
+	v4l2_buffer_set_timestamp(vb, usbvision->frame[vb->index].ts);
 	vb->sequence = usbvision->frame[vb->index].sequence;
 	return 0;
 }
@@ -756,7 +756,7 @@ static int vidioc_dqbuf(struct file *file, void *priv, struct v4l2_buffer *vb)
 		V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
 	vb->index = f->index;
 	vb->sequence = f->sequence;
-	vb->timestamp = ns_to_timeval(f->ts);
+	v4l2_buffer_set_timestamp(vb, f->ts);
 	vb->field = V4L2_FIELD_NONE;
 	vb->bytesused = f->scanlength;
 
diff --git a/drivers/media/v4l2-core/videobuf-core.c b/drivers/media/v4l2-core/videobuf-core.c
index 939fc11cf080..ab650371c151 100644
--- a/drivers/media/v4l2-core/videobuf-core.c
+++ b/drivers/media/v4l2-core/videobuf-core.c
@@ -364,7 +364,7 @@ static void videobuf_status(struct videobuf_queue *q, struct v4l2_buffer *b,
 	}
 
 	b->field     = vb->field;
-	b->timestamp = ns_to_timeval(vb->ts);
+	v4l2_buffer_set_timestamp(b, vb->ts);
 	b->bytesused = vb->size;
 	b->sequence  = vb->field_count >> 1;
 }
@@ -578,7 +578,7 @@ int videobuf_qbuf(struct videobuf_queue *q, struct v4l2_buffer *b)
 		    || q->type == V4L2_BUF_TYPE_SDR_OUTPUT) {
 			buf->size = b->bytesused;
 			buf->field = b->field;
-			buf->ts = v4l2_timeval_to_ns(&b->timestamp);
+			buf->ts = v4l2_buffer_get_timestamp(b);
 		}
 		break;
 	case V4L2_MEMORY_USERPTR:
diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h
index 16c0ed6c50a7..4086036e37d5 100644
--- a/include/linux/videodev2.h
+++ b/include/linux/videodev2.h
@@ -56,7 +56,22 @@
 #ifndef __LINUX_VIDEODEV2_H
 #define __LINUX_VIDEODEV2_H
 
-#include <linux/time.h>     /* need struct timeval */
+#include <linux/time.h>
 #include <uapi/linux/videodev2.h>
 
+static inline u64 v4l2_buffer_get_timestamp(const struct v4l2_buffer *buf)
+{
+	return buf->timestamp.tv_sec * NSEC_PER_SEC +
+	       (u32)buf->timestamp.tv_usec * NSEC_PER_USEC;
+}
+
+static inline void v4l2_buffer_set_timestamp(struct v4l2_buffer *buf,
+					     u64 timestamp)
+{
+	struct timespec64 ts = ns_to_timespec64(timestamp);
+
+	buf->timestamp.tv_sec  = ts.tv_sec;
+	buf->timestamp.tv_usec = ts.tv_nsec / NSEC_PER_USEC;
+}
+
 #endif /* __LINUX_VIDEODEV2_H */
diff --git a/include/trace/events/v4l2.h b/include/trace/events/v4l2.h
index 83860de120e3..248bc09bfc99 100644
--- a/include/trace/events/v4l2.h
+++ b/include/trace/events/v4l2.h
@@ -130,7 +130,7 @@ DECLARE_EVENT_CLASS(v4l2_event_class,
 		__entry->bytesused = buf->bytesused;
 		__entry->flags = buf->flags;
 		__entry->field = buf->field;
-		__entry->timestamp = timeval_to_ns(&buf->timestamp);
+		__entry->timestamp = v4l2_buffer_get_timestamp(buf);
 		__entry->timecode_type = buf->timecode.type;
 		__entry->timecode_flags = buf->timecode.flags;
 		__entry->timecode_frames = buf->timecode.frames;
diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
index 530638dffd93..74d3d522f3db 100644
--- a/include/uapi/linux/videodev2.h
+++ b/include/uapi/linux/videodev2.h
@@ -1010,6 +1010,7 @@ struct v4l2_buffer {
 	};
 };
 
+#ifndef __KERNEL__
 /**
  * v4l2_timeval_to_ns - Convert timeval to nanoseconds
  * @ts:		pointer to the timeval variable to be converted
@@ -1021,6 +1022,7 @@ static inline __u64 v4l2_timeval_to_ns(const struct timeval *tv)
 {
 	return (__u64)tv->tv_sec * 1000000000ULL + tv->tv_usec * 1000;
 }
+#endif
 
 /*  Flags for 'flags' field */
 /* Buffer is mapped (flag) */
-- 
2.20.0


^ permalink raw reply related	[flat|nested] 25+ messages in thread

* [PATCH v4 3/8] media: v4l2-core: compat: ignore native command codes
  2019-11-11 20:38 [PATCH v4 0/8] y2038 safety in v4l2 Arnd Bergmann
  2019-11-11 20:38 ` [PATCH v4 1/8] media: documentation: fix video_event description Arnd Bergmann
  2019-11-11 20:38 ` [PATCH v4 2/8] media: v4l2: abstract timeval handling in v4l2_buffer Arnd Bergmann
@ 2019-11-11 20:38 ` Arnd Bergmann
       [not found]   ` <20191122070015.D5A702068E@mail.kernel.org>
  2019-11-11 20:38 ` [PATCH v4 4/8] media: v4l2-core: split out data copy from video_usercopy Arnd Bergmann
                   ` (5 subsequent siblings)
  8 siblings, 1 reply; 25+ messages in thread
From: Arnd Bergmann @ 2019-11-11 20:38 UTC (permalink / raw)
  To: Mauro Carvalho Chehab, linux-media
  Cc: Hans Verkuil, linux-kernel, y2038, Arnd Bergmann, stable

The do_video_ioctl() compat handler converts the compat command
codes into the native ones before processing further, but this
causes problems for 32-bit user applications that pass a command
code that matches a 64-bit native number, which will then be
handled the same way.

Specifically, this breaks VIDIOC_DQEVENT_TIME from user space
applications with 64-bit time_t, as the structure layout is
the same as the native 64-bit layout on many architectures
(x86 being the notable exception).

Change the handler to use the converted command code only for
passing into the native ioctl handler, not for deciding on the
conversion, in order to make the compat behavior match the
native behavior.

Actual support for the 64-bit time_t version of VIDIOC_DQEVENT_TIME
and other commands still needs to be added in a separate patch.

Cc: stable@vger.kernel.org
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/media/v4l2-core/v4l2-compat-ioctl32.c | 148 +++++++++---------
 1 file changed, 75 insertions(+), 73 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
index e1eaf1135c7f..7ad6db8dd9f6 100644
--- a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
+++ b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
@@ -1183,36 +1183,38 @@ static long do_video_ioctl(struct file *file, unsigned int cmd, unsigned long ar
 	u32 aux_space;
 	int compatible_arg = 1;
 	long err = 0;
+	unsigned int ncmd;
 
 	/*
 	 * 1. When struct size is different, converts the command.
 	 */
 	switch (cmd) {
-	case VIDIOC_G_FMT32: cmd = VIDIOC_G_FMT; break;
-	case VIDIOC_S_FMT32: cmd = VIDIOC_S_FMT; break;
-	case VIDIOC_QUERYBUF32: cmd = VIDIOC_QUERYBUF; break;
-	case VIDIOC_G_FBUF32: cmd = VIDIOC_G_FBUF; break;
-	case VIDIOC_S_FBUF32: cmd = VIDIOC_S_FBUF; break;
-	case VIDIOC_QBUF32: cmd = VIDIOC_QBUF; break;
-	case VIDIOC_DQBUF32: cmd = VIDIOC_DQBUF; break;
-	case VIDIOC_ENUMSTD32: cmd = VIDIOC_ENUMSTD; break;
-	case VIDIOC_ENUMINPUT32: cmd = VIDIOC_ENUMINPUT; break;
-	case VIDIOC_TRY_FMT32: cmd = VIDIOC_TRY_FMT; break;
-	case VIDIOC_G_EXT_CTRLS32: cmd = VIDIOC_G_EXT_CTRLS; break;
-	case VIDIOC_S_EXT_CTRLS32: cmd = VIDIOC_S_EXT_CTRLS; break;
-	case VIDIOC_TRY_EXT_CTRLS32: cmd = VIDIOC_TRY_EXT_CTRLS; break;
-	case VIDIOC_DQEVENT32: cmd = VIDIOC_DQEVENT; break;
-	case VIDIOC_OVERLAY32: cmd = VIDIOC_OVERLAY; break;
-	case VIDIOC_STREAMON32: cmd = VIDIOC_STREAMON; break;
-	case VIDIOC_STREAMOFF32: cmd = VIDIOC_STREAMOFF; break;
-	case VIDIOC_G_INPUT32: cmd = VIDIOC_G_INPUT; break;
-	case VIDIOC_S_INPUT32: cmd = VIDIOC_S_INPUT; break;
-	case VIDIOC_G_OUTPUT32: cmd = VIDIOC_G_OUTPUT; break;
-	case VIDIOC_S_OUTPUT32: cmd = VIDIOC_S_OUTPUT; break;
-	case VIDIOC_CREATE_BUFS32: cmd = VIDIOC_CREATE_BUFS; break;
-	case VIDIOC_PREPARE_BUF32: cmd = VIDIOC_PREPARE_BUF; break;
-	case VIDIOC_G_EDID32: cmd = VIDIOC_G_EDID; break;
-	case VIDIOC_S_EDID32: cmd = VIDIOC_S_EDID; break;
+	case VIDIOC_G_FMT32: ncmd = VIDIOC_G_FMT; break;
+	case VIDIOC_S_FMT32: ncmd = VIDIOC_S_FMT; break;
+	case VIDIOC_QUERYBUF32: ncmd = VIDIOC_QUERYBUF; break;
+	case VIDIOC_G_FBUF32: ncmd = VIDIOC_G_FBUF; break;
+	case VIDIOC_S_FBUF32: ncmd = VIDIOC_S_FBUF; break;
+	case VIDIOC_QBUF32: ncmd = VIDIOC_QBUF; break;
+	case VIDIOC_DQBUF32: ncmd = VIDIOC_DQBUF; break;
+	case VIDIOC_ENUMSTD32: ncmd = VIDIOC_ENUMSTD; break;
+	case VIDIOC_ENUMINPUT32: ncmd = VIDIOC_ENUMINPUT; break;
+	case VIDIOC_TRY_FMT32: ncmd = VIDIOC_TRY_FMT; break;
+	case VIDIOC_G_EXT_CTRLS32: ncmd = VIDIOC_G_EXT_CTRLS; break;
+	case VIDIOC_S_EXT_CTRLS32: ncmd = VIDIOC_S_EXT_CTRLS; break;
+	case VIDIOC_TRY_EXT_CTRLS32: ncmd = VIDIOC_TRY_EXT_CTRLS; break;
+	case VIDIOC_DQEVENT32: ncmd = VIDIOC_DQEVENT; break;
+	case VIDIOC_OVERLAY32: ncmd = VIDIOC_OVERLAY; break;
+	case VIDIOC_STREAMON32: ncmd = VIDIOC_STREAMON; break;
+	case VIDIOC_STREAMOFF32: ncmd = VIDIOC_STREAMOFF; break;
+	case VIDIOC_G_INPUT32: ncmd = VIDIOC_G_INPUT; break;
+	case VIDIOC_S_INPUT32: ncmd = VIDIOC_S_INPUT; break;
+	case VIDIOC_G_OUTPUT32: ncmd = VIDIOC_G_OUTPUT; break;
+	case VIDIOC_S_OUTPUT32: ncmd = VIDIOC_S_OUTPUT; break;
+	case VIDIOC_CREATE_BUFS32: ncmd = VIDIOC_CREATE_BUFS; break;
+	case VIDIOC_PREPARE_BUF32: ncmd = VIDIOC_PREPARE_BUF; break;
+	case VIDIOC_G_EDID32: ncmd = VIDIOC_G_EDID; break;
+	case VIDIOC_S_EDID32: ncmd = VIDIOC_S_EDID; break;
+	default: ncmd = cmd; break;
 	}
 
 	/*
@@ -1221,11 +1223,11 @@ static long do_video_ioctl(struct file *file, unsigned int cmd, unsigned long ar
 	 * argument into it.
 	 */
 	switch (cmd) {
-	case VIDIOC_OVERLAY:
-	case VIDIOC_STREAMON:
-	case VIDIOC_STREAMOFF:
-	case VIDIOC_S_INPUT:
-	case VIDIOC_S_OUTPUT:
+	case VIDIOC_OVERLAY32:
+	case VIDIOC_STREAMON32:
+	case VIDIOC_STREAMOFF32:
+	case VIDIOC_S_INPUT32:
+	case VIDIOC_S_OUTPUT32:
 		err = alloc_userspace(sizeof(unsigned int), 0, &new_p64);
 		if (!err && assign_in_user((unsigned int __user *)new_p64,
 					   (compat_uint_t __user *)p32))
@@ -1233,23 +1235,23 @@ static long do_video_ioctl(struct file *file, unsigned int cmd, unsigned long ar
 		compatible_arg = 0;
 		break;
 
-	case VIDIOC_G_INPUT:
-	case VIDIOC_G_OUTPUT:
+	case VIDIOC_G_INPUT32:
+	case VIDIOC_G_OUTPUT32:
 		err = alloc_userspace(sizeof(unsigned int), 0, &new_p64);
 		compatible_arg = 0;
 		break;
 
-	case VIDIOC_G_EDID:
-	case VIDIOC_S_EDID:
+	case VIDIOC_G_EDID32:
+	case VIDIOC_S_EDID32:
 		err = alloc_userspace(sizeof(struct v4l2_edid), 0, &new_p64);
 		if (!err)
 			err = get_v4l2_edid32(new_p64, p32);
 		compatible_arg = 0;
 		break;
 
-	case VIDIOC_G_FMT:
-	case VIDIOC_S_FMT:
-	case VIDIOC_TRY_FMT:
+	case VIDIOC_G_FMT32:
+	case VIDIOC_S_FMT32:
+	case VIDIOC_TRY_FMT32:
 		err = bufsize_v4l2_format(p32, &aux_space);
 		if (!err)
 			err = alloc_userspace(sizeof(struct v4l2_format),
@@ -1262,7 +1264,7 @@ static long do_video_ioctl(struct file *file, unsigned int cmd, unsigned long ar
 		compatible_arg = 0;
 		break;
 
-	case VIDIOC_CREATE_BUFS:
+	case VIDIOC_CREATE_BUFS32:
 		err = bufsize_v4l2_create(p32, &aux_space);
 		if (!err)
 			err = alloc_userspace(sizeof(struct v4l2_create_buffers),
@@ -1275,10 +1277,10 @@ static long do_video_ioctl(struct file *file, unsigned int cmd, unsigned long ar
 		compatible_arg = 0;
 		break;
 
-	case VIDIOC_PREPARE_BUF:
-	case VIDIOC_QUERYBUF:
-	case VIDIOC_QBUF:
-	case VIDIOC_DQBUF:
+	case VIDIOC_PREPARE_BUF32:
+	case VIDIOC_QUERYBUF32:
+	case VIDIOC_QBUF32:
+	case VIDIOC_DQBUF32:
 		err = bufsize_v4l2_buffer(p32, &aux_space);
 		if (!err)
 			err = alloc_userspace(sizeof(struct v4l2_buffer),
@@ -1291,7 +1293,7 @@ static long do_video_ioctl(struct file *file, unsigned int cmd, unsigned long ar
 		compatible_arg = 0;
 		break;
 
-	case VIDIOC_S_FBUF:
+	case VIDIOC_S_FBUF32:
 		err = alloc_userspace(sizeof(struct v4l2_framebuffer), 0,
 				      &new_p64);
 		if (!err)
@@ -1299,13 +1301,13 @@ static long do_video_ioctl(struct file *file, unsigned int cmd, unsigned long ar
 		compatible_arg = 0;
 		break;
 
-	case VIDIOC_G_FBUF:
+	case VIDIOC_G_FBUF32:
 		err = alloc_userspace(sizeof(struct v4l2_framebuffer), 0,
 				      &new_p64);
 		compatible_arg = 0;
 		break;
 
-	case VIDIOC_ENUMSTD:
+	case VIDIOC_ENUMSTD32:
 		err = alloc_userspace(sizeof(struct v4l2_standard), 0,
 				      &new_p64);
 		if (!err)
@@ -1313,16 +1315,16 @@ static long do_video_ioctl(struct file *file, unsigned int cmd, unsigned long ar
 		compatible_arg = 0;
 		break;
 
-	case VIDIOC_ENUMINPUT:
+	case VIDIOC_ENUMINPUT32:
 		err = alloc_userspace(sizeof(struct v4l2_input), 0, &new_p64);
 		if (!err)
 			err = get_v4l2_input32(new_p64, p32);
 		compatible_arg = 0;
 		break;
 
-	case VIDIOC_G_EXT_CTRLS:
-	case VIDIOC_S_EXT_CTRLS:
-	case VIDIOC_TRY_EXT_CTRLS:
+	case VIDIOC_G_EXT_CTRLS32:
+	case VIDIOC_S_EXT_CTRLS32:
+	case VIDIOC_TRY_EXT_CTRLS32:
 		err = bufsize_v4l2_ext_controls(p32, &aux_space);
 		if (!err)
 			err = alloc_userspace(sizeof(struct v4l2_ext_controls),
@@ -1334,7 +1336,7 @@ static long do_video_ioctl(struct file *file, unsigned int cmd, unsigned long ar
 		}
 		compatible_arg = 0;
 		break;
-	case VIDIOC_DQEVENT:
+	case VIDIOC_DQEVENT32:
 		err = alloc_userspace(sizeof(struct v4l2_event), 0, &new_p64);
 		compatible_arg = 0;
 		break;
@@ -1352,9 +1354,9 @@ static long do_video_ioctl(struct file *file, unsigned int cmd, unsigned long ar
 	 * Otherwise, it will pass the newly allocated @new_p64 argument.
 	 */
 	if (compatible_arg)
-		err = native_ioctl(file, cmd, (unsigned long)p32);
+		err = native_ioctl(file, ncmd, (unsigned long)p32);
 	else
-		err = native_ioctl(file, cmd, (unsigned long)new_p64);
+		err = native_ioctl(file, ncmd, (unsigned long)new_p64);
 
 	if (err == -ENOTTY)
 		return err;
@@ -1370,13 +1372,13 @@ static long do_video_ioctl(struct file *file, unsigned int cmd, unsigned long ar
 	 * the blocks to maximum allowed value.
 	 */
 	switch (cmd) {
-	case VIDIOC_G_EXT_CTRLS:
-	case VIDIOC_S_EXT_CTRLS:
-	case VIDIOC_TRY_EXT_CTRLS:
+	case VIDIOC_G_EXT_CTRLS32:
+	case VIDIOC_S_EXT_CTRLS32:
+	case VIDIOC_TRY_EXT_CTRLS32:
 		if (put_v4l2_ext_controls32(file, new_p64, p32))
 			err = -EFAULT;
 		break;
-	case VIDIOC_S_EDID:
+	case VIDIOC_S_EDID32:
 		if (put_v4l2_edid32(new_p64, p32))
 			err = -EFAULT;
 		break;
@@ -1389,49 +1391,49 @@ static long do_video_ioctl(struct file *file, unsigned int cmd, unsigned long ar
 	 * the original 32 bits structure.
 	 */
 	switch (cmd) {
-	case VIDIOC_S_INPUT:
-	case VIDIOC_S_OUTPUT:
-	case VIDIOC_G_INPUT:
-	case VIDIOC_G_OUTPUT:
+	case VIDIOC_S_INPUT32:
+	case VIDIOC_S_OUTPUT32:
+	case VIDIOC_G_INPUT32:
+	case VIDIOC_G_OUTPUT32:
 		if (assign_in_user((compat_uint_t __user *)p32,
 				   ((unsigned int __user *)new_p64)))
 			err = -EFAULT;
 		break;
 
-	case VIDIOC_G_FBUF:
+	case VIDIOC_G_FBUF32:
 		err = put_v4l2_framebuffer32(new_p64, p32);
 		break;
 
-	case VIDIOC_DQEVENT:
+	case VIDIOC_DQEVENT32:
 		err = put_v4l2_event32(new_p64, p32);
 		break;
 
-	case VIDIOC_G_EDID:
+	case VIDIOC_G_EDID32:
 		err = put_v4l2_edid32(new_p64, p32);
 		break;
 
-	case VIDIOC_G_FMT:
-	case VIDIOC_S_FMT:
-	case VIDIOC_TRY_FMT:
+	case VIDIOC_G_FMT32:
+	case VIDIOC_S_FMT32:
+	case VIDIOC_TRY_FMT32:
 		err = put_v4l2_format32(new_p64, p32);
 		break;
 
-	case VIDIOC_CREATE_BUFS:
+	case VIDIOC_CREATE_BUFS32:
 		err = put_v4l2_create32(new_p64, p32);
 		break;
 
-	case VIDIOC_PREPARE_BUF:
-	case VIDIOC_QUERYBUF:
-	case VIDIOC_QBUF:
-	case VIDIOC_DQBUF:
+	case VIDIOC_PREPARE_BUF32:
+	case VIDIOC_QUERYBUF32:
+	case VIDIOC_QBUF32:
+	case VIDIOC_DQBUF32:
 		err = put_v4l2_buffer32(new_p64, p32);
 		break;
 
-	case VIDIOC_ENUMSTD:
+	case VIDIOC_ENUMSTD32:
 		err = put_v4l2_standard32(new_p64, p32);
 		break;
 
-	case VIDIOC_ENUMINPUT:
+	case VIDIOC_ENUMINPUT32:
 		err = put_v4l2_input32(new_p64, p32);
 		break;
 	}
-- 
2.20.0


^ permalink raw reply related	[flat|nested] 25+ messages in thread

* [PATCH v4 4/8] media: v4l2-core: split out data copy from video_usercopy
  2019-11-11 20:38 [PATCH v4 0/8] y2038 safety in v4l2 Arnd Bergmann
                   ` (2 preceding siblings ...)
  2019-11-11 20:38 ` [PATCH v4 3/8] media: v4l2-core: compat: ignore native command codes Arnd Bergmann
@ 2019-11-11 20:38 ` Arnd Bergmann
  2019-11-11 20:38 ` [PATCH v4 5/8] media: v4l2-core: fix VIDIOC_DQEVENT for time64 ABI Arnd Bergmann
                   ` (4 subsequent siblings)
  8 siblings, 0 replies; 25+ messages in thread
From: Arnd Bergmann @ 2019-11-11 20:38 UTC (permalink / raw)
  To: Mauro Carvalho Chehab, linux-media
  Cc: Hans Verkuil, linux-kernel, y2038, Arnd Bergmann

The copy-in/out portions of video_usercopy() are about to
get more complex, so turn then into separate functions as
a cleanup first.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/media/v4l2-core/v4l2-ioctl.c | 107 +++++++++++++++++----------
 1 file changed, 68 insertions(+), 39 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
index 51b912743f0f..693f9eb8e01b 100644
--- a/drivers/media/v4l2-core/v4l2-ioctl.c
+++ b/drivers/media/v4l2-core/v4l2-ioctl.c
@@ -3008,8 +3008,69 @@ static int check_array_args(unsigned int cmd, void *parg, size_t *array_size,
 	return ret;
 }
 
+static unsigned int video_translate_cmd(unsigned int cmd)
+{
+	return cmd;
+}
+
+static int video_get_user(void __user *arg, void *parg, unsigned int cmd,
+			  bool *always_copy)
+{
+	unsigned int n = _IOC_SIZE(cmd);
+
+	if (!(_IOC_DIR(cmd) & _IOC_WRITE)) {
+		/* read-only ioctl */
+		memset(parg, 0, n);
+		return 0;
+	}
+
+	switch (cmd) {
+	default:
+		/*
+		 * In some cases, only a few fields are used as input,
+		 * i.e. when the app sets "index" and then the driver
+		 * fills in the rest of the structure for the thing
+		 * with that index.  We only need to copy up the first
+		 * non-input field.
+		 */
+		if (v4l2_is_known_ioctl(cmd)) {
+			u32 flags = v4l2_ioctls[_IOC_NR(cmd)].flags;
+
+			if (flags & INFO_FL_CLEAR_MASK)
+				n = (flags & INFO_FL_CLEAR_MASK) >> 16;
+			*always_copy = flags & INFO_FL_ALWAYS_COPY;
+		}
+
+		if (copy_from_user(parg, (void __user *)arg, n))
+			return -EFAULT;
+
+		/* zero out anything we don't copy from userspace */
+		if (n < _IOC_SIZE(cmd))
+			memset((u8 *)parg + n, 0, _IOC_SIZE(cmd) - n);
+		break;
+	}
+
+	return 0;
+}
+
+static int video_put_user(void __user *arg, void *parg, unsigned int cmd)
+{
+	if (!(_IOC_DIR(cmd) & _IOC_READ))
+		return 0;
+
+	switch (cmd) {
+	default:
+		/*  Copy results into user buffer  */
+		if (copy_to_user(arg, parg, _IOC_SIZE(cmd)))
+			return -EFAULT;
+		break;
+	}
+
+	return 0;
+}
+
 long
-video_usercopy(struct file *file, unsigned int cmd, unsigned long arg,
+video_usercopy(struct file *file, unsigned int orig_cmd, unsigned long arg,
 	       v4l2_kioctl func)
 {
 	char	sbuf[128];
@@ -3021,6 +3082,7 @@ video_usercopy(struct file *file, unsigned int cmd, unsigned long arg,
 	size_t  array_size = 0;
 	void __user *user_ptr = NULL;
 	void	**kernel_ptr = NULL;
+	unsigned int cmd = video_translate_cmd(orig_cmd);
 	const size_t ioc_size = _IOC_SIZE(cmd);
 
 	/*  Copy arguments into temp kernel buffer  */
@@ -3035,37 +3097,12 @@ video_usercopy(struct file *file, unsigned int cmd, unsigned long arg,
 			parg = mbuf;
 		}
 
-		err = -EFAULT;
-		if (_IOC_DIR(cmd) & _IOC_WRITE) {
-			unsigned int n = ioc_size;
-
-			/*
-			 * In some cases, only a few fields are used as input,
-			 * i.e. when the app sets "index" and then the driver
-			 * fills in the rest of the structure for the thing
-			 * with that index.  We only need to copy up the first
-			 * non-input field.
-			 */
-			if (v4l2_is_known_ioctl(cmd)) {
-				u32 flags = v4l2_ioctls[_IOC_NR(cmd)].flags;
-
-				if (flags & INFO_FL_CLEAR_MASK)
-					n = (flags & INFO_FL_CLEAR_MASK) >> 16;
-				always_copy = flags & INFO_FL_ALWAYS_COPY;
-			}
-
-			if (copy_from_user(parg, (void __user *)arg, n))
-				goto out;
-
-			/* zero out anything we don't copy from userspace */
-			if (n < ioc_size)
-				memset((u8 *)parg + n, 0, ioc_size - n);
-		} else {
-			/* read-only ioctl */
-			memset(parg, 0, ioc_size);
-		}
 	}
 
+	err = video_get_user((void __user *)arg, parg, orig_cmd, &always_copy);
+	if (err)
+		goto out;
+
 	err = check_array_args(cmd, parg, &array_size, &user_ptr, &kernel_ptr);
 	if (err < 0)
 		goto out;
@@ -3116,15 +3153,7 @@ video_usercopy(struct file *file, unsigned int cmd, unsigned long arg,
 		goto out;
 
 out_array_args:
-	/*  Copy results into user buffer  */
-	switch (_IOC_DIR(cmd)) {
-	case _IOC_READ:
-	case (_IOC_WRITE | _IOC_READ):
-		if (copy_to_user((void __user *)arg, parg, ioc_size))
-			err = -EFAULT;
-		break;
-	}
-
+	err = video_put_user((void __user *)arg, parg, orig_cmd);
 out:
 	kvfree(mbuf);
 	return err;
-- 
2.20.0


^ permalink raw reply related	[flat|nested] 25+ messages in thread

* [PATCH v4 5/8] media: v4l2-core: fix VIDIOC_DQEVENT for time64 ABI
  2019-11-11 20:38 [PATCH v4 0/8] y2038 safety in v4l2 Arnd Bergmann
                   ` (3 preceding siblings ...)
  2019-11-11 20:38 ` [PATCH v4 4/8] media: v4l2-core: split out data copy from video_usercopy Arnd Bergmann
@ 2019-11-11 20:38 ` Arnd Bergmann
  2019-11-25 14:40   ` Hans Verkuil
  2019-11-11 20:38 ` [PATCH v4 6/8] media: v4l2-core: fix v4l2_buffer handling " Arnd Bergmann
                   ` (3 subsequent siblings)
  8 siblings, 1 reply; 25+ messages in thread
From: Arnd Bergmann @ 2019-11-11 20:38 UTC (permalink / raw)
  To: Mauro Carvalho Chehab, linux-media
  Cc: Hans Verkuil, linux-kernel, y2038, Arnd Bergmann

The v4l2_event structure contains a 'struct timespec' member that is
defined by the user space C library, creating an ABI incompatibility
when that gets updated to a 64-bit time_t.

While passing a 32-bit time_t here would be sufficient for CLOCK_MONOTONIC
timestamps, simply redefining the structure to use the kernel's
__kernel_old_timespec would not work for any library that uses a copy
of the linux/videodev2.h header file rather than including the copy from
the latest kernel headers.

This means the kernel has to be changed to handle both versions of the
structure layout on a 32-bit architecture. The easiest way to do this
is during the copy from/to user space.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/media/v4l2-core/v4l2-event.c  |  5 ++++-
 drivers/media/v4l2-core/v4l2-ioctl.c  | 24 ++++++++++++++++++++-
 drivers/media/v4l2-core/v4l2-subdev.c | 20 +++++++++++++++++-
 include/uapi/linux/videodev2.h        | 30 +++++++++++++++++++++++++++
 4 files changed, 76 insertions(+), 3 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-event.c b/drivers/media/v4l2-core/v4l2-event.c
index 9d673d113d7a..290c6b213179 100644
--- a/drivers/media/v4l2-core/v4l2-event.c
+++ b/drivers/media/v4l2-core/v4l2-event.c
@@ -27,6 +27,7 @@ static unsigned sev_pos(const struct v4l2_subscribed_event *sev, unsigned idx)
 static int __v4l2_event_dequeue(struct v4l2_fh *fh, struct v4l2_event *event)
 {
 	struct v4l2_kevent *kev;
+	struct timespec64 ts;
 	unsigned long flags;
 
 	spin_lock_irqsave(&fh->vdev->fh_lock, flags);
@@ -44,7 +45,9 @@ static int __v4l2_event_dequeue(struct v4l2_fh *fh, struct v4l2_event *event)
 
 	kev->event.pending = fh->navailable;
 	*event = kev->event;
-	event->timestamp = ns_to_timespec(kev->ts);
+	ts = ns_to_timespec64(kev->ts);
+	event->timestamp.tv_sec = ts.tv_sec;
+	event->timestamp.tv_nsec = ts.tv_nsec;
 	kev->sev->first = sev_pos(kev->sev, 1);
 	kev->sev->in_use--;
 
diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
index 693f9eb8e01b..1de939d11628 100644
--- a/drivers/media/v4l2-core/v4l2-ioctl.c
+++ b/drivers/media/v4l2-core/v4l2-ioctl.c
@@ -821,7 +821,7 @@ static void v4l_print_event(const void *arg, bool write_only)
 	const struct v4l2_event *p = arg;
 	const struct v4l2_event_ctrl *c;
 
-	pr_cont("type=0x%x, pending=%u, sequence=%u, id=%u, timestamp=%lu.%9.9lu\n",
+	pr_cont("type=0x%x, pending=%u, sequence=%u, id=%u, timestamp=%llu.%9.9llu\n",
 			p->type, p->pending, p->sequence, p->id,
 			p->timestamp.tv_sec, p->timestamp.tv_nsec);
 	switch (p->type) {
@@ -3010,6 +3010,13 @@ static int check_array_args(unsigned int cmd, void *parg, size_t *array_size,
 
 static unsigned int video_translate_cmd(unsigned int cmd)
 {
+	switch (cmd) {
+#ifdef CONFIG_COMPAT_32BIT_TIME
+	case VIDIOC_DQEVENT_TIME32:
+		return VIDIOC_DQEVENT;
+#endif
+	}
+
 	return cmd;
 }
 
@@ -3059,6 +3066,21 @@ static int video_put_user(void __user *arg, void *parg, unsigned int cmd)
 		return 0;
 
 	switch (cmd) {
+#ifdef CONFIG_COMPAT_32BIT_TIME
+	case VIDIOC_DQEVENT_TIME32: {
+		struct v4l2_event_time32 ev32;
+		struct v4l2_event *ev = parg;
+
+	        memcpy(&ev32, ev, offsetof(struct v4l2_event, timestamp));
+	        ev32.timestamp.tv_sec = ev->timestamp.tv_sec;
+	        ev32.timestamp.tv_nsec = ev->timestamp.tv_nsec;
+	        memcpy(&ev32.id, &ev->id, sizeof(*ev) - offsetof(struct v4l2_event, id));
+
+		if (copy_to_user(arg, &ev32, sizeof(ev32)))
+			return -EFAULT;
+		break;
+	}
+#endif
 	default:
 		/*  Copy results into user buffer  */
 		if (copy_to_user(arg, parg, _IOC_SIZE(cmd)))
diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
index f725cd9b66b9..45454a628e45 100644
--- a/drivers/media/v4l2-core/v4l2-subdev.c
+++ b/drivers/media/v4l2-core/v4l2-subdev.c
@@ -331,8 +331,8 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
 	struct v4l2_fh *vfh = file->private_data;
 #if defined(CONFIG_VIDEO_V4L2_SUBDEV_API)
 	struct v4l2_subdev_fh *subdev_fh = to_v4l2_subdev_fh(vfh);
-	int rval;
 #endif
+	int rval;
 
 	switch (cmd) {
 	case VIDIOC_QUERYCTRL:
@@ -392,6 +392,24 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
 
 		return v4l2_event_dequeue(vfh, arg, file->f_flags & O_NONBLOCK);
 
+	case VIDIOC_DQEVENT_TIME32: {
+		struct v4l2_event_time32 *ev32 = arg;
+		struct v4l2_event ev;
+
+		if (!(sd->flags & V4L2_SUBDEV_FL_HAS_EVENTS))
+			return -ENOIOCTLCMD;
+
+		rval = v4l2_event_dequeue(vfh, &ev, file->f_flags & O_NONBLOCK);
+
+		memcpy(ev32, &ev, offsetof(struct v4l2_event, timestamp));
+		ev32->timestamp.tv_sec = ev.timestamp.tv_sec;
+		ev32->timestamp.tv_nsec = ev.timestamp.tv_nsec;
+		memcpy(&ev32->id, &ev.id,
+		       sizeof(ev) - offsetof(struct v4l2_event, id));
+
+		return rval;
+	}
+
 	case VIDIOC_SUBSCRIBE_EVENT:
 		return v4l2_subdev_call(sd, core, subscribe_event, vfh, arg);
 
diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
index 74d3d522f3db..1d2553d4ed5b 100644
--- a/include/uapi/linux/videodev2.h
+++ b/include/uapi/linux/videodev2.h
@@ -2329,11 +2329,40 @@ struct v4l2_event {
 	} u;
 	__u32				pending;
 	__u32				sequence;
+#ifdef __KERNEL__
+	struct __kernel_timespec	timestamp;
+#else
 	struct timespec			timestamp;
+#endif
 	__u32				id;
 	__u32				reserved[8];
 };
 
+#ifdef __KERNEL__
+/*
+ * The user space interpretation of the 'v4l2_event' differs
+ * based on the 'time_t' definition on 32-bit architectures, so
+ * the kernel has to handle both.
+ * This is the old version for 32-bit architectures.
+ */
+struct v4l2_event_time32 {
+	__u32				type;
+	union {
+		struct v4l2_event_vsync		vsync;
+		struct v4l2_event_ctrl		ctrl;
+		struct v4l2_event_frame_sync	frame_sync;
+		struct v4l2_event_src_change	src_change;
+		struct v4l2_event_motion_det	motion_det;
+		__u8				data[64];
+	} u;
+	__u32				pending;
+	__u32				sequence;
+	struct old_timespec32		timestamp;
+	__u32				id;
+	__u32				reserved[8];
+};
+#endif
+
 #define V4L2_EVENT_SUB_FL_SEND_INITIAL		(1 << 0)
 #define V4L2_EVENT_SUB_FL_ALLOW_FEEDBACK	(1 << 1)
 
@@ -2486,6 +2515,7 @@ struct v4l2_create_buffers {
 #define	VIDIOC_S_DV_TIMINGS	_IOWR('V', 87, struct v4l2_dv_timings)
 #define	VIDIOC_G_DV_TIMINGS	_IOWR('V', 88, struct v4l2_dv_timings)
 #define	VIDIOC_DQEVENT		 _IOR('V', 89, struct v4l2_event)
+#define	VIDIOC_DQEVENT_TIME32	 _IOR('V', 89, struct v4l2_event_time32)
 #define	VIDIOC_SUBSCRIBE_EVENT	 _IOW('V', 90, struct v4l2_event_subscription)
 #define	VIDIOC_UNSUBSCRIBE_EVENT _IOW('V', 91, struct v4l2_event_subscription)
 #define VIDIOC_CREATE_BUFS	_IOWR('V', 92, struct v4l2_create_buffers)
-- 
2.20.0


^ permalink raw reply related	[flat|nested] 25+ messages in thread

* [PATCH v4 6/8] media: v4l2-core: fix v4l2_buffer handling for time64 ABI
  2019-11-11 20:38 [PATCH v4 0/8] y2038 safety in v4l2 Arnd Bergmann
                   ` (4 preceding siblings ...)
  2019-11-11 20:38 ` [PATCH v4 5/8] media: v4l2-core: fix VIDIOC_DQEVENT for time64 ABI Arnd Bergmann
@ 2019-11-11 20:38 ` Arnd Bergmann
  2019-11-25 14:57   ` Hans Verkuil
  2019-11-11 20:38 ` [PATCH v4 7/8] media: v4l2-core: fix compat VIDIOC_DQEVENT " Arnd Bergmann
                   ` (2 subsequent siblings)
  8 siblings, 1 reply; 25+ messages in thread
From: Arnd Bergmann @ 2019-11-11 20:38 UTC (permalink / raw)
  To: Mauro Carvalho Chehab, linux-media
  Cc: Hans Verkuil, linux-kernel, y2038, Arnd Bergmann

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 | 57 ++++++++++++++++++++++++++--
 include/uapi/linux/videodev2.h       | 45 ++++++++++++++++++++++
 2 files changed, 98 insertions(+), 4 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
index 1de939d11628..4ae1bcaec3fa 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,
@@ -3014,6 +3014,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
 	}
 
@@ -3032,6 +3040,30 @@ 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;
+
+		memcpy(vb, &vb32, offsetof(struct v4l2_buffer, timestamp));
+		vb->timestamp.tv_sec = vb32.timestamp.tv_sec;
+		vb->timestamp.tv_usec = vb32.timestamp.tv_usec;
+	        memcpy(&vb->timecode, &vb32.timecode,
+		       sizeof(*vb) - offsetof(struct v4l2_buffer, timecode));
+
+		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,
@@ -3080,6 +3112,23 @@ 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_time32 vb32;
+		struct v4l2_buffer *vb = parg;
+
+		memcpy(&vb32, vb, offsetof(struct v4l2_buffer, timestamp));
+		vb32.timestamp.tv_sec = vb->timestamp.tv_sec;
+		vb32.timestamp.tv_usec = vb->timestamp.tv_usec;
+	        memcpy(&vb32.timecode, &vb->timecode,
+		       sizeof(*vb) - offsetof(struct v4l2_buffer, timecode));
+
+		if (copy_to_user(arg, &vb32, sizeof(vb32)))
+			return -EFAULT;
+		break;
+	}
 #endif
 	default:
 		/*  Copy results into user buffer  */
diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
index 1d2553d4ed5b..f05c54d63f96 100644
--- a/include/uapi/linux/videodev2.h
+++ b/include/uapi/linux/videodev2.h
@@ -990,7 +990,47 @@ struct v4l2_buffer {
 	__u32			bytesused;
 	__u32			flags;
 	__u32			field;
+#ifdef __KERNEL__
+	/* match glibc timeval64 format */
+	struct {
+		long long	tv_sec;
+# if defined(__sparc__) && defined(__arch64__)
+		int		tv_usec;
+		int		__pad;
+# else
+		long long	tv_usec;
+# endif
+	} timestamp;
+#else
 	struct timeval		timestamp;
+#endif
+	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;
+	};
+};
+
+#ifdef __KERNEL__
+struct v4l2_buffer_time32 {
+	__u32			index;
+	__u32			type;
+	__u32			bytesused;
+	__u32			flags;
+	__u32			field;
+	struct old_timeval32	timestamp;
 	struct v4l2_timecode	timecode;
 	__u32			sequence;
 
@@ -1009,6 +1049,7 @@ struct v4l2_buffer {
 		__u32		reserved;
 	};
 };
+#endif
 
 #ifndef __KERNEL__
 /**
@@ -2446,12 +2487,15 @@ struct v4l2_create_buffers {
 #define VIDIOC_S_FMT		_IOWR('V',  5, struct v4l2_format)
 #define VIDIOC_REQBUFS		_IOWR('V',  8, struct v4l2_requestbuffers)
 #define VIDIOC_QUERYBUF		_IOWR('V',  9, struct v4l2_buffer)
+#define VIDIOC_QUERYBUF_TIME32	_IOWR('V',  9, struct v4l2_buffer_time32)
 #define VIDIOC_G_FBUF		 _IOR('V', 10, struct v4l2_framebuffer)
 #define VIDIOC_S_FBUF		 _IOW('V', 11, struct v4l2_framebuffer)
 #define VIDIOC_OVERLAY		 _IOW('V', 14, int)
 #define VIDIOC_QBUF		_IOWR('V', 15, struct v4l2_buffer)
+#define VIDIOC_QBUF_TIME32	_IOWR('V', 15, struct v4l2_buffer_time32)
 #define VIDIOC_EXPBUF		_IOWR('V', 16, struct v4l2_exportbuffer)
 #define VIDIOC_DQBUF		_IOWR('V', 17, struct v4l2_buffer)
+#define VIDIOC_DQBUF_TIME32	_IOWR('V', 17, struct v4l2_buffer_time32)
 #define VIDIOC_STREAMON		 _IOW('V', 18, int)
 #define VIDIOC_STREAMOFF	 _IOW('V', 19, int)
 #define VIDIOC_G_PARM		_IOWR('V', 21, struct v4l2_streamparm)
@@ -2520,6 +2564,7 @@ struct v4l2_create_buffers {
 #define	VIDIOC_UNSUBSCRIBE_EVENT _IOW('V', 91, struct v4l2_event_subscription)
 #define VIDIOC_CREATE_BUFS	_IOWR('V', 92, struct v4l2_create_buffers)
 #define VIDIOC_PREPARE_BUF	_IOWR('V', 93, struct v4l2_buffer)
+#define VIDIOC_PREPARE_BUF_TIME32 _IOWR('V', 93, struct v4l2_buffer_time32)
 #define VIDIOC_G_SELECTION	_IOWR('V', 94, struct v4l2_selection)
 #define VIDIOC_S_SELECTION	_IOWR('V', 95, struct v4l2_selection)
 #define VIDIOC_DECODER_CMD	_IOWR('V', 96, struct v4l2_decoder_cmd)
-- 
2.20.0


^ permalink raw reply related	[flat|nested] 25+ messages in thread

* [PATCH v4 7/8] media: v4l2-core: fix compat VIDIOC_DQEVENT for time64 ABI
  2019-11-11 20:38 [PATCH v4 0/8] y2038 safety in v4l2 Arnd Bergmann
                   ` (5 preceding siblings ...)
  2019-11-11 20:38 ` [PATCH v4 6/8] media: v4l2-core: fix v4l2_buffer handling " Arnd Bergmann
@ 2019-11-11 20:38 ` Arnd Bergmann
  2019-11-11 20:38 ` [PATCH v4 8/8] media: v4l2-core: fix compat v4l2_buffer handling " Arnd Bergmann
  2019-11-25 16:02 ` [PATCH v4 0/8] y2038 safety in v4l2 Hans Verkuil
  8 siblings, 0 replies; 25+ messages in thread
From: Arnd Bergmann @ 2019-11-11 20:38 UTC (permalink / raw)
  To: Mauro Carvalho Chehab, linux-media
  Cc: Hans Verkuil, linux-kernel, y2038, Arnd Bergmann

The native code supports the variant of struct v4l2_event for 64-bit
time_t, so add the compat version as well.

Here, a new incompatibility arises: while almost all 32-bit architectures
now use the same layout as 64-bit architectures and the commands can
simply be passed through, on x86 the internal alignment of v4l2_event
is different because of the 64-bit member in v4l2_event_ctrl.

To handle all architectures, this now requires defining four different
versions of the structure to cover all possible combinations. The compat
handling for VIDIOC_DQEVENT32 and VIDIOC_DQEVENT32_TIME32 is now inside
of an #ifdef so it does not get used on architectures other than x86.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/media/v4l2-core/v4l2-compat-ioctl32.c | 57 ++++++++++++++++++-
 1 file changed, 56 insertions(+), 1 deletion(-)

diff --git a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
index 7ad6db8dd9f6..46cd84879c1f 100644
--- a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
+++ b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
@@ -1028,6 +1028,15 @@ static int put_v4l2_ext_controls32(struct file *file,
 	return 0;
 }
 
+#ifdef CONFIG_X86_64
+/*
+ * x86 is the only compat architecture with different struct alignment
+ * between 32-bit and 64-bit tasks.
+ *
+ * On all other architectures, v4l2_event32 and v4l2_event32_time32 are
+ * the same as v4l2_event and v4l2_event_time32, so we can use the native
+ * handlers, converting v4l2_event to v4l2_event_time32 if necessary.
+ */
 struct v4l2_event32 {
 	__u32				type;
 	union {
@@ -1036,7 +1045,20 @@ struct v4l2_event32 {
 	} u;
 	__u32				pending;
 	__u32				sequence;
-	struct compat_timespec		timestamp;
+	struct __kernel_timespec	timestamp;
+	__u32				id;
+	__u32				reserved[8];
+};
+
+struct v4l2_event32_time32 {
+	__u32				type;
+	union {
+		compat_s64		value64;
+		__u8			data[64];
+	} u;
+	__u32				pending;
+	__u32				sequence;
+	struct old_timespec32		timestamp;
 	__u32				id;
 	__u32				reserved[8];
 };
@@ -1057,6 +1079,23 @@ static int put_v4l2_event32(struct v4l2_event __user *p64,
 	return 0;
 }
 
+static int put_v4l2_event32_time32(struct v4l2_event_time32 __user *p64,
+				   struct v4l2_event32_time32 __user *p32)
+{
+	if (!access_ok(p32, sizeof(*p32)) ||
+	    assign_in_user(&p32->type, &p64->type) ||
+	    copy_in_user(&p32->u, &p64->u, sizeof(p64->u)) ||
+	    assign_in_user(&p32->pending, &p64->pending) ||
+	    assign_in_user(&p32->sequence, &p64->sequence) ||
+	    assign_in_user(&p32->timestamp.tv_sec, &p64->timestamp.tv_sec) ||
+	    assign_in_user(&p32->timestamp.tv_nsec, &p64->timestamp.tv_nsec) ||
+	    assign_in_user(&p32->id, &p64->id) ||
+	    copy_in_user(p32->reserved, p64->reserved, sizeof(p32->reserved)))
+		return -EFAULT;
+	return 0;
+}
+#endif
+
 struct v4l2_edid32 {
 	__u32 pad;
 	__u32 start_block;
@@ -1121,6 +1160,7 @@ static int put_v4l2_edid32(struct v4l2_edid __user *p64,
 #define VIDIOC_S_EXT_CTRLS32    _IOWR('V', 72, struct v4l2_ext_controls32)
 #define VIDIOC_TRY_EXT_CTRLS32  _IOWR('V', 73, struct v4l2_ext_controls32)
 #define	VIDIOC_DQEVENT32	_IOR ('V', 89, struct v4l2_event32)
+#define	VIDIOC_DQEVENT32_TIME32	_IOR ('V', 89, struct v4l2_event32_time32)
 #define VIDIOC_CREATE_BUFS32	_IOWR('V', 92, struct v4l2_create_buffers32)
 #define VIDIOC_PREPARE_BUF32	_IOWR('V', 93, struct v4l2_buffer32)
 
@@ -1202,7 +1242,10 @@ static long do_video_ioctl(struct file *file, unsigned int cmd, unsigned long ar
 	case VIDIOC_G_EXT_CTRLS32: ncmd = VIDIOC_G_EXT_CTRLS; break;
 	case VIDIOC_S_EXT_CTRLS32: ncmd = VIDIOC_S_EXT_CTRLS; break;
 	case VIDIOC_TRY_EXT_CTRLS32: ncmd = VIDIOC_TRY_EXT_CTRLS; break;
+#ifdef CONFIG_X86_64
 	case VIDIOC_DQEVENT32: ncmd = VIDIOC_DQEVENT; break;
+	case VIDIOC_DQEVENT32_TIME32: ncmd = VIDIOC_DQEVENT_TIME32; break;
+#endif
 	case VIDIOC_OVERLAY32: ncmd = VIDIOC_OVERLAY; break;
 	case VIDIOC_STREAMON32: ncmd = VIDIOC_STREAMON; break;
 	case VIDIOC_STREAMOFF32: ncmd = VIDIOC_STREAMOFF; break;
@@ -1336,10 +1379,16 @@ static long do_video_ioctl(struct file *file, unsigned int cmd, unsigned long ar
 		}
 		compatible_arg = 0;
 		break;
+#ifdef CONFIG_X86_64
 	case VIDIOC_DQEVENT32:
 		err = alloc_userspace(sizeof(struct v4l2_event), 0, &new_p64);
 		compatible_arg = 0;
 		break;
+	case VIDIOC_DQEVENT32_TIME32:
+		err = alloc_userspace(sizeof(struct v4l2_event_time32), 0, &new_p64);
+		compatible_arg = 0;
+		break;
+#endif
 	}
 	if (err)
 		return err;
@@ -1404,10 +1453,16 @@ static long do_video_ioctl(struct file *file, unsigned int cmd, unsigned long ar
 		err = put_v4l2_framebuffer32(new_p64, p32);
 		break;
 
+#ifdef CONFIG_X86_64
 	case VIDIOC_DQEVENT32:
 		err = put_v4l2_event32(new_p64, p32);
 		break;
 
+	case VIDIOC_DQEVENT32_TIME32:
+		err = put_v4l2_event32_time32(new_p64, p32);
+		break;
+#endif
+
 	case VIDIOC_G_EDID32:
 		err = put_v4l2_edid32(new_p64, p32);
 		break;
-- 
2.20.0


^ permalink raw reply related	[flat|nested] 25+ messages in thread

* [PATCH v4 8/8] media: v4l2-core: fix compat v4l2_buffer handling for time64 ABI
  2019-11-11 20:38 [PATCH v4 0/8] y2038 safety in v4l2 Arnd Bergmann
                   ` (6 preceding siblings ...)
  2019-11-11 20:38 ` [PATCH v4 7/8] media: v4l2-core: fix compat VIDIOC_DQEVENT " Arnd Bergmann
@ 2019-11-11 20:38 ` Arnd Bergmann
  2019-11-25 16:02 ` [PATCH v4 0/8] y2038 safety in v4l2 Hans Verkuil
  8 siblings, 0 replies; 25+ messages in thread
From: Arnd Bergmann @ 2019-11-11 20:38 UTC (permalink / raw)
  To: Mauro Carvalho Chehab, linux-media
  Cc: Hans Verkuil, linux-kernel, y2038, Arnd Bergmann

Add support for the four new variants of ioctl commands for 64-bit time_t
in v4l2_buffer.

The existing v4l2_buffer32 structure for the traditional format gets
changed to match the new v4l2_buffer format, and the old layout is
now called v4l2_buffer32_time32. Neither of these matches the native
64-bit architecture format of v4l2_buffer, so both require special
handling in compat code.

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.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/media/v4l2-core/v4l2-compat-ioctl32.c | 265 +++++++++++++++++-
 1 file changed, 264 insertions(+), 1 deletion(-)

diff --git a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
index 46cd84879c1f..3bbf47d950e0 100644
--- a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
+++ b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
@@ -474,7 +474,33 @@ struct v4l2_buffer32 {
 	__u32			bytesused;
 	__u32			flags;
 	__u32			field;	/* enum v4l2_field */
-	struct compat_timeval	timestamp;
+	struct {
+		long long	tv_sec;
+		long long	tv_usec;
+	}			timestamp;
+	struct v4l2_timecode	timecode;
+	__u32			sequence;
+
+	/* memory location */
+	__u32			memory;	/* enum v4l2_memory */
+	union {
+		__u32           offset;
+		compat_long_t   userptr;
+		compat_caddr_t  planes;
+		__s32		fd;
+	} m;
+	__u32			length;
+	__u32			reserved2;
+	__s32			request_fd;
+};
+
+struct v4l2_buffer32_time32 {
+	__u32			index;
+	__u32			type;	/* enum v4l2_buf_type */
+	__u32			bytesused;
+	__u32			flags;
+	__u32			field;	/* enum v4l2_field */
+	struct old_timeval32	timestamp;
 	struct v4l2_timecode	timecode;
 	__u32			sequence;
 
@@ -581,6 +607,31 @@ static int bufsize_v4l2_buffer(struct v4l2_buffer32 __user *p32, u32 *size)
 	return 0;
 }
 
+static int bufsize_v4l2_buffer_time32(struct v4l2_buffer32_time32 __user *p32, u32 *size)
+{
+	u32 type;
+	u32 length;
+
+	if (!access_ok(p32, sizeof(*p32)) ||
+	    get_user(type, &p32->type) ||
+	    get_user(length, &p32->length))
+		return -EFAULT;
+
+	if (V4L2_TYPE_IS_MULTIPLANAR(type)) {
+		if (length > VIDEO_MAX_PLANES)
+			return -EINVAL;
+
+		/*
+		 * We don't really care if userspace decides to kill itself
+		 * by passing a very big length value
+		 */
+		*size = length * sizeof(struct v4l2_plane);
+	} else {
+		*size = 0;
+	}
+	return 0;
+}
+
 static int get_v4l2_buffer32(struct v4l2_buffer __user *p64,
 			     struct v4l2_buffer32 __user *p32,
 			     void __user *aux_buf, u32 aux_space)
@@ -681,6 +732,106 @@ static int get_v4l2_buffer32(struct v4l2_buffer __user *p64,
 	return 0;
 }
 
+static int get_v4l2_buffer32_time32(struct v4l2_buffer_time32 __user *p64,
+				    struct v4l2_buffer32_time32 __user *p32,
+				    void __user *aux_buf, u32 aux_space)
+{
+	u32 type;
+	u32 length;
+	s32 request_fd;
+	enum v4l2_memory memory;
+	struct v4l2_plane32 __user *uplane32;
+	struct v4l2_plane __user *uplane;
+	compat_caddr_t p;
+	int ret;
+
+	if (!access_ok(p32, sizeof(*p32)) ||
+	    assign_in_user(&p64->index, &p32->index) ||
+	    get_user(type, &p32->type) ||
+	    put_user(type, &p64->type) ||
+	    assign_in_user(&p64->flags, &p32->flags) ||
+	    get_user(memory, &p32->memory) ||
+	    put_user(memory, &p64->memory) ||
+	    get_user(length, &p32->length) ||
+	    put_user(length, &p64->length) ||
+	    get_user(request_fd, &p32->request_fd) ||
+	    put_user(request_fd, &p64->request_fd))
+		return -EFAULT;
+
+	if (V4L2_TYPE_IS_OUTPUT(type))
+		if (assign_in_user(&p64->bytesused, &p32->bytesused) ||
+		    assign_in_user(&p64->field, &p32->field) ||
+		    assign_in_user(&p64->timestamp.tv_sec,
+				   &p32->timestamp.tv_sec) ||
+		    assign_in_user(&p64->timestamp.tv_usec,
+				   &p32->timestamp.tv_usec))
+			return -EFAULT;
+
+	if (V4L2_TYPE_IS_MULTIPLANAR(type)) {
+		u32 num_planes = length;
+
+		if (num_planes == 0) {
+			/*
+			 * num_planes == 0 is legal, e.g. when userspace doesn't
+			 * need planes array on DQBUF
+			 */
+			return put_user(NULL, &p64->m.planes);
+		}
+		if (num_planes > VIDEO_MAX_PLANES)
+			return -EINVAL;
+
+		if (get_user(p, &p32->m.planes))
+			return -EFAULT;
+
+		uplane32 = compat_ptr(p);
+		if (!access_ok(uplane32,
+			       num_planes * sizeof(*uplane32)))
+			return -EFAULT;
+
+		/*
+		 * We don't really care if userspace decides to kill itself
+		 * by passing a very big num_planes value
+		 */
+		if (aux_space < num_planes * sizeof(*uplane))
+			return -EFAULT;
+
+		uplane = aux_buf;
+		if (put_user_force(uplane, &p64->m.planes))
+			return -EFAULT;
+
+		while (num_planes--) {
+			ret = get_v4l2_plane32(uplane, uplane32, memory);
+			if (ret)
+				return ret;
+			uplane++;
+			uplane32++;
+		}
+	} else {
+		switch (memory) {
+		case V4L2_MEMORY_MMAP:
+		case V4L2_MEMORY_OVERLAY:
+			if (assign_in_user(&p64->m.offset, &p32->m.offset))
+				return -EFAULT;
+			break;
+		case V4L2_MEMORY_USERPTR: {
+			compat_ulong_t userptr;
+
+			if (get_user(userptr, &p32->m.userptr) ||
+			    put_user((unsigned long)compat_ptr(userptr),
+				     &p64->m.userptr))
+				return -EFAULT;
+			break;
+		}
+		case V4L2_MEMORY_DMABUF:
+			if (assign_in_user(&p64->m.fd, &p32->m.fd))
+				return -EFAULT;
+			break;
+		}
+	}
+
+	return 0;
+}
+
 static int put_v4l2_buffer32(struct v4l2_buffer __user *p64,
 			     struct v4l2_buffer32 __user *p32)
 {
@@ -761,6 +912,87 @@ static int put_v4l2_buffer32(struct v4l2_buffer __user *p64,
 	return 0;
 }
 
+
+static int put_v4l2_buffer32_time32(struct v4l2_buffer_time32 __user *p64,
+				    struct v4l2_buffer32_time32 __user *p32)
+{
+	u32 type;
+	u32 length;
+	enum v4l2_memory memory;
+	struct v4l2_plane32 __user *uplane32;
+	struct v4l2_plane *uplane;
+	compat_caddr_t p;
+	int ret;
+
+	if (!access_ok(p32, sizeof(*p32)) ||
+	    assign_in_user(&p32->index, &p64->index) ||
+	    get_user(type, &p64->type) ||
+	    put_user(type, &p32->type) ||
+	    assign_in_user(&p32->flags, &p64->flags) ||
+	    get_user(memory, &p64->memory) ||
+	    put_user(memory, &p32->memory))
+		return -EFAULT;
+
+	if (assign_in_user(&p32->bytesused, &p64->bytesused) ||
+	    assign_in_user(&p32->field, &p64->field) ||
+	    assign_in_user(&p32->timestamp.tv_sec, &p64->timestamp.tv_sec) ||
+	    assign_in_user(&p32->timestamp.tv_usec, &p64->timestamp.tv_usec) ||
+	    copy_in_user(&p32->timecode, &p64->timecode, sizeof(p64->timecode)) ||
+	    assign_in_user(&p32->sequence, &p64->sequence) ||
+	    assign_in_user(&p32->reserved2, &p64->reserved2) ||
+	    assign_in_user(&p32->request_fd, &p64->request_fd) ||
+	    get_user(length, &p64->length) ||
+	    put_user(length, &p32->length))
+		return -EFAULT;
+
+	if (V4L2_TYPE_IS_MULTIPLANAR(type)) {
+		u32 num_planes = length;
+
+		if (num_planes == 0)
+			return 0;
+		/* We need to define uplane without __user, even though
+		 * it does point to data in userspace here. The reason is
+		 * that v4l2-ioctl.c copies it from userspace to kernelspace,
+		 * so its definition in videodev2.h doesn't have a
+		 * __user markup. Defining uplane with __user causes
+		 * smatch warnings, so instead declare it without __user
+		 * and cast it as a userspace pointer to put_v4l2_plane32().
+		 */
+		if (get_user(uplane, &p64->m.planes))
+			return -EFAULT;
+		if (get_user(p, &p32->m.planes))
+			return -EFAULT;
+		uplane32 = compat_ptr(p);
+
+		while (num_planes--) {
+			ret = put_v4l2_plane32((void __user *)uplane,
+					       uplane32, memory);
+			if (ret)
+				return ret;
+			++uplane;
+			++uplane32;
+		}
+	} else {
+		switch (memory) {
+		case V4L2_MEMORY_MMAP:
+		case V4L2_MEMORY_OVERLAY:
+			if (assign_in_user(&p32->m.offset, &p64->m.offset))
+				return -EFAULT;
+			break;
+		case V4L2_MEMORY_USERPTR:
+			if (assign_in_user(&p32->m.userptr, &p64->m.userptr))
+				return -EFAULT;
+			break;
+		case V4L2_MEMORY_DMABUF:
+			if (assign_in_user(&p32->m.fd, &p64->m.fd))
+				return -EFAULT;
+			break;
+		}
+	}
+
+	return 0;
+}
+
 struct v4l2_framebuffer32 {
 	__u32			capability;
 	__u32			flags;
@@ -1147,10 +1379,13 @@ static int put_v4l2_edid32(struct v4l2_edid __user *p64,
 #define VIDIOC_G_FMT32		_IOWR('V',  4, struct v4l2_format32)
 #define VIDIOC_S_FMT32		_IOWR('V',  5, struct v4l2_format32)
 #define VIDIOC_QUERYBUF32	_IOWR('V',  9, struct v4l2_buffer32)
+#define VIDIOC_QUERYBUF32_TIME32 _IOWR('V',  9, struct v4l2_buffer32_time32)
 #define VIDIOC_G_FBUF32		_IOR ('V', 10, struct v4l2_framebuffer32)
 #define VIDIOC_S_FBUF32		_IOW ('V', 11, struct v4l2_framebuffer32)
 #define VIDIOC_QBUF32		_IOWR('V', 15, struct v4l2_buffer32)
+#define VIDIOC_QBUF32_TIME32	_IOWR('V', 15, struct v4l2_buffer32_time32)
 #define VIDIOC_DQBUF32		_IOWR('V', 17, struct v4l2_buffer32)
+#define VIDIOC_DQBUF32_TIME32	_IOWR('V', 17, struct v4l2_buffer32_time32)
 #define VIDIOC_ENUMSTD32	_IOWR('V', 25, struct v4l2_standard32)
 #define VIDIOC_ENUMINPUT32	_IOWR('V', 26, struct v4l2_input32)
 #define VIDIOC_G_EDID32		_IOWR('V', 40, struct v4l2_edid32)
@@ -1163,6 +1398,7 @@ static int put_v4l2_edid32(struct v4l2_edid __user *p64,
 #define	VIDIOC_DQEVENT32_TIME32	_IOR ('V', 89, struct v4l2_event32_time32)
 #define VIDIOC_CREATE_BUFS32	_IOWR('V', 92, struct v4l2_create_buffers32)
 #define VIDIOC_PREPARE_BUF32	_IOWR('V', 93, struct v4l2_buffer32)
+#define VIDIOC_PREPARE_BUF32_TIME32 _IOWR('V', 93, struct v4l2_buffer32_time32)
 
 #define VIDIOC_OVERLAY32	_IOW ('V', 14, s32)
 #define VIDIOC_STREAMON32	_IOW ('V', 18, s32)
@@ -1232,10 +1468,13 @@ static long do_video_ioctl(struct file *file, unsigned int cmd, unsigned long ar
 	case VIDIOC_G_FMT32: ncmd = VIDIOC_G_FMT; break;
 	case VIDIOC_S_FMT32: ncmd = VIDIOC_S_FMT; break;
 	case VIDIOC_QUERYBUF32: ncmd = VIDIOC_QUERYBUF; break;
+	case VIDIOC_QUERYBUF32_TIME32: ncmd = VIDIOC_QUERYBUF_TIME32; break;
 	case VIDIOC_G_FBUF32: ncmd = VIDIOC_G_FBUF; break;
 	case VIDIOC_S_FBUF32: ncmd = VIDIOC_S_FBUF; break;
 	case VIDIOC_QBUF32: ncmd = VIDIOC_QBUF; break;
+	case VIDIOC_QBUF32_TIME32: ncmd = VIDIOC_QBUF_TIME32; break;
 	case VIDIOC_DQBUF32: ncmd = VIDIOC_DQBUF; break;
+	case VIDIOC_DQBUF32_TIME32: ncmd = VIDIOC_DQBUF_TIME32; break;
 	case VIDIOC_ENUMSTD32: ncmd = VIDIOC_ENUMSTD; break;
 	case VIDIOC_ENUMINPUT32: ncmd = VIDIOC_ENUMINPUT; break;
 	case VIDIOC_TRY_FMT32: ncmd = VIDIOC_TRY_FMT; break;
@@ -1255,6 +1494,7 @@ static long do_video_ioctl(struct file *file, unsigned int cmd, unsigned long ar
 	case VIDIOC_S_OUTPUT32: ncmd = VIDIOC_S_OUTPUT; break;
 	case VIDIOC_CREATE_BUFS32: ncmd = VIDIOC_CREATE_BUFS; break;
 	case VIDIOC_PREPARE_BUF32: ncmd = VIDIOC_PREPARE_BUF; break;
+	case VIDIOC_PREPARE_BUF32_TIME32: ncmd = VIDIOC_PREPARE_BUF_TIME32; break;
 	case VIDIOC_G_EDID32: ncmd = VIDIOC_G_EDID; break;
 	case VIDIOC_S_EDID32: ncmd = VIDIOC_S_EDID; break;
 	default: ncmd = cmd; break;
@@ -1336,6 +1576,22 @@ static long do_video_ioctl(struct file *file, unsigned int cmd, unsigned long ar
 		compatible_arg = 0;
 		break;
 
+	case VIDIOC_PREPARE_BUF32_TIME32:
+	case VIDIOC_QUERYBUF32_TIME32:
+	case VIDIOC_QBUF32_TIME32:
+	case VIDIOC_DQBUF32_TIME32:
+		err = bufsize_v4l2_buffer_time32(p32, &aux_space);
+		if (!err)
+			err = alloc_userspace(sizeof(struct v4l2_buffer),
+					      aux_space, &new_p64);
+		if (!err) {
+			aux_buf = new_p64 + sizeof(struct v4l2_buffer);
+			err = get_v4l2_buffer32_time32(new_p64, p32,
+						       aux_buf, aux_space);
+		}
+		compatible_arg = 0;
+		break;
+
 	case VIDIOC_S_FBUF32:
 		err = alloc_userspace(sizeof(struct v4l2_framebuffer), 0,
 				      &new_p64);
@@ -1484,6 +1740,13 @@ static long do_video_ioctl(struct file *file, unsigned int cmd, unsigned long ar
 		err = put_v4l2_buffer32(new_p64, p32);
 		break;
 
+	case VIDIOC_PREPARE_BUF32_TIME32:
+	case VIDIOC_QUERYBUF32_TIME32:
+	case VIDIOC_QBUF32_TIME32:
+	case VIDIOC_DQBUF32_TIME32:
+		err = put_v4l2_buffer32_time32(new_p64, p32);
+		break;
+
 	case VIDIOC_ENUMSTD32:
 		err = put_v4l2_standard32(new_p64, p32);
 		break;
-- 
2.20.0


^ permalink raw reply related	[flat|nested] 25+ messages in thread

* Re: [PATCH v4 3/8] media: v4l2-core: compat: ignore native command codes
       [not found]   ` <20191122070015.D5A702068E@mail.kernel.org>
@ 2019-11-22 12:29     ` Arnd Bergmann
  0 siblings, 0 replies; 25+ messages in thread
From: Arnd Bergmann @ 2019-11-22 12:29 UTC (permalink / raw)
  To: Sasha Levin; +Cc: Mauro Carvalho Chehab, Hans Verkuil, linux-kernel, # 3.4.x

On Fri, Nov 22, 2019 at 8:00 AM Sasha Levin <sashal@kernel.org> wrote:
>
> Hi,
>
> [This is an automated email]
>
> This commit has been processed because it contains a -stable tag.
> The stable tag indicates that it's relevant for the following trees: all
>
> The bot has tested the following trees: v5.3.11, v4.19.84, v4.14.154, v4.9.201, v4.4.201.
>
> v5.3.11: Build OK!
> v4.19.84: Build OK!

Ok, good.

> v4.14.154: Failed to apply! Possible dependencies:
>     6dd0394f5fcd ("media: v4l2-compat-ioctl32: better name userspace pointers")
>     fef6cc6b3618 ("media: v4l2-compat-ioctl32: fix several __user annotations")

The fef6cc6b3618 is probably a candidate for backporting (it fixes smatch
and sparse warnings and should have no other effect), the 6dd0394f5fcd
may be a little too big (but also harmless).

The downside of not backporting the patch is that user space code built
with 64-bit time_t would get incorrect data rather than failing with an
error code on older kernels.

I do not expect to see backports of 64-bit time_t support to kernels older
than 4.19, so this probably won't matter much, but in theory it's still
possible that users can run into it.

> v4.9.201: Failed to apply! Possible dependencies:
>     6dd0394f5fcd ("media: v4l2-compat-ioctl32: better name userspace pointers")
>     a56bc171598c ("[media] v4l: compat: Prevent allocating excessive amounts of memory")
>     ba7ed691dcce ("[media] v4l2-compat-ioctl32: VIDIOC_S_EDID should return all fields on error")
>     fb9ffa6a7f7e ("[media] v4l: Add metadata buffer type and format")
>     fef6cc6b3618 ("media: v4l2-compat-ioctl32: fix several __user annotations")
>
> v4.4.201: Failed to apply! Possible dependencies:
>     0579e6e3a326 ("doc-rst: linux_tv: remove whitespaces")
>     17defc282fe6 ("Documentation: add meta-documentation for Sphinx and kernel-doc")
>     22cba31bae9d ("Documentation/sphinx: add basic working Sphinx configuration and build")
>     234d549662a7 ("doc-rst: video: use reference for VIDIOC_ENUMINPUT")
>     5377d91f3e88 ("doc-rst: linux_tv DocBook to reST migration (docs-next)")
>     6dd0394f5fcd ("media: v4l2-compat-ioctl32: better name userspace pointers")
>     7347081e8a52 ("doc-rst: linux_tv: simplify references")
>     789818845202 ("doc-rst: audio: Fix some cross references")
>     94fff0dc5333 ("doc-rst: dmx_fcalls: improve man-like format")
>     9e00ffca8cc7 ("doc-rst: querycap: fix troubles on some references")
>     af4a4d0db8ab ("doc-rst: linux_tv: Replace reference names to match ioctls")
>     c2b66cafdf02 ("[media] v4l: doc: Remove row numbers from tables")
>     e6702ee18e24 ("doc-rst: app-pri: Fix a bad reference")
>     fb9ffa6a7f7e ("[media] v4l: Add metadata buffer type and format")
>
>
> NOTE: The patch will not be queued to stable trees until it is upstream.
>
> How should we proceed with this patch?

I'm happy to provide a hand-backported version of the patch for the older
kernels if Mauro and Hans think we should do that, otherwise I think it's
we're fine with having it on 4.19+.

      Arnd

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v4 5/8] media: v4l2-core: fix VIDIOC_DQEVENT for time64 ABI
  2019-11-11 20:38 ` [PATCH v4 5/8] media: v4l2-core: fix VIDIOC_DQEVENT for time64 ABI Arnd Bergmann
@ 2019-11-25 14:40   ` Hans Verkuil
  2019-11-26 14:43     ` Arnd Bergmann
  0 siblings, 1 reply; 25+ messages in thread
From: Hans Verkuil @ 2019-11-25 14:40 UTC (permalink / raw)
  To: Arnd Bergmann, Mauro Carvalho Chehab, linux-media; +Cc: linux-kernel, y2038

On 11/11/19 9:38 PM, Arnd Bergmann wrote:
> The v4l2_event structure contains a 'struct timespec' member that is
> defined by the user space C library, creating an ABI incompatibility
> when that gets updated to a 64-bit time_t.
> 
> While passing a 32-bit time_t here would be sufficient for CLOCK_MONOTONIC
> timestamps, simply redefining the structure to use the kernel's
> __kernel_old_timespec would not work for any library that uses a copy
> of the linux/videodev2.h header file rather than including the copy from
> the latest kernel headers.
> 
> This means the kernel has to be changed to handle both versions of the
> structure layout on a 32-bit architecture. The easiest way to do this
> is during the copy from/to user space.
> 
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>  drivers/media/v4l2-core/v4l2-event.c  |  5 ++++-
>  drivers/media/v4l2-core/v4l2-ioctl.c  | 24 ++++++++++++++++++++-
>  drivers/media/v4l2-core/v4l2-subdev.c | 20 +++++++++++++++++-
>  include/uapi/linux/videodev2.h        | 30 +++++++++++++++++++++++++++
>  4 files changed, 76 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-event.c b/drivers/media/v4l2-core/v4l2-event.c
> index 9d673d113d7a..290c6b213179 100644
> --- a/drivers/media/v4l2-core/v4l2-event.c
> +++ b/drivers/media/v4l2-core/v4l2-event.c
> @@ -27,6 +27,7 @@ static unsigned sev_pos(const struct v4l2_subscribed_event *sev, unsigned idx)
>  static int __v4l2_event_dequeue(struct v4l2_fh *fh, struct v4l2_event *event)
>  {
>  	struct v4l2_kevent *kev;
> +	struct timespec64 ts;
>  	unsigned long flags;
>  
>  	spin_lock_irqsave(&fh->vdev->fh_lock, flags);
> @@ -44,7 +45,9 @@ static int __v4l2_event_dequeue(struct v4l2_fh *fh, struct v4l2_event *event)
>  
>  	kev->event.pending = fh->navailable;
>  	*event = kev->event;
> -	event->timestamp = ns_to_timespec(kev->ts);
> +	ts = ns_to_timespec64(kev->ts);
> +	event->timestamp.tv_sec = ts.tv_sec;
> +	event->timestamp.tv_nsec = ts.tv_nsec;
>  	kev->sev->first = sev_pos(kev->sev, 1);
>  	kev->sev->in_use--;
>  
> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
> index 693f9eb8e01b..1de939d11628 100644
> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
> @@ -821,7 +821,7 @@ static void v4l_print_event(const void *arg, bool write_only)
>  	const struct v4l2_event *p = arg;
>  	const struct v4l2_event_ctrl *c;
>  
> -	pr_cont("type=0x%x, pending=%u, sequence=%u, id=%u, timestamp=%lu.%9.9lu\n",
> +	pr_cont("type=0x%x, pending=%u, sequence=%u, id=%u, timestamp=%llu.%9.9llu\n",
>  			p->type, p->pending, p->sequence, p->id,
>  			p->timestamp.tv_sec, p->timestamp.tv_nsec);
>  	switch (p->type) {
> @@ -3010,6 +3010,13 @@ static int check_array_args(unsigned int cmd, void *parg, size_t *array_size,
>  
>  static unsigned int video_translate_cmd(unsigned int cmd)
>  {
> +	switch (cmd) {
> +#ifdef CONFIG_COMPAT_32BIT_TIME
> +	case VIDIOC_DQEVENT_TIME32:
> +		return VIDIOC_DQEVENT;
> +#endif
> +	}
> +
>  	return cmd;
>  }
>  
> @@ -3059,6 +3066,21 @@ static int video_put_user(void __user *arg, void *parg, unsigned int cmd)
>  		return 0;
>  
>  	switch (cmd) {
> +#ifdef CONFIG_COMPAT_32BIT_TIME
> +	case VIDIOC_DQEVENT_TIME32: {
> +		struct v4l2_event_time32 ev32;
> +		struct v4l2_event *ev = parg;
> +
> +	        memcpy(&ev32, ev, offsetof(struct v4l2_event, timestamp));
> +	        ev32.timestamp.tv_sec = ev->timestamp.tv_sec;
> +	        ev32.timestamp.tv_nsec = ev->timestamp.tv_nsec;
> +	        memcpy(&ev32.id, &ev->id, sizeof(*ev) - offsetof(struct v4l2_event, id));

This looks dangerous: due to 64-bit alignment requirements the
v4l2_event struct may end with a 4-byte hole at the end of the struct,
which you do not want to copy to ev32.

I think it is safer to just copy id and reserved separately:

		ev32.id = ev->id;
		memcpy(ev32.reserved, ev->reserved, sizeof(ev->reserved));

> +
> +		if (copy_to_user(arg, &ev32, sizeof(ev32)))
> +			return -EFAULT;
> +		break;
> +	}
> +#endif
>  	default:
>  		/*  Copy results into user buffer  */
>  		if (copy_to_user(arg, parg, _IOC_SIZE(cmd)))
> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> index f725cd9b66b9..45454a628e45 100644
> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> @@ -331,8 +331,8 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
>  	struct v4l2_fh *vfh = file->private_data;
>  #if defined(CONFIG_VIDEO_V4L2_SUBDEV_API)
>  	struct v4l2_subdev_fh *subdev_fh = to_v4l2_subdev_fh(vfh);
> -	int rval;
>  #endif
> +	int rval;
>  
>  	switch (cmd) {
>  	case VIDIOC_QUERYCTRL:
> @@ -392,6 +392,24 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
>  
>  		return v4l2_event_dequeue(vfh, arg, file->f_flags & O_NONBLOCK);
>  
> +	case VIDIOC_DQEVENT_TIME32: {
> +		struct v4l2_event_time32 *ev32 = arg;
> +		struct v4l2_event ev;
> +
> +		if (!(sd->flags & V4L2_SUBDEV_FL_HAS_EVENTS))
> +			return -ENOIOCTLCMD;
> +
> +		rval = v4l2_event_dequeue(vfh, &ev, file->f_flags & O_NONBLOCK);
> +
> +		memcpy(ev32, &ev, offsetof(struct v4l2_event, timestamp));
> +		ev32->timestamp.tv_sec = ev.timestamp.tv_sec;
> +		ev32->timestamp.tv_nsec = ev.timestamp.tv_nsec;
> +		memcpy(&ev32->id, &ev.id,
> +		       sizeof(ev) - offsetof(struct v4l2_event, id));

Ditto.

> +
> +		return rval;
> +	}
> +
>  	case VIDIOC_SUBSCRIBE_EVENT:
>  		return v4l2_subdev_call(sd, core, subscribe_event, vfh, arg);
>  
> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> index 74d3d522f3db..1d2553d4ed5b 100644
> --- a/include/uapi/linux/videodev2.h
> +++ b/include/uapi/linux/videodev2.h
> @@ -2329,11 +2329,40 @@ struct v4l2_event {
>  	} u;
>  	__u32				pending;
>  	__u32				sequence;
> +#ifdef __KERNEL__
> +	struct __kernel_timespec	timestamp;
> +#else
>  	struct timespec			timestamp;
> +#endif
>  	__u32				id;
>  	__u32				reserved[8];
>  };
>  
> +#ifdef __KERNEL__
> +/*
> + * The user space interpretation of the 'v4l2_event' differs
> + * based on the 'time_t' definition on 32-bit architectures, so
> + * the kernel has to handle both.
> + * This is the old version for 32-bit architectures.
> + */
> +struct v4l2_event_time32 {
> +	__u32				type;
> +	union {
> +		struct v4l2_event_vsync		vsync;
> +		struct v4l2_event_ctrl		ctrl;
> +		struct v4l2_event_frame_sync	frame_sync;
> +		struct v4l2_event_src_change	src_change;
> +		struct v4l2_event_motion_det	motion_det;
> +		__u8				data[64];
> +	} u;
> +	__u32				pending;
> +	__u32				sequence;
> +	struct old_timespec32		timestamp;
> +	__u32				id;
> +	__u32				reserved[8];
> +};
> +#endif
> +
>  #define V4L2_EVENT_SUB_FL_SEND_INITIAL		(1 << 0)
>  #define V4L2_EVENT_SUB_FL_ALLOW_FEEDBACK	(1 << 1)
>  
> @@ -2486,6 +2515,7 @@ struct v4l2_create_buffers {
>  #define	VIDIOC_S_DV_TIMINGS	_IOWR('V', 87, struct v4l2_dv_timings)
>  #define	VIDIOC_G_DV_TIMINGS	_IOWR('V', 88, struct v4l2_dv_timings)
>  #define	VIDIOC_DQEVENT		 _IOR('V', 89, struct v4l2_event)
> +#define	VIDIOC_DQEVENT_TIME32	 _IOR('V', 89, struct v4l2_event_time32)

Shouldn't this be under #ifdef __KERNEL__?

And should this be in the public header at all? media/v4l2-ioctl.h might be a better
place.

Regards,

	Hans

>  #define	VIDIOC_SUBSCRIBE_EVENT	 _IOW('V', 90, struct v4l2_event_subscription)
>  #define	VIDIOC_UNSUBSCRIBE_EVENT _IOW('V', 91, struct v4l2_event_subscription)
>  #define VIDIOC_CREATE_BUFS	_IOWR('V', 92, struct v4l2_create_buffers)
> 

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v4 6/8] media: v4l2-core: fix v4l2_buffer handling for time64 ABI
  2019-11-11 20:38 ` [PATCH v4 6/8] media: v4l2-core: fix v4l2_buffer handling " Arnd Bergmann
@ 2019-11-25 14:57   ` Hans Verkuil
  2019-11-26 13:50     ` Arnd Bergmann
  0 siblings, 1 reply; 25+ messages in thread
From: Hans Verkuil @ 2019-11-25 14:57 UTC (permalink / raw)
  To: Arnd Bergmann, Mauro Carvalho Chehab, linux-media; +Cc: linux-kernel, y2038

On 11/11/19 9:38 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 | 57 ++++++++++++++++++++++++++--
>  include/uapi/linux/videodev2.h       | 45 ++++++++++++++++++++++
>  2 files changed, 98 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
> index 1de939d11628..4ae1bcaec3fa 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,
> @@ -3014,6 +3014,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
>  	}
>  
> @@ -3032,6 +3040,30 @@ 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;
> +
> +		memcpy(vb, &vb32, offsetof(struct v4l2_buffer, timestamp));
> +		vb->timestamp.tv_sec = vb32.timestamp.tv_sec;
> +		vb->timestamp.tv_usec = vb32.timestamp.tv_usec;
> +	        memcpy(&vb->timecode, &vb32.timecode,
> +		       sizeof(*vb) - offsetof(struct v4l2_buffer, timecode));

I have similar concerns as with dqevent about whether this memcpy is the right approach.
Unless you can prove with a utility like pahole that this memcpy is safe.

> +
> +		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,
> @@ -3080,6 +3112,23 @@ 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_time32 vb32;
> +		struct v4l2_buffer *vb = parg;
> +
> +		memcpy(&vb32, vb, offsetof(struct v4l2_buffer, timestamp));
> +		vb32.timestamp.tv_sec = vb->timestamp.tv_sec;
> +		vb32.timestamp.tv_usec = vb->timestamp.tv_usec;
> +	        memcpy(&vb32.timecode, &vb->timecode,
> +		       sizeof(*vb) - offsetof(struct v4l2_buffer, timecode));

Ditto.

> +
> +		if (copy_to_user(arg, &vb32, sizeof(vb32)))
> +			return -EFAULT;
> +		break;
> +	}
>  #endif
>  	default:
>  		/*  Copy results into user buffer  */
> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> index 1d2553d4ed5b..f05c54d63f96 100644
> --- a/include/uapi/linux/videodev2.h
> +++ b/include/uapi/linux/videodev2.h
> @@ -990,7 +990,47 @@ struct v4l2_buffer {
>  	__u32			bytesused;
>  	__u32			flags;
>  	__u32			field;
> +#ifdef __KERNEL__
> +	/* match glibc timeval64 format */
> +	struct {
> +		long long	tv_sec;
> +# if defined(__sparc__) && defined(__arch64__)
> +		int		tv_usec;
> +		int		__pad;
> +# else
> +		long long	tv_usec;
> +# endif
> +	} timestamp;

Ewww!

Are there more places where this is needed? If so, then I very much prefer
that a __kernel_timeval struct is defined somewhere, with appropriate
comments.

> +#else
>  	struct timeval		timestamp;
> +#endif
> +	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;
> +	};
> +};
> +
> +#ifdef __KERNEL__
> +struct v4l2_buffer_time32 {
> +	__u32			index;
> +	__u32			type;
> +	__u32			bytesused;
> +	__u32			flags;
> +	__u32			field;
> +	struct old_timeval32	timestamp;
>  	struct v4l2_timecode	timecode;
>  	__u32			sequence;
>  
> @@ -1009,6 +1049,7 @@ struct v4l2_buffer {
>  		__u32		reserved;
>  	};
>  };
> +#endif

Can this be moved to v4l2-ioctls.h?

>  
>  #ifndef __KERNEL__
>  /**
> @@ -2446,12 +2487,15 @@ struct v4l2_create_buffers {
>  #define VIDIOC_S_FMT		_IOWR('V',  5, struct v4l2_format)
>  #define VIDIOC_REQBUFS		_IOWR('V',  8, struct v4l2_requestbuffers)
>  #define VIDIOC_QUERYBUF		_IOWR('V',  9, struct v4l2_buffer)
> +#define VIDIOC_QUERYBUF_TIME32	_IOWR('V',  9, struct v4l2_buffer_time32)

And all these should be moved there as well.

>  #define VIDIOC_G_FBUF		 _IOR('V', 10, struct v4l2_framebuffer)
>  #define VIDIOC_S_FBUF		 _IOW('V', 11, struct v4l2_framebuffer)
>  #define VIDIOC_OVERLAY		 _IOW('V', 14, int)
>  #define VIDIOC_QBUF		_IOWR('V', 15, struct v4l2_buffer)
> +#define VIDIOC_QBUF_TIME32	_IOWR('V', 15, struct v4l2_buffer_time32)
>  #define VIDIOC_EXPBUF		_IOWR('V', 16, struct v4l2_exportbuffer)
>  #define VIDIOC_DQBUF		_IOWR('V', 17, struct v4l2_buffer)
> +#define VIDIOC_DQBUF_TIME32	_IOWR('V', 17, struct v4l2_buffer_time32)
>  #define VIDIOC_STREAMON		 _IOW('V', 18, int)
>  #define VIDIOC_STREAMOFF	 _IOW('V', 19, int)
>  #define VIDIOC_G_PARM		_IOWR('V', 21, struct v4l2_streamparm)
> @@ -2520,6 +2564,7 @@ struct v4l2_create_buffers {
>  #define	VIDIOC_UNSUBSCRIBE_EVENT _IOW('V', 91, struct v4l2_event_subscription)
>  #define VIDIOC_CREATE_BUFS	_IOWR('V', 92, struct v4l2_create_buffers)
>  #define VIDIOC_PREPARE_BUF	_IOWR('V', 93, struct v4l2_buffer)
> +#define VIDIOC_PREPARE_BUF_TIME32 _IOWR('V', 93, struct v4l2_buffer_time32)
>  #define VIDIOC_G_SELECTION	_IOWR('V', 94, struct v4l2_selection)
>  #define VIDIOC_S_SELECTION	_IOWR('V', 95, struct v4l2_selection)
>  #define VIDIOC_DECODER_CMD	_IOWR('V', 96, struct v4l2_decoder_cmd)
> 

Regards,

	Hans

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v4 2/8] media: v4l2: abstract timeval handling in v4l2_buffer
  2019-11-11 20:38 ` [PATCH v4 2/8] media: v4l2: abstract timeval handling in v4l2_buffer Arnd Bergmann
@ 2019-11-25 15:52   ` Hans Verkuil
  2019-11-26 11:34     ` Arnd Bergmann
  0 siblings, 1 reply; 25+ messages in thread
From: Hans Verkuil @ 2019-11-25 15:52 UTC (permalink / raw)
  To: Arnd Bergmann, Mauro Carvalho Chehab, linux-media; +Cc: linux-kernel, y2038

On 11/11/19 9:38 PM, Arnd Bergmann wrote:
> As a preparation for adding 64-bit time_t support in the uapi,
> change the drivers to no longer care about the format of the
> timestamp field in struct v4l2_buffer.
> 
> The v4l2_timeval_to_ns() function is no longer needed in the
> kernel after this, but there may be userspace code relying on
> it because it is part of the uapi header.

There is indeed userspace code that relies on this.

> 
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>  drivers/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/videobuf-core.c         |  4 ++--
>  include/linux/videodev2.h                       | 17 ++++++++++++++++-
>  include/trace/events/v4l2.h                     |  2 +-
>  include/uapi/linux/videodev2.h                  |  2 ++
>  9 files changed, 30 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c
> index 5a9ba3846f0a..9ec710878db6 100644
> --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
> +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
> @@ -143,7 +143,7 @@ static void __copy_timestamp(struct vb2_buffer *vb, const void *pb)
>  		 * and the timecode field and flag if needed.
>  		 */
>  		if (q->copy_timestamp)
> -			vb->timestamp = v4l2_timeval_to_ns(&b->timestamp);
> +			vb->timestamp = v4l2_buffer_get_timestamp(b);
>  		vbuf->flags |= b->flags & V4L2_BUF_FLAG_TIMECODE;
>  		if (b->flags & V4L2_BUF_FLAG_TIMECODE)
>  			vbuf->timecode = b->timecode;
> @@ -476,7 +476,7 @@ static void __fill_v4l2_buffer(struct vb2_buffer *vb, void *pb)
>  
>  	b->flags = vbuf->flags;
>  	b->field = vbuf->field;
> -	b->timestamp = ns_to_timeval(vb->timestamp);
> +	v4l2_buffer_set_timestamp(b, vb->timestamp);
>  	b->timecode = vbuf->timecode;
>  	b->sequence = vbuf->sequence;
>  	b->reserved2 = 0;
> diff --git a/drivers/media/pci/meye/meye.c b/drivers/media/pci/meye/meye.c
> index 0e61c81356ef..3a4c29bc0ba5 100644
> --- a/drivers/media/pci/meye/meye.c
> +++ b/drivers/media/pci/meye/meye.c
> @@ -1266,7 +1266,7 @@ static int vidioc_querybuf(struct file *file, void *fh, struct v4l2_buffer *buf)
>  		buf->flags |= V4L2_BUF_FLAG_DONE;
>  
>  	buf->field = V4L2_FIELD_NONE;
> -	buf->timestamp = ns_to_timeval(meye.grab_buffer[index].ts);
> +	v4l2_buffer_set_timestamp(buf, meye.grab_buffer[index].ts);
>  	buf->sequence = meye.grab_buffer[index].sequence;
>  	buf->memory = V4L2_MEMORY_MMAP;
>  	buf->m.offset = index * gbufsize;
> @@ -1332,7 +1332,7 @@ static int vidioc_dqbuf(struct file *file, void *fh, struct v4l2_buffer *buf)
>  	buf->bytesused = meye.grab_buffer[reqnr].size;
>  	buf->flags = V4L2_BUF_FLAG_MAPPED | V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
>  	buf->field = V4L2_FIELD_NONE;
> -	buf->timestamp = ns_to_timeval(meye.grab_buffer[reqnr].ts);
> +	v4l2_buffer_set_timestamp(buf, meye.grab_buffer[reqnr].ts);
>  	buf->sequence = meye.grab_buffer[reqnr].sequence;
>  	buf->memory = V4L2_MEMORY_MMAP;
>  	buf->m.offset = reqnr * gbufsize;
> diff --git a/drivers/media/usb/cpia2/cpia2_v4l.c b/drivers/media/usb/cpia2/cpia2_v4l.c
> index 626264a56517..9d3d05125d7b 100644
> --- a/drivers/media/usb/cpia2/cpia2_v4l.c
> +++ b/drivers/media/usb/cpia2/cpia2_v4l.c
> @@ -800,7 +800,7 @@ static int cpia2_querybuf(struct file *file, void *fh, struct v4l2_buffer *buf)
>  		break;
>  	case FRAME_READY:
>  		buf->bytesused = cam->buffers[buf->index].length;
> -		buf->timestamp = ns_to_timeval(cam->buffers[buf->index].ts);
> +		v4l2_buffer_set_timestamp(buf, cam->buffers[buf->index].ts);
>  		buf->sequence = cam->buffers[buf->index].seq;
>  		buf->flags = V4L2_BUF_FLAG_DONE;
>  		break;
> @@ -907,7 +907,7 @@ static int cpia2_dqbuf(struct file *file, void *fh, struct v4l2_buffer *buf)
>  	buf->flags = V4L2_BUF_FLAG_MAPPED | V4L2_BUF_FLAG_DONE
>  		| V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
>  	buf->field = V4L2_FIELD_NONE;
> -	buf->timestamp = ns_to_timeval(cam->buffers[buf->index].ts);
> +	v4l2_buffer_set_timestamp(buf, cam->buffers[buf->index].ts);
>  	buf->sequence = cam->buffers[buf->index].seq;
>  	buf->m.offset = cam->buffers[buf->index].data - cam->frame_buffer;
>  	buf->length = cam->frame_size;
> diff --git a/drivers/media/usb/stkwebcam/stk-webcam.c b/drivers/media/usb/stkwebcam/stk-webcam.c
> index 21f90a887485..b22501f76b78 100644
> --- a/drivers/media/usb/stkwebcam/stk-webcam.c
> +++ b/drivers/media/usb/stkwebcam/stk-webcam.c
> @@ -1125,7 +1125,7 @@ static int stk_vidioc_dqbuf(struct file *filp,
>  	sbuf->v4lbuf.flags &= ~V4L2_BUF_FLAG_QUEUED;
>  	sbuf->v4lbuf.flags |= V4L2_BUF_FLAG_DONE;
>  	sbuf->v4lbuf.sequence = ++dev->sequence;
> -	sbuf->v4lbuf.timestamp = ns_to_timeval(ktime_get_ns());
> +	v4l2_buffer_set_timestamp(&sbuf->v4lbuf, ktime_get_ns());
>  
>  	*buf = sbuf->v4lbuf;
>  	return 0;
> diff --git a/drivers/media/usb/usbvision/usbvision-video.c b/drivers/media/usb/usbvision/usbvision-video.c
> index cdc66adda755..15a423c5deb7 100644
> --- a/drivers/media/usb/usbvision/usbvision-video.c
> +++ b/drivers/media/usb/usbvision/usbvision-video.c
> @@ -687,7 +687,7 @@ static int vidioc_querybuf(struct file *file,
>  	vb->length = usbvision->curwidth *
>  		usbvision->curheight *
>  		usbvision->palette.bytes_per_pixel;
> -	vb->timestamp = ns_to_timeval(usbvision->frame[vb->index].ts);
> +	v4l2_buffer_set_timestamp(vb, usbvision->frame[vb->index].ts);
>  	vb->sequence = usbvision->frame[vb->index].sequence;
>  	return 0;
>  }
> @@ -756,7 +756,7 @@ static int vidioc_dqbuf(struct file *file, void *priv, struct v4l2_buffer *vb)
>  		V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
>  	vb->index = f->index;
>  	vb->sequence = f->sequence;
> -	vb->timestamp = ns_to_timeval(f->ts);
> +	v4l2_buffer_set_timestamp(vb, f->ts);
>  	vb->field = V4L2_FIELD_NONE;
>  	vb->bytesused = f->scanlength;
>  
> diff --git a/drivers/media/v4l2-core/videobuf-core.c b/drivers/media/v4l2-core/videobuf-core.c
> index 939fc11cf080..ab650371c151 100644
> --- a/drivers/media/v4l2-core/videobuf-core.c
> +++ b/drivers/media/v4l2-core/videobuf-core.c
> @@ -364,7 +364,7 @@ static void videobuf_status(struct videobuf_queue *q, struct v4l2_buffer *b,
>  	}
>  
>  	b->field     = vb->field;
> -	b->timestamp = ns_to_timeval(vb->ts);
> +	v4l2_buffer_set_timestamp(b, vb->ts);
>  	b->bytesused = vb->size;
>  	b->sequence  = vb->field_count >> 1;
>  }
> @@ -578,7 +578,7 @@ int videobuf_qbuf(struct videobuf_queue *q, struct v4l2_buffer *b)
>  		    || q->type == V4L2_BUF_TYPE_SDR_OUTPUT) {
>  			buf->size = b->bytesused;
>  			buf->field = b->field;
> -			buf->ts = v4l2_timeval_to_ns(&b->timestamp);
> +			buf->ts = v4l2_buffer_get_timestamp(b);
>  		}
>  		break;
>  	case V4L2_MEMORY_USERPTR:
> diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h
> index 16c0ed6c50a7..4086036e37d5 100644
> --- a/include/linux/videodev2.h
> +++ b/include/linux/videodev2.h
> @@ -56,7 +56,22 @@
>  #ifndef __LINUX_VIDEODEV2_H
>  #define __LINUX_VIDEODEV2_H
>  
> -#include <linux/time.h>     /* need struct timeval */
> +#include <linux/time.h>
>  #include <uapi/linux/videodev2.h>
>  
> +static inline u64 v4l2_buffer_get_timestamp(const struct v4l2_buffer *buf)
> +{
> +	return buf->timestamp.tv_sec * NSEC_PER_SEC +
> +	       (u32)buf->timestamp.tv_usec * NSEC_PER_USEC;

Why the (u32) cast?

> +}
> +
> +static inline void v4l2_buffer_set_timestamp(struct v4l2_buffer *buf,
> +					     u64 timestamp)
> +{
> +	struct timespec64 ts = ns_to_timespec64(timestamp);
> +
> +	buf->timestamp.tv_sec  = ts.tv_sec;
> +	buf->timestamp.tv_usec = ts.tv_nsec / NSEC_PER_USEC;
> +}
> +

This does not belong in the public header. This is kernel specific,
so media/v4l2-common.h would be a good place.

Regards,

	Hans

>  #endif /* __LINUX_VIDEODEV2_H */
> diff --git a/include/trace/events/v4l2.h b/include/trace/events/v4l2.h
> index 83860de120e3..248bc09bfc99 100644
> --- a/include/trace/events/v4l2.h
> +++ b/include/trace/events/v4l2.h
> @@ -130,7 +130,7 @@ DECLARE_EVENT_CLASS(v4l2_event_class,
>  		__entry->bytesused = buf->bytesused;
>  		__entry->flags = buf->flags;
>  		__entry->field = buf->field;
> -		__entry->timestamp = timeval_to_ns(&buf->timestamp);
> +		__entry->timestamp = v4l2_buffer_get_timestamp(buf);
>  		__entry->timecode_type = buf->timecode.type;
>  		__entry->timecode_flags = buf->timecode.flags;
>  		__entry->timecode_frames = buf->timecode.frames;
> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> index 530638dffd93..74d3d522f3db 100644
> --- a/include/uapi/linux/videodev2.h
> +++ b/include/uapi/linux/videodev2.h
> @@ -1010,6 +1010,7 @@ struct v4l2_buffer {
>  	};
>  };
>  
> +#ifndef __KERNEL__
>  /**
>   * v4l2_timeval_to_ns - Convert timeval to nanoseconds
>   * @ts:		pointer to the timeval variable to be converted
> @@ -1021,6 +1022,7 @@ static inline __u64 v4l2_timeval_to_ns(const struct timeval *tv)
>  {
>  	return (__u64)tv->tv_sec * 1000000000ULL + tv->tv_usec * 1000;
>  }
> +#endif
>  
>  /*  Flags for 'flags' field */
>  /* Buffer is mapped (flag) */
> 


^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v4 0/8] y2038 safety in v4l2
  2019-11-11 20:38 [PATCH v4 0/8] y2038 safety in v4l2 Arnd Bergmann
                   ` (7 preceding siblings ...)
  2019-11-11 20:38 ` [PATCH v4 8/8] media: v4l2-core: fix compat v4l2_buffer handling " Arnd Bergmann
@ 2019-11-25 16:02 ` Hans Verkuil
  2019-11-26 11:13   ` Arnd Bergmann
  8 siblings, 1 reply; 25+ messages in thread
From: Hans Verkuil @ 2019-11-25 16:02 UTC (permalink / raw)
  To: Arnd Bergmann, Mauro Carvalho Chehab, linux-media; +Cc: linux-kernel, y2038

Hi Arnd,

On 11/11/19 9:38 PM, Arnd Bergmann wrote:
> 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.

Thank you for working on this. Much appreciated!

I've reviewed this v4 series and found a few issues (mostly things
that ended up in videodev2.h that shouldn't be there).

Once a v5 is posted I'll try to test this more.

Is there an easy(-ish) way to test this with a time64 ABI? Do you
have such an environment set up for testing?

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

A cleanup is indeed much more work. This y2038 code is awful, but that's
really because the original structs are aligned in the worst possible
way.

Regards,

	Hans

> 
> 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. I updated the tree at [2]
> to this version now.
> 
>       Arnd
> 
> [1] https://lwn.net/Articles/657754/
> [2] https://git.kernel.org/pub/scm/linux/kernel/git/arnd/playground.git/log/?h=y2038-v4l2
> 
> 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          | 188 +++++--
>  drivers/media/v4l2-core/v4l2-subdev.c         |  20 +-
>  drivers/media/v4l2-core/videobuf-core.c       |   4 +-
>  include/linux/videodev2.h                     |  17 +-
>  include/trace/events/v4l2.h                   |   2 +-
>  include/uapi/linux/videodev2.h                |  77 +++
>  15 files changed, 669 insertions(+), 136 deletions(-)
> 


^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v4 0/8] y2038 safety in v4l2
  2019-11-25 16:02 ` [PATCH v4 0/8] y2038 safety in v4l2 Hans Verkuil
@ 2019-11-26 11:13   ` Arnd Bergmann
  0 siblings, 0 replies; 25+ messages in thread
From: Arnd Bergmann @ 2019-11-26 11:13 UTC (permalink / raw)
  To: Hans Verkuil
  Cc: Mauro Carvalho Chehab, Linux Media Mailing List, linux-kernel,
	y2038 Mailman List

On Mon, Nov 25, 2019 at 5:02 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>
> Hi Arnd,
>
> On 11/11/19 9:38 PM, Arnd Bergmann wrote:
> > 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.
>
> Thank you for working on this. Much appreciated!
>
> I've reviewed this v4 series and found a few issues (mostly things
> that ended up in videodev2.h that shouldn't be there).
>
> Once a v5 is posted I'll try to test this more.

Ok, great!

>
> Is there an easy(-ish) way to test this with a time64 ABI? Do you
> have such an environment set up for testing?

If you can build your user space with musl, you can test using
a recent snapshot from http://git.musl-libc.org/cgit/musl/.

> > 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.
>
> A cleanup is indeed much more work. This y2038 code is awful, but that's
> really because the original structs are aligned in the worst possible
> way.

Ok.

         Arnd

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v4 2/8] media: v4l2: abstract timeval handling in v4l2_buffer
  2019-11-25 15:52   ` Hans Verkuil
@ 2019-11-26 11:34     ` Arnd Bergmann
  2019-11-26 11:43       ` Hans Verkuil
  0 siblings, 1 reply; 25+ messages in thread
From: Arnd Bergmann @ 2019-11-26 11:34 UTC (permalink / raw)
  To: Hans Verkuil
  Cc: Mauro Carvalho Chehab, Linux Media Mailing List, linux-kernel,
	y2038 Mailman List

On Mon, Nov 25, 2019 at 4:52 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>
> On 11/11/19 9:38 PM, Arnd Bergmann wrote:
> > As a preparation for adding 64-bit time_t support in the uapi,
> > change the drivers to no longer care about the format of the
> > timestamp field in struct v4l2_buffer.
> >
> > The v4l2_timeval_to_ns() function is no longer needed in the
> > kernel after this, but there may be userspace code relying on
> > it because it is part of the uapi header.
>
> There is indeed userspace code that relies on this.

Ok, good to know. I rephrased the changelog text as

The v4l2_timeval_to_ns() function is no longer needed in the
kernel after this, but there is userspace code relying on
it to be part of the uapi header.

> >
> > +static inline u64 v4l2_buffer_get_timestamp(const struct v4l2_buffer *buf)
> > +{
> > +     return buf->timestamp.tv_sec * NSEC_PER_SEC +
> > +            (u32)buf->timestamp.tv_usec * NSEC_PER_USEC;
>
> Why the (u32) cast?

Simple question, long answer:

on 32-bit architectures, the tv_usec member may be 32-bit wide plus
padding in user space when interpreted as a regular 'struct timeval',
but the kernel implementation now sees it as a 64-bit member,
with half of it being possibly uninitialized user space data.

The 32-bit cast avoids that uninitialized data and ensures user space
passing garbage in the upper half gets ignored, as it has to be on 32-bit
user space.

On 64-bit native user space, the tv_usec field is always 64 bit wide,
so this is a change in behavior for denormalized timeval data
with tv_usec > U32_MAX, but the current behavior does not appear
worth preserving either.

The correct way would probably be to return an error for
 tv_usec >USEC_PER_SEC, but as the code never did that, this
would risk a regression for user space that relies on passing
invalid timestamps without getting an error.

> > +static inline void v4l2_buffer_set_timestamp(struct v4l2_buffer *buf,
> > +                                          u64 timestamp)
> > +{
> > +     struct timespec64 ts = ns_to_timespec64(timestamp);
> > +
> > +     buf->timestamp.tv_sec  = ts.tv_sec;
> > +     buf->timestamp.tv_usec = ts.tv_nsec / NSEC_PER_USEC;
> > +}
> > +
>
> This does not belong in the public header. This is kernel specific,

Note: this is not the uapi header but the in-kernel one.

> so media/v4l2-common.h would be a good place.

Ok, sounds good. I wasn't sure where to put it, and ended up
with include/linux/videodev2.h as the best replacement for
include/uapi/linux/videodev2.h, changed it to
include/media/v4l2-common.h now.

       Arnd

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v4 2/8] media: v4l2: abstract timeval handling in v4l2_buffer
  2019-11-26 11:34     ` Arnd Bergmann
@ 2019-11-26 11:43       ` Hans Verkuil
  2019-11-26 12:42         ` Arnd Bergmann
  0 siblings, 1 reply; 25+ messages in thread
From: Hans Verkuil @ 2019-11-26 11:43 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Mauro Carvalho Chehab, Linux Media Mailing List, linux-kernel,
	y2038 Mailman List

On 11/26/19 12:34 PM, Arnd Bergmann wrote:
> On Mon, Nov 25, 2019 at 4:52 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>>
>> On 11/11/19 9:38 PM, Arnd Bergmann wrote:
>>> As a preparation for adding 64-bit time_t support in the uapi,
>>> change the drivers to no longer care about the format of the
>>> timestamp field in struct v4l2_buffer.
>>>
>>> The v4l2_timeval_to_ns() function is no longer needed in the
>>> kernel after this, but there may be userspace code relying on
>>> it because it is part of the uapi header.
>>
>> There is indeed userspace code that relies on this.
> 
> Ok, good to know. I rephrased the changelog text as
> 
> The v4l2_timeval_to_ns() function is no longer needed in the
> kernel after this, but there is userspace code relying on
> it to be part of the uapi header.
> 
>>>
>>> +static inline u64 v4l2_buffer_get_timestamp(const struct v4l2_buffer *buf)
>>> +{
>>> +     return buf->timestamp.tv_sec * NSEC_PER_SEC +
>>> +            (u32)buf->timestamp.tv_usec * NSEC_PER_USEC;
>>
>> Why the (u32) cast?
> 
> Simple question, long answer:
> 
> on 32-bit architectures, the tv_usec member may be 32-bit wide plus
> padding in user space when interpreted as a regular 'struct timeval',
> but the kernel implementation now sees it as a 64-bit member,
> with half of it being possibly uninitialized user space data.
> 
> The 32-bit cast avoids that uninitialized data and ensures user space
> passing garbage in the upper half gets ignored, as it has to be on 32-bit
> user space.

But that's only valid for little endian 32 bit systems, right?
Is this only an issue for x86 platforms?

> 
> On 64-bit native user space, the tv_usec field is always 64 bit wide,
> so this is a change in behavior for denormalized timeval data
> with tv_usec > U32_MAX, but the current behavior does not appear
> worth preserving either.
> 
> The correct way would probably be to return an error for
>  tv_usec >USEC_PER_SEC, but as the code never did that, this
> would risk a regression for user space that relies on passing
> invalid timestamps without getting an error.

This long answer needs to be added to a comment to that function.
Because otherwise someone will come along later and remove that
seemingly unnecessary cast.

It's OK if it is a long comment, it's a non-trivial reason.

> 
>>> +static inline void v4l2_buffer_set_timestamp(struct v4l2_buffer *buf,
>>> +                                          u64 timestamp)
>>> +{
>>> +     struct timespec64 ts = ns_to_timespec64(timestamp);
>>> +
>>> +     buf->timestamp.tv_sec  = ts.tv_sec;
>>> +     buf->timestamp.tv_usec = ts.tv_nsec / NSEC_PER_USEC;
>>> +}
>>> +
>>
>> This does not belong in the public header. This is kernel specific,
> 
> Note: this is not the uapi header but the in-kernel one.

Ah, I missed that.

> 
>> so media/v4l2-common.h would be a good place.
> 
> Ok, sounds good. I wasn't sure where to put it, and ended up
> with include/linux/videodev2.h as the best replacement for
> include/uapi/linux/videodev2.h, changed it to
> include/media/v4l2-common.h now.

Never use include/linux/videodev2.h. It's just a wrapper around
the uapi header and should not contain any 'real' code.

It's also why I missed that you modified that header since we never
touch it.

Regards,

	Hans

> 
>        Arnd
> 


^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v4 2/8] media: v4l2: abstract timeval handling in v4l2_buffer
  2019-11-26 11:43       ` Hans Verkuil
@ 2019-11-26 12:42         ` Arnd Bergmann
  0 siblings, 0 replies; 25+ messages in thread
From: Arnd Bergmann @ 2019-11-26 12:42 UTC (permalink / raw)
  To: Hans Verkuil
  Cc: Mauro Carvalho Chehab, Linux Media Mailing List, linux-kernel,
	y2038 Mailman List

On Tue, Nov 26, 2019 at 12:43 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
> On 11/26/19 12:34 PM, Arnd Bergmann wrote:
> > On Mon, Nov 25, 2019 at 4:52 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
> >>>
> >>> +static inline u64 v4l2_buffer_get_timestamp(const struct v4l2_buffer *buf)
> >>> +{
> >>> +     return buf->timestamp.tv_sec * NSEC_PER_SEC +
> >>> +            (u32)buf->timestamp.tv_usec * NSEC_PER_USEC;
> >>
> >> Why the (u32) cast?
> >
> > Simple question, long answer:
> >
> > on 32-bit architectures, the tv_usec member may be 32-bit wide plus
> > padding in user space when interpreted as a regular 'struct timeval',
> > but the kernel implementation now sees it as a 64-bit member,
> > with half of it being possibly uninitialized user space data.
> >
> > The 32-bit cast avoids that uninitialized data and ensures user space
> > passing garbage in the upper half gets ignored, as it has to be on 32-bit
> > user space.
>
> But that's only valid for little endian 32 bit systems, right?
> Is this only an issue for x86 platforms?

Uninitialized data is an issue on all 32-bit architectures. The layout
of the new timeval is such that the low 32 bits of tv_sec are in the
same place on both 32-bit and 64-bit architectures of the same
endianess, but if an application initializes the fields individually
without a memset before it, it may still pass invalid data.

> > On 64-bit native user space, the tv_usec field is always 64 bit wide,
> > so this is a change in behavior for denormalized timeval data
> > with tv_usec > U32_MAX, but the current behavior does not appear
> > worth preserving either.
> >
> > The correct way would probably be to return an error for
> >  tv_usec >USEC_PER_SEC, but as the code never did that, this
> > would risk a regression for user space that relies on passing
> > invalid timestamps without getting an error.
>
> This long answer needs to be added to a comment to that function.
> Because otherwise someone will come along later and remove that
> seemingly unnecessary cast.
>
> It's OK if it is a long comment, it's a non-trivial reason.

Added this comment now:

        /*
         * When the timestamp comes from 32-bit user space, there may be
         * uninitialized data in tv_usec, so cast it to u32.
         * Otherwise allow invalid input for backwards compatibility.
         */

Let me know if you prefer a more elaborate version.

> >> so media/v4l2-common.h would be a good place.
> >
> > Ok, sounds good. I wasn't sure where to put it, and ended up
> > with include/linux/videodev2.h as the best replacement for
> > include/uapi/linux/videodev2.h, changed it to
> > include/media/v4l2-common.h now.
>
> Never use include/linux/videodev2.h. It's just a wrapper around
> the uapi header and should not contain any 'real' code.
>
> It's also why I missed that you modified that header since we never
> touch it.

Ok, got it. I now tried to remove this file completely, hoping that the
include <linux/time.h> is no longer needed after my series, but
it seems we still need it.

       Arnd

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v4 6/8] media: v4l2-core: fix v4l2_buffer handling for time64 ABI
  2019-11-25 14:57   ` Hans Verkuil
@ 2019-11-26 13:50     ` Arnd Bergmann
  2019-11-26 14:15       ` Hans Verkuil
  0 siblings, 1 reply; 25+ messages in thread
From: Arnd Bergmann @ 2019-11-26 13:50 UTC (permalink / raw)
  To: Hans Verkuil
  Cc: Mauro Carvalho Chehab, Linux Media Mailing List, linux-kernel,
	y2038 Mailman List

On Mon, Nov 25, 2019 at 3:57 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
> On 11/11/19 9:38 PM, Arnd Bergmann wrote:

> >       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;
> > +
> > +             memcpy(vb, &vb32, offsetof(struct v4l2_buffer, timestamp));
> > +             vb->timestamp.tv_sec = vb32.timestamp.tv_sec;
> > +             vb->timestamp.tv_usec = vb32.timestamp.tv_usec;
> > +             memcpy(&vb->timecode, &vb32.timecode,
> > +                    sizeof(*vb) - offsetof(struct v4l2_buffer, timecode));
>
> I have similar concerns as with dqevent about whether this memcpy is the right approach.
> Unless you can prove with a utility like pahole that this memcpy is safe.

This is the video_get_user() function, so the input data comes from user
space and gets copied into the kernel, which has to check each field for
validity already, so I think this is safe regardless of the padding (which
exists before the 64-bit timestamp on 32-bit architectures). The fields
match because the definition of all members other than the timeval is
the same.

On the other hand, I agree it's not obvious from the code why this
is correct. I've changed my copy to this version below now, do you like
that better?

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

This way, all padding is zeroed out, and it's obvious to human
readers that each field gets set in the correct location.

> > +             memcpy(&vb32, vb, offsetof(struct v4l2_buffer, timestamp));
> > +             vb32.timestamp.tv_sec = vb->timestamp.tv_sec;
> > +             vb32.timestamp.tv_usec = vb->timestamp.tv_usec;
> > +             memcpy(&vb32.timecode, &vb->timecode,
> > +                    sizeof(*vb) - offsetof(struct v4l2_buffer, timecode));
>
> Ditto.

This is my new version:

                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->usercopy,
                        .length         = vb->length,
                        .request_fd     = vb->request_fd,
                };

                if (copy_to_user(arg, &vb32, sizeof(vb32)))
                        return -EFAULT;

> >       __u32                   field;
> > +#ifdef __KERNEL__
> > +     /* match glibc timeval64 format */
> > +     struct {
> > +             long long       tv_sec;
> > +# if defined(__sparc__) && defined(__arch64__)
> > +             int             tv_usec;
> > +             int             __pad;
> > +# else
> > +             long long       tv_usec;
> > +# endif
> > +     } timestamp;
>
> Ewww!
>
> Are there more places where this is needed? If so, then I very much prefer
> that a __kernel_timeval struct is defined somewhere, with appropriate
> comments.

I was trying hard to avoid adding a modern version of timeval, because
all new code should be encouraged to use __kernel_timespec instead.

There are not many users of timeval in the uapi, and this is the last one
after the others all got invididual treatment.

Usually what I would do is to have a kernel-internal type based
on timespec or u64, and then define three uapi types:
old native (based on __kernel_old_timeval), old compat (using
old_timeval32) and the new type with 64-bit time_t.

The problem with v4l2_buffer is that it includes another
compat-incompatible field (m.userptr) and that it's passed
between kernel functions, so then I'd probably need five variants
of it in total, and it would slow down the common case (64-bit
native) because it would require an extra copy.

I can try a few more things here, but I don't expect to find anything
much better than this.

> > +#ifdef __KERNEL__
> > +struct v4l2_buffer_time32 {
> > +     __u32                   index;
> > +     __u32                   type;
> > +     __u32                   bytesused;
> > +     __u32                   flags;
> > +     __u32                   field;
> > +     struct old_timeval32    timestamp;
> >       struct v4l2_timecode    timecode;
> >       __u32                   sequence;
> >
> > @@ -1009,6 +1049,7 @@ struct v4l2_buffer {
> >               __u32           reserved;
> >       };
> >  };
> > +#endif
>
> Can this be moved to v4l2-ioctls.h?

done.

> >  #ifndef __KERNEL__
> >  /**
> > @@ -2446,12 +2487,15 @@ struct v4l2_create_buffers {
> >  #define VIDIOC_S_FMT         _IOWR('V',  5, struct v4l2_format)
> >  #define VIDIOC_REQBUFS               _IOWR('V',  8, struct v4l2_requestbuffers)
> >  #define VIDIOC_QUERYBUF              _IOWR('V',  9, struct v4l2_buffer)
> > +#define VIDIOC_QUERYBUF_TIME32       _IOWR('V',  9, struct v4l2_buffer_time32)
>
> And all these should be moved there as well.

done.

      Arnd

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v4 6/8] media: v4l2-core: fix v4l2_buffer handling for time64 ABI
  2019-11-26 13:50     ` Arnd Bergmann
@ 2019-11-26 14:15       ` Hans Verkuil
  2019-11-26 15:17         ` Arnd Bergmann
  0 siblings, 1 reply; 25+ messages in thread
From: Hans Verkuil @ 2019-11-26 14:15 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Mauro Carvalho Chehab, Linux Media Mailing List, linux-kernel,
	y2038 Mailman List

On 11/26/19 2:50 PM, Arnd Bergmann wrote:
> On Mon, Nov 25, 2019 at 3:57 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>> On 11/11/19 9:38 PM, Arnd Bergmann wrote:
> 
>>>       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;
>>> +
>>> +             memcpy(vb, &vb32, offsetof(struct v4l2_buffer, timestamp));
>>> +             vb->timestamp.tv_sec = vb32.timestamp.tv_sec;
>>> +             vb->timestamp.tv_usec = vb32.timestamp.tv_usec;
>>> +             memcpy(&vb->timecode, &vb32.timecode,
>>> +                    sizeof(*vb) - offsetof(struct v4l2_buffer, timecode));
>>
>> I have similar concerns as with dqevent about whether this memcpy is the right approach.
>> Unless you can prove with a utility like pahole that this memcpy is safe.
> 
> This is the video_get_user() function, so the input data comes from user
> space and gets copied into the kernel, which has to check each field for
> validity already, so I think this is safe regardless of the padding (which
> exists before the 64-bit timestamp on 32-bit architectures). The fields
> match because the definition of all members other than the timeval is
> the same.
> 
> On the other hand, I agree it's not obvious from the code why this
> is correct. I've changed my copy to this version below now, do you like
> that better?
> 
>                 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.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));
> 
> This way, all padding is zeroed out, and it's obvious to human
> readers that each field gets set in the correct location.
> 
>>> +             memcpy(&vb32, vb, offsetof(struct v4l2_buffer, timestamp));
>>> +             vb32.timestamp.tv_sec = vb->timestamp.tv_sec;
>>> +             vb32.timestamp.tv_usec = vb->timestamp.tv_usec;
>>> +             memcpy(&vb32.timecode, &vb->timecode,
>>> +                    sizeof(*vb) - offsetof(struct v4l2_buffer, timecode));
>>
>> Ditto.
> 
> This is my new version:
> 
>                 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->usercopy,
>                         .length         = vb->length,
>                         .request_fd     = vb->request_fd,
>                 };

That looks clean.

> 
>                 if (copy_to_user(arg, &vb32, sizeof(vb32)))
>                         return -EFAULT;
> 
>>>       __u32                   field;
>>> +#ifdef __KERNEL__
>>> +     /* match glibc timeval64 format */
>>> +     struct {
>>> +             long long       tv_sec;
>>> +# if defined(__sparc__) && defined(__arch64__)
>>> +             int             tv_usec;
>>> +             int             __pad;
>>> +# else
>>> +             long long       tv_usec;
>>> +# endif
>>> +     } timestamp;
>>
>> Ewww!
>>
>> Are there more places where this is needed? If so, then I very much prefer
>> that a __kernel_timeval struct is defined somewhere, with appropriate
>> comments.
> 
> I was trying hard to avoid adding a modern version of timeval, because
> all new code should be encouraged to use __kernel_timespec instead.
> 
> There are not many users of timeval in the uapi, and this is the last one
> after the others all got invididual treatment.
> 
> Usually what I would do is to have a kernel-internal type based
> on timespec or u64, and then define three uapi types:
> old native (based on __kernel_old_timeval), old compat (using
> old_timeval32) and the new type with 64-bit time_t.
> 
> The problem with v4l2_buffer is that it includes another
> compat-incompatible field (m.userptr) and that it's passed
> between kernel functions, so then I'd probably need five variants
> of it in total, and it would slow down the common case (64-bit
> native) because it would require an extra copy.
> 
> I can try a few more things here, but I don't expect to find anything
> much better than this.

How about something like this in videodev2.h:

Split off the ugly kernel timeval definition in a separate struct:

#ifdef __KERNEL__
	/* match glibc timeval64 format */
	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

Then use that in the struct v4l2_buffer definition:

struct v4l2_buffer {
...
#ifdef __KERNEL__
 	struct __kernel_v4l2_timeval timestamp;
#else
 	struct timeval		     timestamp;
#endif

That keeps struct v4l2_buffer fairly clean. And it also makes it
possible to have a bit more extensive documentation for the
struct __kernel_v4l2_timeval without polluting the actual struct
v4l2_buffer definition.

The videodev2.h header is something users of the API look at a
lot and having this really ugly kernel timestamp in there is
not acceptably IMHO. But splitting it off should work.

> 
>>> +#ifdef __KERNEL__
>>> +struct v4l2_buffer_time32 {
>>> +     __u32                   index;
>>> +     __u32                   type;
>>> +     __u32                   bytesused;
>>> +     __u32                   flags;
>>> +     __u32                   field;
>>> +     struct old_timeval32    timestamp;
>>>       struct v4l2_timecode    timecode;
>>>       __u32                   sequence;
>>>
>>> @@ -1009,6 +1049,7 @@ struct v4l2_buffer {
>>>               __u32           reserved;
>>>       };
>>>  };
>>> +#endif
>>
>> Can this be moved to v4l2-ioctls.h?
> 
> done.
> 
>>>  #ifndef __KERNEL__
>>>  /**
>>> @@ -2446,12 +2487,15 @@ struct v4l2_create_buffers {
>>>  #define VIDIOC_S_FMT         _IOWR('V',  5, struct v4l2_format)
>>>  #define VIDIOC_REQBUFS               _IOWR('V',  8, struct v4l2_requestbuffers)
>>>  #define VIDIOC_QUERYBUF              _IOWR('V',  9, struct v4l2_buffer)
>>> +#define VIDIOC_QUERYBUF_TIME32       _IOWR('V',  9, struct v4l2_buffer_time32)
>>
>> And all these should be moved there as well.
> 
> done.
> 
>       Arnd
> 

Regards,

	Hans

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v4 5/8] media: v4l2-core: fix VIDIOC_DQEVENT for time64 ABI
  2019-11-25 14:40   ` Hans Verkuil
@ 2019-11-26 14:43     ` Arnd Bergmann
  2019-11-26 15:10       ` Hans Verkuil
  0 siblings, 1 reply; 25+ messages in thread
From: Arnd Bergmann @ 2019-11-26 14:43 UTC (permalink / raw)
  To: Hans Verkuil
  Cc: Mauro Carvalho Chehab, Linux Media Mailing List, linux-kernel,
	y2038 Mailman List

On Mon, Nov 25, 2019 at 3:40 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
> On 11/11/19 9:38 PM, Arnd Bergmann wrote:

> >       switch (cmd) {
> > +#ifdef CONFIG_COMPAT_32BIT_TIME
> > +     case VIDIOC_DQEVENT_TIME32: {
> > +             struct v4l2_event_time32 ev32;
> > +             struct v4l2_event *ev = parg;
> > +
> > +             memcpy(&ev32, ev, offsetof(struct v4l2_event, timestamp));
> > +             ev32.timestamp.tv_sec = ev->timestamp.tv_sec;
> > +             ev32.timestamp.tv_nsec = ev->timestamp.tv_nsec;
> > +             memcpy(&ev32.id, &ev->id, sizeof(*ev) - offsetof(struct v4l2_event, id));
>
> This looks dangerous: due to 64-bit alignment requirements the
> v4l2_event struct may end with a 4-byte hole at the end of the struct,
> which you do not want to copy to ev32.
>
> I think it is safer to just copy id and reserved separately:
>
>                 ev32.id = ev->id;
>                 memcpy(ev32.reserved, ev->reserved, sizeof(ev->reserved));

Actually I think it's that's also bad: The padding in *ev must already be
cleared here (otherwise there is a leak of stack data in the kernel
already), so  *not* copying the padding requires at least adding a memset
upfront.

I would do the per-member copy like I did for v4l2_buffer in my
other reply:

                struct v4l2_event *ev = parg;
                struct v4l2_event_time32 ev32 = {
                        .type           = ev->type,
                        .pending        = ev->pending,
                        .sequence       = ev->sequence,
                        .timestamp.tv_sec  = ev->timestamp.tv_sec,
                        .timestamp.tv_nsec = ev->timestamp.tv_nsec,
                        .id             = ev->id,
                };

                memcpy(ev32.u, ev->u, sizeof(ev->u));
                memcpy(ev32.reserved, ev->reserved, sizeof(ev->reserved));

                if (copy_to_user(arg, &ev32, sizeof(ev32)))
                        return -EFAULT;

Unfortunately this is a little uglier because it still requires the two
memcpy() for the arrays, but I think it's good enough.

Any other ideas? Let me know if I should do a memset()
plus individual member copy instead.
> > +             if (!(sd->flags & V4L2_SUBDEV_FL_HAS_EVENTS))
> > +                     return -ENOIOCTLCMD;
> > +
> > +             rval = v4l2_event_dequeue(vfh, &ev, file->f_flags & O_NONBLOCK);
> > +
> > +             memcpy(ev32, &ev, offsetof(struct v4l2_event, timestamp));
> > +             ev32->timestamp.tv_sec = ev.timestamp.tv_sec;
> > +             ev32->timestamp.tv_nsec = ev.timestamp.tv_nsec;
> > +             memcpy(&ev32->id, &ev.id,
> > +                    sizeof(ev) - offsetof(struct v4l2_event, id));
>
> Ditto.

Using the corresponding code here as well.

> > +
> >  #define V4L2_EVENT_SUB_FL_SEND_INITIAL               (1 << 0)
> >  #define V4L2_EVENT_SUB_FL_ALLOW_FEEDBACK     (1 << 1)
> >
> > @@ -2486,6 +2515,7 @@ struct v4l2_create_buffers {
> >  #define      VIDIOC_S_DV_TIMINGS     _IOWR('V', 87, struct v4l2_dv_timings)
> >  #define      VIDIOC_G_DV_TIMINGS     _IOWR('V', 88, struct v4l2_dv_timings)
> >  #define      VIDIOC_DQEVENT           _IOR('V', 89, struct v4l2_event)
> > +#define      VIDIOC_DQEVENT_TIME32    _IOR('V', 89, struct v4l2_event_time32)
>
> Shouldn't this be under #ifdef __KERNEL__?
>
> And should this be in the public header at all? media/v4l2-ioctl.h might be a better
> place.

Done.

       Arnd

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v4 5/8] media: v4l2-core: fix VIDIOC_DQEVENT for time64 ABI
  2019-11-26 14:43     ` Arnd Bergmann
@ 2019-11-26 15:10       ` Hans Verkuil
  2019-11-26 15:19         ` Arnd Bergmann
  0 siblings, 1 reply; 25+ messages in thread
From: Hans Verkuil @ 2019-11-26 15:10 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Mauro Carvalho Chehab, Linux Media Mailing List, linux-kernel,
	y2038 Mailman List

On 11/26/19 3:43 PM, Arnd Bergmann wrote:
> On Mon, Nov 25, 2019 at 3:40 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>> On 11/11/19 9:38 PM, Arnd Bergmann wrote:
> 
>>>       switch (cmd) {
>>> +#ifdef CONFIG_COMPAT_32BIT_TIME
>>> +     case VIDIOC_DQEVENT_TIME32: {
>>> +             struct v4l2_event_time32 ev32;
>>> +             struct v4l2_event *ev = parg;
>>> +
>>> +             memcpy(&ev32, ev, offsetof(struct v4l2_event, timestamp));
>>> +             ev32.timestamp.tv_sec = ev->timestamp.tv_sec;
>>> +             ev32.timestamp.tv_nsec = ev->timestamp.tv_nsec;
>>> +             memcpy(&ev32.id, &ev->id, sizeof(*ev) - offsetof(struct v4l2_event, id));
>>
>> This looks dangerous: due to 64-bit alignment requirements the
>> v4l2_event struct may end with a 4-byte hole at the end of the struct,
>> which you do not want to copy to ev32.
>>
>> I think it is safer to just copy id and reserved separately:
>>
>>                 ev32.id = ev->id;
>>                 memcpy(ev32.reserved, ev->reserved, sizeof(ev->reserved));
> 
> Actually I think it's that's also bad: The padding in *ev must already be
> cleared here (otherwise there is a leak of stack data in the kernel
> already), so  *not* copying the padding requires at least adding a memset
> upfront.
> 
> I would do the per-member copy like I did for v4l2_buffer in my
> other reply:
> 
>                 struct v4l2_event *ev = parg;
>                 struct v4l2_event_time32 ev32 = {
>                         .type           = ev->type,
>                         .pending        = ev->pending,
>                         .sequence       = ev->sequence,
>                         .timestamp.tv_sec  = ev->timestamp.tv_sec,
>                         .timestamp.tv_nsec = ev->timestamp.tv_nsec,
>                         .id             = ev->id,
>                 };
> 
>                 memcpy(ev32.u, ev->u, sizeof(ev->u));
>                 memcpy(ev32.reserved, ev->reserved, sizeof(ev->reserved));
> 
>                 if (copy_to_user(arg, &ev32, sizeof(ev32)))
>                         return -EFAULT;
> 
> Unfortunately this is a little uglier because it still requires the two
> memcpy() for the arrays, but I think it's good enough.

I agree.

Hmm, can't you do .u = ev->u ? Or is that not allowed by this syntax?

> 
> Any other ideas? Let me know if I should do a memset()
> plus individual member copy instead.

I think we have to, unless you have a better solution. This is leaking
information from the holes in the struct.

>>> +             if (!(sd->flags & V4L2_SUBDEV_FL_HAS_EVENTS))
>>> +                     return -ENOIOCTLCMD;
>>> +
>>> +             rval = v4l2_event_dequeue(vfh, &ev, file->f_flags & O_NONBLOCK);
>>> +
>>> +             memcpy(ev32, &ev, offsetof(struct v4l2_event, timestamp));
>>> +             ev32->timestamp.tv_sec = ev.timestamp.tv_sec;
>>> +             ev32->timestamp.tv_nsec = ev.timestamp.tv_nsec;
>>> +             memcpy(&ev32->id, &ev.id,
>>> +                    sizeof(ev) - offsetof(struct v4l2_event, id));
>>
>> Ditto.
> 
> Using the corresponding code here as well.
> 
>>> +
>>>  #define V4L2_EVENT_SUB_FL_SEND_INITIAL               (1 << 0)
>>>  #define V4L2_EVENT_SUB_FL_ALLOW_FEEDBACK     (1 << 1)
>>>
>>> @@ -2486,6 +2515,7 @@ struct v4l2_create_buffers {
>>>  #define      VIDIOC_S_DV_TIMINGS     _IOWR('V', 87, struct v4l2_dv_timings)
>>>  #define      VIDIOC_G_DV_TIMINGS     _IOWR('V', 88, struct v4l2_dv_timings)
>>>  #define      VIDIOC_DQEVENT           _IOR('V', 89, struct v4l2_event)
>>> +#define      VIDIOC_DQEVENT_TIME32    _IOR('V', 89, struct v4l2_event_time32)
>>
>> Shouldn't this be under #ifdef __KERNEL__?
>>
>> And should this be in the public header at all? media/v4l2-ioctl.h might be a better
>> place.
> 
> Done.
> 
>        Arnd
> 

Regards,

	Hans

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v4 6/8] media: v4l2-core: fix v4l2_buffer handling for time64 ABI
  2019-11-26 14:15       ` Hans Verkuil
@ 2019-11-26 15:17         ` Arnd Bergmann
  2019-11-26 15:24           ` Hans Verkuil
  0 siblings, 1 reply; 25+ messages in thread
From: Arnd Bergmann @ 2019-11-26 15:17 UTC (permalink / raw)
  To: Hans Verkuil
  Cc: Mauro Carvalho Chehab, Linux Media Mailing List, linux-kernel,
	y2038 Mailman List

On Tue, Nov 26, 2019 at 3:15 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>
> Then use that in the struct v4l2_buffer definition:
>
> struct v4l2_buffer {
> ...
> #ifdef __KERNEL__
>         struct __kernel_v4l2_timeval timestamp;
> #else
>         struct timeval               timestamp;
> #endif
>
> That keeps struct v4l2_buffer fairly clean. And it also makes it
> possible to have a bit more extensive documentation for the
> struct __kernel_v4l2_timeval without polluting the actual struct
> v4l2_buffer definition.

Yes, good idea. I've added this version now:

#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

I briefly considered using #else #define __kernel_v4l2_timeval timeval
to avoid the second #ifdef, but went back to your version again
for clarify.

> The videodev2.h header is something users of the API look at a
> lot and having this really ugly kernel timestamp in there is
> not acceptably IMHO. But splitting it off should work.

Do you also mean moving it into a separate header file, or
just outside of struct v4l2_buffer? Since it's hidden in #ifdef
__KERNEL__, it could be moved to media/ioctl.h or elsewhere.

      Arnd

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v4 5/8] media: v4l2-core: fix VIDIOC_DQEVENT for time64 ABI
  2019-11-26 15:10       ` Hans Verkuil
@ 2019-11-26 15:19         ` Arnd Bergmann
  0 siblings, 0 replies; 25+ messages in thread
From: Arnd Bergmann @ 2019-11-26 15:19 UTC (permalink / raw)
  To: Hans Verkuil
  Cc: Mauro Carvalho Chehab, Linux Media Mailing List, linux-kernel,
	y2038 Mailman List

On Tue, Nov 26, 2019 at 4:10 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>
> On 11/26/19 3:43 PM, Arnd Bergmann wrote:
> > On Mon, Nov 25, 2019 at 3:40 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
> >> On 11/11/19 9:38 PM, Arnd Bergmann wrote:
> >                 struct v4l2_event *ev = parg;
> >                 struct v4l2_event_time32 ev32 = {
> >                         .type           = ev->type,
> >                         .pending        = ev->pending,
> >                         .sequence       = ev->sequence,
> >                         .timestamp.tv_sec  = ev->timestamp.tv_sec,
> >                         .timestamp.tv_nsec = ev->timestamp.tv_nsec,
> >                         .id             = ev->id,
> >                 };
> >
> >                 memcpy(ev32.u, ev->u, sizeof(ev->u));
> >                 memcpy(ev32.reserved, ev->reserved, sizeof(ev->reserved));
> >
> >                 if (copy_to_user(arg, &ev32, sizeof(ev32)))
> >                         return -EFAULT;
> >
> > Unfortunately this is a little uglier because it still requires the two
> > memcpy() for the arrays, but I think it's good enough.
>
> I agree.
>
> Hmm, can't you do .u = ev->u ? Or is that not allowed by this syntax?

No, that doesn't work here since the two unions are considered
different types despite being defined identically. It would work by
giving the union a name, but that name would also become visible
to user space without adding more hacks.

       Arnd

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v4 6/8] media: v4l2-core: fix v4l2_buffer handling for time64 ABI
  2019-11-26 15:17         ` Arnd Bergmann
@ 2019-11-26 15:24           ` Hans Verkuil
  0 siblings, 0 replies; 25+ messages in thread
From: Hans Verkuil @ 2019-11-26 15:24 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Mauro Carvalho Chehab, Linux Media Mailing List, linux-kernel,
	y2038 Mailman List

On 11/26/19 4:17 PM, Arnd Bergmann wrote:
> On Tue, Nov 26, 2019 at 3:15 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>>
>> Then use that in the struct v4l2_buffer definition:
>>
>> struct v4l2_buffer {
>> ...
>> #ifdef __KERNEL__
>>         struct __kernel_v4l2_timeval timestamp;
>> #else
>>         struct timeval               timestamp;
>> #endif
>>
>> That keeps struct v4l2_buffer fairly clean. And it also makes it
>> possible to have a bit more extensive documentation for the
>> struct __kernel_v4l2_timeval without polluting the actual struct
>> v4l2_buffer definition.
> 
> Yes, good idea. I've added this version now:
> 
> #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
> 
> I briefly considered using #else #define __kernel_v4l2_timeval timeval
> to avoid the second #ifdef, but went back to your version again
> for clarify.
> 
>> The videodev2.h header is something users of the API look at a
>> lot and having this really ugly kernel timestamp in there is
>> not acceptably IMHO. But splitting it off should work.
> 
> Do you also mean moving it into a separate header file, or
> just outside of struct v4l2_buffer? Since it's hidden in #ifdef
> __KERNEL__, it could be moved to media/ioctl.h or elsewhere.

I've thought about that, but that risks having to change drivers
since they would now have to include another header to get the
right timeval definition. In the end I don't think it is worth the
effort.

I think it is best to define __kernel_v4l2_timeval just before
the struct v4l2_requestbuffers definition rather than before the
struct v4l2_buffer. That way it doesn't interfere with the
userspace structs for the buffer API.

Regards,

	Hans

> 
>       Arnd
> 


^ permalink raw reply	[flat|nested] 25+ messages in thread

end of thread, other threads:[~2019-11-26 15:24 UTC | newest]

Thread overview: 25+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-11 20:38 [PATCH v4 0/8] y2038 safety in v4l2 Arnd Bergmann
2019-11-11 20:38 ` [PATCH v4 1/8] media: documentation: fix video_event description Arnd Bergmann
2019-11-11 20:38 ` [PATCH v4 2/8] media: v4l2: abstract timeval handling in v4l2_buffer Arnd Bergmann
2019-11-25 15:52   ` Hans Verkuil
2019-11-26 11:34     ` Arnd Bergmann
2019-11-26 11:43       ` Hans Verkuil
2019-11-26 12:42         ` Arnd Bergmann
2019-11-11 20:38 ` [PATCH v4 3/8] media: v4l2-core: compat: ignore native command codes Arnd Bergmann
     [not found]   ` <20191122070015.D5A702068E@mail.kernel.org>
2019-11-22 12:29     ` Arnd Bergmann
2019-11-11 20:38 ` [PATCH v4 4/8] media: v4l2-core: split out data copy from video_usercopy Arnd Bergmann
2019-11-11 20:38 ` [PATCH v4 5/8] media: v4l2-core: fix VIDIOC_DQEVENT for time64 ABI Arnd Bergmann
2019-11-25 14:40   ` Hans Verkuil
2019-11-26 14:43     ` Arnd Bergmann
2019-11-26 15:10       ` Hans Verkuil
2019-11-26 15:19         ` Arnd Bergmann
2019-11-11 20:38 ` [PATCH v4 6/8] media: v4l2-core: fix v4l2_buffer handling " Arnd Bergmann
2019-11-25 14:57   ` Hans Verkuil
2019-11-26 13:50     ` Arnd Bergmann
2019-11-26 14:15       ` Hans Verkuil
2019-11-26 15:17         ` Arnd Bergmann
2019-11-26 15:24           ` Hans Verkuil
2019-11-11 20:38 ` [PATCH v4 7/8] media: v4l2-core: fix compat VIDIOC_DQEVENT " Arnd Bergmann
2019-11-11 20:38 ` [PATCH v4 8/8] media: v4l2-core: fix compat v4l2_buffer handling " Arnd Bergmann
2019-11-25 16:02 ` [PATCH v4 0/8] y2038 safety in v4l2 Hans Verkuil
2019-11-26 11:13   ` Arnd Bergmann

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).