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

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

On Wed, May 06, 2026 at 08:48:06AM +0200, Ricardo Ribalda wrote:
> On Wed, 6 May 2026 at 01:12, Laurent Pinchart 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@chromium.org>
> > > ---
> > >  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@ideasonboard.com>
> >
> > 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.

If the compiler can optimize them out, then I expect the static analysis
tools to also get the ability to avoid those false positives.

> The added checks make the code more robust for future refactoring and
> serve as useful documentation.

In this specific case, if we want to make the code more robust, I think
we should have an array of minor offsets and calculate the minor ranges
based on that. It would avoid offsets and ranges getting out of sync. I
don't think it's worth it though, because I don't foresee we will ever
change ranges for the fixed minors case.

> Furthermore, when false positives occur
> in new code, they force the author to write more idiomatic code, which
> improves maintainability.

Up to a point only. Sometimes guarantees are provided in a very remote
place. For instance, control values have ranges that are set when
creating the control, and enforced by the control framework. Adding
range checks to .s_ctrl() is redundant, but I don't expect static
analysis tools to be able to understand that the value is guaranteed to
be within the range (especially given that we support modifying ranges
at runtime). How can we avoid those redundant checks (or mask
operations, as in the mt9p031 patch in this series) ?

> 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.

I do like smatch a lot, it points out real issues. My concern is that we
have code that offers guarantees in ways that are not visible to static
analysis tools (or to compilers), and having to add more runtime checks
everywhere to silence warning is not nice. Those checks can over time
become redundant (because tools improve, or because we remove the code),
and will likely not be removed because nobody notices.

There's a broader question here of how to connect pieces of code where
one piece offers a guarantee that the other piece depends on, in a way
that can be exposed to tools.

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

-- 
Regards,

Laurent Pinchart