| Message ID | d5e1ee76-75b3-26cb-23ae-cf6ab40597b7@xs4all.nl (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: vb2: frame_vector.c: replace WARN_ONCE with a comment | expand |
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>
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; > } >
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
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
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; }
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> ---