linux-kernel - Re: [PATCH] media: vivid: Enforce a monotonic sequence number

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <99b9aa7f-f2eb-450a-a589-0e63adf97fcf@xs4all.nl>
Date: Mon, 22 Apr 2024 14:16:59 +0200
From: Hans Verkuil <hverkuil@...all.nl>
To: Ricardo Ribalda <ribalda@...omium.org>,
 Mauro Carvalho Chehab <mchehab@...nel.org>
Cc: linux-media@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] media: vivid: Enforce a monotonic sequence number

On 17/04/2024 11:30, Ricardo Ribalda wrote:
> Make sure that the sequence number is at least one unit bigger than the
> previous one.
> 
> This solves situations where the sequence number calculation does not
> have enough accuracy and repeats a sequence number.
> 
> Signed-off-by: Ricardo Ribalda <ribalda@...omium.org>
> ---
> We have seen some examples where the sequence number has been
> duplicated. This should not happen.
> ---
>  drivers/media/test-drivers/vivid/vivid-kthread-cap.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/media/test-drivers/vivid/vivid-kthread-cap.c b/drivers/media/test-drivers/vivid/vivid-kthread-cap.c
> index 42048727d7ff..6375e6484a4b 100644
> --- a/drivers/media/test-drivers/vivid/vivid-kthread-cap.c
> +++ b/drivers/media/test-drivers/vivid/vivid-kthread-cap.c
> @@ -665,7 +665,7 @@ static int vivid_thread_vid_cap(void *data)
>  {
>  	struct vivid_dev *dev = data;
>  	u64 numerators_since_start;
> -	u64 buffers_since_start;
> +	u64 buffers_since_start = ~0;
>  	u64 next_jiffies_since_start;
>  	unsigned long jiffies_since_start;
>  	unsigned long cur_jiffies;
> @@ -691,6 +691,7 @@ static int vivid_thread_vid_cap(void *data)
>  	vivid_cap_update_frame_period(dev);
>  
>  	for (;;) {
> +		u64 tmp;
>  		try_to_freeze();
>  		if (kthread_should_stop())
>  			break;
> @@ -719,9 +720,10 @@ static int vivid_thread_vid_cap(void *data)
>  		/* Calculate the number of jiffies since we started streaming */
>  		jiffies_since_start = cur_jiffies - dev->jiffies_vid_cap;
>  		/* Get the number of buffers streamed since the start */
> -		buffers_since_start = (u64)jiffies_since_start * denominator +
> -				      (HZ * numerator) / 2;
> -		do_div(buffers_since_start, HZ * numerator);
> +		tmp = (u64)jiffies_since_start * denominator +
> +		      (HZ * numerator) / 2;
> +		do_div(tmp, HZ * numerator);
> +		buffers_since_start = max(tmp, buffers_since_start + 1);
>  
>  		/*
>  		 * After more than 0xf0000000 (rounded down to a multiple of
> @@ -746,7 +748,7 @@ static int vivid_thread_vid_cap(void *data)
>  		 * Calculate the number of 'numerators' streamed since we started,
>  		 * including the current buffer.
>  		 */
> -		numerators_since_start = ++buffers_since_start * numerator;
> +		numerators_since_start = (buffers_since_start + 1) * numerator;
>  
>  		/* And the number of jiffies since we started */
>  		jiffies_since_start = jiffies - dev->jiffies_vid_cap;
> 

Just FYI: this patch isn't right: it's fixing the symptom, not the cause.
I am working on a better patch for this. It's the same for the v4l2-compliance
patch.

So I'll reject these two patches in patchwork.

Regards,

	Hans

> ---
> base-commit: 836e2548524d2dfcb5acaf3be78f203b6b4bde6f
> change-id: 20240417-vivid-seq-d0eb044a085f
> 
> Best regards,