[PATCH] media: vb2: frame_vector.c: replace WARN_ONCE with a comment

[PATCH] media: vb2: frame_vector.c: replace WARN_ONCE with a comment

[Hans Verkuil]

The WARN_ONCE was issued also in cases that had nothing to do with VM_IO
(e.g. if the start address was just a random value and uaccess fails with
-EFAULT).

There are no reports of WARN_ONCE being issued for actual VM_IO cases, so
just drop it and instead add a note to the comment before the function.

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
---
diff --git a/drivers/media/common/videobuf2/frame_vector.c b/drivers/media/common/videobuf2/frame_vector.c
index 0f430ddc1f67..fd87747be9b1 100644
--- a/drivers/media/common/videobuf2/frame_vector.c
+++ b/drivers/media/common/videobuf2/frame_vector.c
@@ -31,6 +31,10 @@
  * different type underlying the specified range of virtual addresses.
  * When the function isn't able to map a single page, it returns error.
  *
+ * Note that get_vaddr_frames() cannot follow VM_IO mappings. It used
+ * to be able to do that, but that could (racily) return non-refcounted
+ * pfns.
+ *
  * This function takes care of grabbing mmap_lock as necessary.
  */
 int get_vaddr_frames(unsigned long start, unsigned int nr_frames, bool write,
@@ -59,8 +63,6 @@ int get_vaddr_frames(unsigned long start, unsigned int nr_frames, bool write,
 	if (likely(ret > 0))
 		return ret;

-	/* This used to (racily) return non-refcounted pfns. Let people know */
-	WARN_ONCE(1, "get_vaddr_frames() cannot follow VM_IO mapping");
 	vec->nr_frames = 0;
 	return ret ? ret : -EFAULT;
 }

Re: [PATCH] media: vb2: frame_vector.c: replace WARN_ONCE with a comment

[David Hildenbrand]

On 17.08.23 12:41, Hans Verkuil wrote:
> The WARN_ONCE was issued also in cases that had nothing to do with VM_IO
> (e.g. if the start address was just a random value and uaccess fails with
> -EFAULT).
> 
> There are no reports of WARN_ONCE being issued for actual VM_IO cases, so
> just drop it and instead add a note to the comment before the function.
> 
> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> ---
> diff --git a/drivers/media/common/videobuf2/frame_vector.c b/drivers/media/common/videobuf2/frame_vector.c
> index 0f430ddc1f67..fd87747be9b1 100644
> --- a/drivers/media/common/videobuf2/frame_vector.c
> +++ b/drivers/media/common/videobuf2/frame_vector.c
> @@ -31,6 +31,10 @@
>    * different type underlying the specified range of virtual addresses.
>    * When the function isn't able to map a single page, it returns error.
>    *
> + * Note that get_vaddr_frames() cannot follow VM_IO mappings. It used
> + * to be able to do that, but that could (racily) return non-refcounted
> + * pfns.
> + *
>    * This function takes care of grabbing mmap_lock as necessary.
>    */
>   int get_vaddr_frames(unsigned long start, unsigned int nr_frames, bool write,
> @@ -59,8 +63,6 @@ int get_vaddr_frames(unsigned long start, unsigned int nr_frames, bool write,
>   	if (likely(ret > 0))
>   		return ret;
> 
> -	/* This used to (racily) return non-refcounted pfns. Let people know */
> -	WARN_ONCE(1, "get_vaddr_frames() cannot follow VM_IO mapping");
>   	vec->nr_frames = 0;
>   	return ret ? ret : -EFAULT;
>   }
> 

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 
Cheers,

David / dhildenb

Re: [PATCH] media: vb2: frame_vector.c: replace WARN_ONCE with a comment

[Hans Verkuil]

On 17/08/2023 12:41, Hans Verkuil wrote:
> The WARN_ONCE was issued also in cases that had nothing to do with VM_IO
> (e.g. if the start address was just a random value and uaccess fails with
> -EFAULT).
> 
> There are no reports of WARN_ONCE being issued for actual VM_IO cases, so
> just drop it and instead add a note to the comment before the function.
> 
> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>

I forgot to add:

Reported-by: Yikebaer Aizezi <yikebaer61@gmail.com>


> ---
> diff --git a/drivers/media/common/videobuf2/frame_vector.c b/drivers/media/common/videobuf2/frame_vector.c
> index 0f430ddc1f67..fd87747be9b1 100644
> --- a/drivers/media/common/videobuf2/frame_vector.c
> +++ b/drivers/media/common/videobuf2/frame_vector.c
> @@ -31,6 +31,10 @@
>   * different type underlying the specified range of virtual addresses.
>   * When the function isn't able to map a single page, it returns error.
>   *
> + * Note that get_vaddr_frames() cannot follow VM_IO mappings. It used
> + * to be able to do that, but that could (racily) return non-refcounted
> + * pfns.
> + *
>   * This function takes care of grabbing mmap_lock as necessary.
>   */
>  int get_vaddr_frames(unsigned long start, unsigned int nr_frames, bool write,
> @@ -59,8 +63,6 @@ int get_vaddr_frames(unsigned long start, unsigned int nr_frames, bool write,
>  	if (likely(ret > 0))
>  		return ret;
> 
> -	/* This used to (racily) return non-refcounted pfns. Let people know */
> -	WARN_ONCE(1, "get_vaddr_frames() cannot follow VM_IO mapping");
>  	vec->nr_frames = 0;
>  	return ret ? ret : -EFAULT;
>  }
> 

Re: [PATCH] media: vb2: frame_vector.c: replace WARN_ONCE with a comment

[Linus Torvalds]

On Thu, 17 Aug 2023 at 12:41, Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote:
>
> There are no reports of WARN_ONCE being issued for actual VM_IO cases, so
> just drop it and instead add a note to the comment before the function.

Ack. That was meant to catch any (unlikely) strange users, but yeah,
it can obviously be triggered by "intentional" strange users, ie
syzbot and friends, so since there seems to be no sign of actual
real-world use, just removing the WARN_ONCE() is the right thing to
do.

I'm assuming I'll get this eventually through the regular media pulls?

                Linus

Re: [PATCH] media: vb2: frame_vector.c: replace WARN_ONCE with a comment

[Hans Verkuil]

On 17/08/2023 16:56, Linus Torvalds wrote:
> On Thu, 17 Aug 2023 at 12:41, Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote:
>>
>> There are no reports of WARN_ONCE being issued for actual VM_IO cases, so
>> just drop it and instead add a note to the comment before the function.
> 
> Ack. That was meant to catch any (unlikely) strange users, but yeah,
> it can obviously be triggered by "intentional" strange users, ie
> syzbot and friends, so since there seems to be no sign of actual
> real-world use, just removing the WARN_ONCE() is the right thing to
> do.
> 
> I'm assuming I'll get this eventually through the regular media pulls?
> 
>                 Linus

Yes, that's the plan.

Regards,

	Hans