| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CANiDSCtFRugwLX-9jLUwkvxxvO2EFZES6899qcdfnoeQwX3fOQ@mail.gmail.com>
Date: Fri, 23 Feb 2024 15:09:39 +0100
From: Ricardo Ribalda <ribalda@...omium.org>
To: Michael Grzeschik <m.grzeschik@...gutronix.de>
Cc: Laurent Pinchart <laurent.pinchart@...asonboard.com>,
Mauro Carvalho Chehab <mchehab@...nel.org>, linux-media@...r.kernel.org,
linux-kernel@...r.kernel.org, kernel@...gutronix.de
Subject: Re: [PATCH] uvc_video: check for fid change early in decode_start and
avoid wrong error counting
Hi Michael
In your code when is uvc_video_stats_update() called or
stream->sequence incremented in normal use case?
I might be interpreting it wrong, but it seems like if buf->bytesused
is !=0 that code is never called.
Regards!
On Wed, 21 Feb 2024 at 23:53, Michael Grzeschik
<m.grzeschik@...gutronix.de> wrote:
>
> When the uvc request will get parsed by uvc_video_decode_start it will
> leave the function with -EAGAIN to be restarted on the next frame. While
> the first wrong parse the statistics will already be updated with
> uvc_video_stats_decode.
>
> One value e.g. is the error_count, which therefor will be incremented
> twice in case the fid has changed on the way. This patch fixes the
> unnecessary extra parsing by returning early from the function when the
> fid has changed.
>
> Signed-off-by: Michael Grzeschik <m.grzeschik@...gutronix.de>
> ---
> drivers/media/usb/uvc/uvc_video.c | 64 +++++++++++++++++++--------------------
> 1 file changed, 32 insertions(+), 32 deletions(-)
>
> diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
> index 7cbf4692bd875..fce5349b5f9fa 100644
> --- a/drivers/media/usb/uvc/uvc_video.c
> +++ b/drivers/media/usb/uvc/uvc_video.c
> @@ -1068,11 +1068,43 @@ static int uvc_video_decode_start(struct uvc_streaming *stream,
>
> fid = data[1] & UVC_STREAM_FID;
>
> + /*
> + * Store the payload FID bit and return immediately when the buffer is
> + * NULL.
> + */
> + if (buf == NULL) {
> + stream->last_fid = fid;
> + return -ENODATA;
> + }
> +
> /*
> * Increase the sequence number regardless of any buffer states, so
> * that discontinuous sequence numbers always indicate lost frames.
> */
> if (stream->last_fid != fid) {
> + /*
> + * Mark the buffer as done if we're at the beginning of a new frame.
> + * End of frame detection is better implemented by checking the EOF
> + * bit (FID bit toggling is delayed by one frame compared to the EOF
> + * bit), but some devices don't set the bit at end of frame (and the
> + * last payload can be lost anyway). We thus must check if the FID has
> + * been toggled.
> + *
> + * stream->last_fid is initialized to -1, so the first isochronous
> + * frame will never trigger an end of frame detection.
> + *
> + * Empty buffers (bytesused == 0) don't trigger end of frame detection
> + * as it doesn't make sense to return an empty buffer. This also
> + * avoids detecting end of frame conditions at FID toggling if the
> + * previous payload had the EOF bit set.
> + */
> + if (buf->bytesused) {
> + uvc_dbg(stream->dev, FRAME,
> + "Frame complete (FID bit toggled)\n");
> + buf->state = UVC_BUF_STATE_READY;
> + return -EAGAIN;
> + }
> +
> stream->sequence++;
> if (stream->sequence)
> uvc_video_stats_update(stream);
> @@ -1081,15 +1113,6 @@ static int uvc_video_decode_start(struct uvc_streaming *stream,
> uvc_video_clock_decode(stream, buf, data, len);
> uvc_video_stats_decode(stream, data, len);
>
> - /*
> - * Store the payload FID bit and return immediately when the buffer is
> - * NULL.
> - */
> - if (buf == NULL) {
> - stream->last_fid = fid;
> - return -ENODATA;
> - }
> -
> /* Mark the buffer as bad if the error bit is set. */
> if (data[1] & UVC_STREAM_ERR) {
> uvc_dbg(stream->dev, FRAME,
> @@ -1124,29 +1147,6 @@ static int uvc_video_decode_start(struct uvc_streaming *stream,
> buf->state = UVC_BUF_STATE_ACTIVE;
> }
>
> - /*
> - * Mark the buffer as done if we're at the beginning of a new frame.
> - * End of frame detection is better implemented by checking the EOF
> - * bit (FID bit toggling is delayed by one frame compared to the EOF
> - * bit), but some devices don't set the bit at end of frame (and the
> - * last payload can be lost anyway). We thus must check if the FID has
> - * been toggled.
> - *
> - * stream->last_fid is initialized to -1, so the first isochronous
> - * frame will never trigger an end of frame detection.
> - *
> - * Empty buffers (bytesused == 0) don't trigger end of frame detection
> - * as it doesn't make sense to return an empty buffer. This also
> - * avoids detecting end of frame conditions at FID toggling if the
> - * previous payload had the EOF bit set.
> - */
> - if (fid != stream->last_fid && buf->bytesused != 0) {
> - uvc_dbg(stream->dev, FRAME,
> - "Frame complete (FID bit toggled)\n");
> - buf->state = UVC_BUF_STATE_READY;
> - return -EAGAIN;
> - }
> -
> stream->last_fid = fid;
>
> return data[0];
>
> ---
> base-commit: 3bf0514dc6f36f81ee11b1becd977cb87b4c90c6
> change-id: 20240221-uvc-host-video-decode-start-af53df5924cd
>
> Best regards,
> --
> Michael Grzeschik <m.grzeschik@...gutronix.de>
>
>
--
Ricardo Ribalda
Powered by blists - more mailing lists