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 02:12:47PM +0200, Ricardo Ribalda wrote:
> On Wed, 6 May 2026 at 13:18, Laurent Pinchart wrote:
> > 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.
> 
> Ideally yes, but LLVM/gcc has hundreds (maybe thousands?) of
> contributors and sparse/smatch are maintained by a handful group of
> heroic warriors.

What I meant is that I don't think compilers will optimize this one out.

> > > 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) ?
> 
> It depends on the nature of the range check and the consequences of
> failing it. We can decide case by case and worst case scenario we can
> ignore specific errors.
> 
> The mt9p031 patch in this series is special, the issue is not the
> range check, but that smatch did not properly handle a mask operation.
> Dan is working to fix it.

In the mt9p031 case, we have

	ctrl->val &= ~7;
	data = ((ctrl->val - 64) << 5) | (1 << 6) | 32;

ctrl->val is a 32-bit value, data is a 16-bit value. We know that
ctrl->val will be at most 1024, because that's the maximum limit set
when creating the control, but I don't see how smatch or a compiler
would be able to see that. I'm not blaming smatch on this one.

> > > 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.
> 
> I agree with the idea, but this might not the right forum. It would be
> a great topic for the Kernel Summit, though.

I was thinking about the same, yes.

> The "rust people" would love to have some annotations about the
> guarantees of the C code.

I wonder if that could be used to offer similar guarantees (or rather a
subset thereof) of the rust type system. Note that I'd expect rust to
also struggle in many cases, as we really have guarantees that won't be
expressable through its type system.

> But until we have those annotations the best we can do is runtime
> checks where they make sense and keep track of where they do not make
> sense (allow lists). That will make future discussions more
> productive.

Should we add comments on top of all those redundant checks, with a
standardized formatting to make them easy to locate in the future ? For
instance

	/* false-positive: Should never happen due to ... */

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

-- 
Regards,

Laurent Pinchart