| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <aPaV56-NxiFFx1XU@valkosipuli.retiisi.eu>
Date: Mon, 20 Oct 2025 23:04:55 +0300
From: Sakari Ailus <sakari.ailus@....fi>
To: Ricardo Ribalda <ribalda@...omium.org>
Cc: Dan Carpenter <dan.carpenter@...aro.org>,
Ricardo Ribalda <ribalda@...nel.org>,
Hans Verkuil <hverkuil+cisco@...nel.org>,
linux-media@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] media: i2c: imx214: Exit early on control init errors
Hi Ricardo,
On Mon, Oct 20, 2025 at 09:58:47PM +0200, Ricardo Ribalda wrote:
> Hi Sakai
>
> On Mon, 20 Oct 2025 at 21:34, Sakari Ailus <sakari.ailus@....fi> wrote:
> >
> > Hi Ricardo,
> >
> > On Mon, Oct 20, 2025 at 08:51:44PM +0200, Ricardo Ribalda wrote:
> > > Hi Sakari
> > >
> > > On Mon, 20 Oct 2025 at 20:28, Sakari Ailus <sakari.ailus@....fi> wrote:
> > > >
> > > > Hi Ricardo,
> > > >
> > > > On Tue, Oct 14, 2025 at 11:00:17AM +0000, Ricardo Ribalda wrote:
> > > > > Now we try to initialize all the controls and at the very end check
> > > > > ctrl_hdlr->error to check if one of them has failed.
> > > > >
> > > > > This confuses smatch, who do not know how to track the state of
> > > > > imx214->link_freq.
> > > > >
> > > > > drivers/media/i2c/imx214.c:1109 imx214_ctrls_init() error: we previously assumed 'imx214->link_freq' could be null (see line 1017)
> > > > >
> > > > > Fix this by exiting early on control initialization errors.
> > > > >
> > > > > Signed-off-by: Ricardo Ribalda <ribalda@...omium.org>
> > > > > ---
> > > > > Right now we are handling this with a quirk in media-ci, if Dan cannot
> > > > > fix smatch in a kernel cycle we should merge this patch.
> > > > > ---
> > > > > Changes in v2:
> > > > > - Fix typo in commit message commit
> > > > > - Move error tag where it belongs (Thanks Hans!)
> > > > > - Link to v1: https://lore.kernel.org/r/20250829-imx214-smatch-v1-1-f3d1653b48e4@chromium.org
> > > > > ---
> > > > > drivers/media/i2c/imx214.c | 7 +++++--
> > > > > 1 file changed, 5 insertions(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/drivers/media/i2c/imx214.c b/drivers/media/i2c/imx214.c
> > > > > index 94ebe625c9e6ee0fb67fe1d89b48b2f1bf58ffc6..c66f0e18726c3fc15df91c37888a797bcea82134 100644
> > > > > --- a/drivers/media/i2c/imx214.c
> > > > > +++ b/drivers/media/i2c/imx214.c
> > > > > @@ -1014,8 +1014,10 @@ static int imx214_ctrls_init(struct imx214 *imx214)
> > > > > V4L2_CID_LINK_FREQ,
> > > > > imx214->bus_cfg.nr_of_link_frequencies - 1,
> > > > > 0, imx214->bus_cfg.link_frequencies);
> > > > > - if (imx214->link_freq)
> > > > > - imx214->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> > > > > + if (!imx214->link_freq)
> > > > > + goto err_init_ctrl;
> > > > > +
> > > > > + imx214->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> > > >
> > > > You could do this cleaner by simply moving the assignment after the handler
> > > > error check. Some drivers do that already.
> > > >
> > > > I wonder why this seems to be a problem for smatch in the imx214 driver as
> > > > the pattern is widely used across the sensor drivers.
> > >
> > > Smatch thinks that there could be case where
> > >
> > > imx->link_freq = NULL, and imx214_pll_update returns 0.
> > >
> > > That is not solved by moving the assignment `imx214->link_freq->flags
> > > |=` after if (ret)
> >
> > Did you test this? The smatch message suggests otherwise (but of course
> > this could just turn into a different smatch error).
>
> Actually smatch do not hate it :)
>
> ribalda@...alda:~/work/linux$ make -i W=1 C=1
> CHECK="../media-ci/third_party/smatch/smatch -p=kernel"
> KCFLAGS="-Wmaybe-uninitialized" drivers/media/i2c/imx214.o
> CC kernel/sched/rq-offsets.s
> In file included from kernel/sched/rq-offsets.c:5:
> kernel/sched/sched.h: In function ‘mm_cid_get’:
> kernel/sched/sched.h:3743:25: error: variable ‘cpumask’ set but not
> used [-Werror=unused-but-set-variable]
> 3743 | struct cpumask *cpumask;
> | ^~~~~~~
> cc1: all warnings being treated as errors
> make[2]: [scripts/Makefile.build:182: kernel/sched/rq-offsets.s] Error
> 1 (ignored)
> /bin/sh: line 1: kernel/sched/rq-offsets.s: No such file or directory
> make[2]: [Kbuild:46: include/generated/rq-offsets.h] Error 1 (ignored)
> CALL scripts/checksyscalls.sh
> DESCEND objtool
> INSTALL libsubcmd_headers
> CC drivers/media/i2c/imx214.o
> CHECK drivers/media/i2c/imx214.c
>
>
> ribalda@...alda:~/work/linux$ git diff
> diff --git a/drivers/media/i2c/imx214.c b/drivers/media/i2c/imx214.c
> index 94ebe625c9e6..a21461b55923 100644
> --- a/drivers/media/i2c/imx214.c
> +++ b/drivers/media/i2c/imx214.c
> @@ -1014,8 +1014,6 @@ static int imx214_ctrls_init(struct imx214 *imx214)
> V4L2_CID_LINK_FREQ,
>
> imx214->bus_cfg.nr_of_link_frequencies - 1,
> 0,
> imx214->bus_cfg.link_frequencies);
> - if (imx214->link_freq)
> - imx214->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
>
> /*
> * WARNING!
> @@ -1038,9 +1036,6 @@ static int imx214_ctrls_init(struct imx214 *imx214)
> imx214->hblank = v4l2_ctrl_new_std(ctrl_hdlr, &imx214_ctrl_ops,
> V4L2_CID_HBLANK, hblank, hblank,
> 1, hblank);
> - if (imx214->hblank)
> - imx214->hblank->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> -
> exposure_max = mode->vts_def - IMX214_EXPOSURE_OFFSET;
> exposure_def = min(exposure_max, IMX214_EXPOSURE_DEFAULT);
> imx214->exposure = v4l2_ctrl_new_std(ctrl_hdlr, &imx214_ctrl_ops,
> @@ -1060,13 +1055,9 @@ static int imx214_ctrls_init(struct imx214 *imx214)
>
> imx214->hflip = v4l2_ctrl_new_std(ctrl_hdlr, &imx214_ctrl_ops,
> V4L2_CID_HFLIP, 0, 1, 1, 0);
> - if (imx214->hflip)
> - imx214->hflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
>
> imx214->vflip = v4l2_ctrl_new_std(ctrl_hdlr, &imx214_ctrl_ops,
> V4L2_CID_VFLIP, 0, 1, 1, 0);
> - if (imx214->vflip)
> - imx214->vflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
>
> v4l2_ctrl_cluster(2, &imx214->hflip);
>
> @@ -1106,6 +1097,11 @@ static int imx214_ctrls_init(struct imx214 *imx214)
> return ret;
> }
>
> + imx214->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> + imx214->hblank->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> + imx214->hflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
> + imx214->vflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
> +
> ret = imx214_pll_update(imx214);
> if (ret < 0) {
> v4l2_ctrl_handler_free(ctrl_hdlr);
>
>
> >
> > >
> > > I believe Dan is already flagged about this, but I do not think that
> > > it will be super simple to fix in his code.
> > >
> > > If smatch can handle this case before rc5 I will delete this patch.
> >
> > There are other options, too, such as storing the link frequency index (the
> > driver won't even support setting it) or the frequency itself.
>
> There are plenty of options :) But I am still failing to see what is
> wrong with this patch.
Error handling is often complicated and poorly tested. If you don't need to
introduce new labels to do it, just don't. The control framework does it
nicely to simplify drivers so let's rely on it.
>
> We exit early when there is an error instead of continuing doing work
> that will be useless.
>
> If you really prefer your way I can send a v3... but we have probably
> more fun work to do :P
Yes, but I don't want this pattern to spread. That tends to happen if you
introduce it in one driver.
--
Regards,
Sakari Ailus
Powered by blists - more mailing lists