Re: [PATCH v3 1/6] media: v4l2-dev: Add range check for vdev->minor

[Ricardo Ribalda]

Hi Laurent

On Wed, 6 May 2026 at 01:12, Laurent Pinchart
<laurent.pinchart@xxxxxxxxxxxxxxxx> wrote:
>
> On Mon, May 04, 2026 at 06:54:04AM +0000, Ricardo Ribalda wrote:
> > If the fixed minor ranges are not properly set we could end up in a
> > situation where the calculated minor is invalid. Add a check for this in
> > the code to make it more robust.
>
> If it was just for that, we could define the ranges in a way that could
> not lead to future programmatic errors.
>
> > This check also fixes the following false positive smatch warning:
> >
> > drivers/media/v4l2-core/v4l2-dev.c:1036 __video_register_device() error: buffer overflow 'video_devices' 256 <= 288
> > drivers/media/v4l2-core/v4l2-dev.c:1043 __video_register_device() error: buffer overflow 'video_devices' 256 <= 288
> > drivers/media/v4l2-core/v4l2-dev.c:1101 __video_register_device() error: buffer overflow 'video_devices' 256 <= 288
> >
> > Signed-off-by: Ricardo Ribalda <ribalda@xxxxxxxxxxxx>
> > ---
> >  drivers/media/v4l2-core/v4l2-dev.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> >
> > diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
> > index 6ce623a1245a..5516b2bbb08f 100644
> > --- a/drivers/media/v4l2-core/v4l2-dev.c
> > +++ b/drivers/media/v4l2-core/v4l2-dev.c
> > @@ -1032,6 +1032,11 @@ int __video_register_device(struct video_device *vdev,
> >       vdev->minor = i + minor_offset;
> >       vdev->num = nr;
> >
> > +     if (WARN_ON(vdev->minor >= VIDEO_NUM_DEVICES)) {
> > +             mutex_unlock(&videodev_lock);
>
> I may get tempted to convert code to using scoped guards at some point.
>
> > +             return -EINVAL;
> > +     }
> > +
>
> I'm annoyed by the proliferation of workarounds for smatch false
> positives that generate useless code :-/ This is in particular is not a
> big deal though, so
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@xxxxxxxxxxxxxxxx>
>
> but I don't want to continue in this direction with every new kernel
> release. We need a way to tell smatch that this is safe with incurring
> an runtime cost.

To provide some context, in this release alone we have found two "Fixes:".

Do you have any data regarding the runtime cost of these new checks?
Most of the time, the compiler simply optimizes them out, so the
impact is either zero or minimal.

The added checks make the code more robust for future refactoring and
serve as useful documentation. Furthermore, when false positives occur
in new code, they force the author to write more idiomatic code, which
improves maintainability. We have a deficit of maintainers, not
authors.

So, I DO want to continue in this direction. I am very grateful for
these static analysis tools; they truly make our code more
maintainable.

Regards!


>
> >       /* Should not happen since we thought this minor was free */
> >       if (WARN_ON(video_devices[vdev->minor])) {
> >               mutex_unlock(&videodev_lock);
> >
>
> --
> Regards,
>
> Laurent Pinchart



-- 
Ricardo Ribalda