| Message ID | 8d6193ed8d53ce94d595cb431627bbc7783c0e4c.1729230718.git.mchehab+huawei@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Media: fix several issues on drivers | expand |
On Fri, 2024-10-18 at 07:53 +0200, Mauro Carvalho Chehab wrote: > fepriv->auto_sub_step is unsigned. Setting it to -1 is just a > trick to avoid calling continue, as reported by Coverity. > > It relies to have this code just afterwards: > > if (!ready) fepriv->auto_sub_step++; > > Simplify the code by simply setting it to zero and use > continue to return to the while loop. > > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2") Oh wow, back to the big-bang-commit ^^' So is this a bug or not? It seems to me that the uint underflows to UINT_MAX, and then wrapps around to 0 again through the ++.. I take the liberty of ++CCing Kees, since I heard him talk a lot about overflowing on Plumbers. If it's not a bug, I would not use "Fixes". If it is a bug, it should be backported to stable, agreed? Plus, is there a report-link somewhere by Coverty that could be linked with "Closes: "? > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org> Anyways, this in my eyes does what it's intended to do: Reviewed-by: Philipp Stanner <pstanner@redhat.com> > --- > drivers/media/dvb-core/dvb_frontend.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/media/dvb-core/dvb_frontend.c > b/drivers/media/dvb-core/dvb_frontend.c > index d48f48fda87c..c9283100332a 100644 > --- a/drivers/media/dvb-core/dvb_frontend.c > +++ b/drivers/media/dvb-core/dvb_frontend.c > @@ -443,8 +443,8 @@ static int dvb_frontend_swzigzag_autotune(struct > dvb_frontend *fe, int check_wra > > default: > fepriv->auto_step++; > - fepriv->auto_sub_step = -1; /* it'll be > incremented to 0 in a moment */ > - break; > + fepriv->auto_sub_step = 0; > + continue; > } > > if (!ready) fepriv->auto_sub_step++;
On October 18, 2024 4:44:20 AM PDT, Philipp Stanner <pstanner@redhat.com> wrote: >On Fri, 2024-10-18 at 07:53 +0200, Mauro Carvalho Chehab wrote: >> fepriv->auto_sub_step is unsigned. Setting it to -1 is just a >> trick to avoid calling continue, as reported by Coverity. >> >> It relies to have this code just afterwards: >> >> if (!ready) fepriv->auto_sub_step++; >> >> Simplify the code by simply setting it to zero and use >> continue to return to the while loop. >> >> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2") > >Oh wow, back to the big-bang-commit ^^' > >So is this a bug or not? It seems to me that the uint underflows to >UINT_MAX, and then wrapps around to 0 again through the ++.. > >I take the liberty of ++CCing Kees, since I heard him talk a lot about >overflowing on Plumbers. > >If it's not a bug, I would not use "Fixes". If it is a bug, it should >be backported to stable, agreed? > >Plus, is there a report-link somewhere by Coverty that could be linked >with "Closes: "? Yeah, this is "avoid currently harmless overflow" fix. It is just avoiding depending on the wrapping behavior, which is an improvement but not really a "bug fix"; more a code style that will keep future work of making the kernel wrapping-safe. > >> Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org> > >Anyways, this in my eyes does what it's intended to do: > >Reviewed-by: Philipp Stanner <pstanner@redhat.com> > >> --- >> drivers/media/dvb-core/dvb_frontend.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/media/dvb-core/dvb_frontend.c >> b/drivers/media/dvb-core/dvb_frontend.c >> index d48f48fda87c..c9283100332a 100644 >> --- a/drivers/media/dvb-core/dvb_frontend.c >> +++ b/drivers/media/dvb-core/dvb_frontend.c >> @@ -443,8 +443,8 @@ static int dvb_frontend_swzigzag_autotune(struct >> dvb_frontend *fe, int check_wra >> >> default: >> fepriv->auto_step++; >> - fepriv->auto_sub_step = -1; /* it'll be >> incremented to 0 in a moment */ >> - break; >> + fepriv->auto_sub_step = 0; >> + continue; >> } >> >> if (!ready) fepriv->auto_sub_step++; > But this change seems incomplete. The above line is no longer needed. And I actually think this could be refractored to avoid needing "ready" at all? -Kees
On Fri, 2024-10-18 at 07:37 -0700, Kees Cook wrote: > > > On October 18, 2024 4:44:20 AM PDT, Philipp Stanner > <pstanner@redhat.com> wrote: > > On Fri, 2024-10-18 at 07:53 +0200, Mauro Carvalho Chehab wrote: > > > fepriv->auto_sub_step is unsigned. Setting it to -1 is just a > > > trick to avoid calling continue, as reported by Coverity. > > > > > > It relies to have this code just afterwards: > > > > > > if (!ready) fepriv->auto_sub_step++; > > > > > > Simplify the code by simply setting it to zero and use > > > continue to return to the while loop. > > > > > > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2") > > > > Oh wow, back to the big-bang-commit ^^' > > > > So is this a bug or not? It seems to me that the uint underflows to > > UINT_MAX, and then wrapps around to 0 again through the ++.. > > > > I take the liberty of ++CCing Kees, since I heard him talk a lot > > about > > overflowing on Plumbers. > > > > If it's not a bug, I would not use "Fixes". If it is a bug, it > > should > > be backported to stable, agreed? > > > > Plus, is there a report-link somewhere by Coverty that could be > > linked > > with "Closes: "? > > Yeah, this is "avoid currently harmless overflow" fix. It is just > avoiding depending on the wrapping behavior, which is an improvement > but not really a "bug fix"; more a code style that will keep future > work of making the kernel wrapping-safe. Alright, then it shouldn't be backported, ack? So I'd drop "Fixes:" > > > > > > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org> > > > > Anyways, this in my eyes does what it's intended to do: > > > > Reviewed-by: Philipp Stanner <pstanner@redhat.com> > > > > > --- > > > drivers/media/dvb-core/dvb_frontend.c | 4 ++-- > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/media/dvb-core/dvb_frontend.c > > > b/drivers/media/dvb-core/dvb_frontend.c > > > index d48f48fda87c..c9283100332a 100644 > > > --- a/drivers/media/dvb-core/dvb_frontend.c > > > +++ b/drivers/media/dvb-core/dvb_frontend.c > > > @@ -443,8 +443,8 @@ static int > > > dvb_frontend_swzigzag_autotune(struct > > > dvb_frontend *fe, int check_wra > > > > > > default: > > > fepriv->auto_step++; > > > - fepriv->auto_sub_step = -1; /* it'll be > > > incremented to 0 in a moment */ > > > - break; > > > + fepriv->auto_sub_step = 0; > > > + continue; > > > } > > > > > > if (!ready) fepriv->auto_sub_step++; > > > > But this change seems incomplete. The above line is no longer needed. I haven't super duper intensively reviewed it, but wouldn't make that statement – all the other branches in the switch-case reach this line. And auto_sub_step might be changed above in the if-check again if lnb_drift has changed; and it is changed in the switch-case. > > And I actually think this could be refractored to avoid needing > "ready" at all? Could be. But that'd be indeed some work to get it right without introducing a subtle bug, and Mauro just seems to want to fix a warning he encountered on the way. Thx P. > > -Kees >
Em Fri, 18 Oct 2024 07:37:52 -0700 Kees Cook <kees@kernel.org> escreveu: > On October 18, 2024 4:44:20 AM PDT, Philipp Stanner <pstanner@redhat.com> wrote: > >On Fri, 2024-10-18 at 07:53 +0200, Mauro Carvalho Chehab wrote: > >> fepriv->auto_sub_step is unsigned. Setting it to -1 is just a > >> trick to avoid calling continue, as reported by Coverity. > >> > >> It relies to have this code just afterwards: > >> > >> if (!ready) fepriv->auto_sub_step++; > >> > >> Simplify the code by simply setting it to zero and use > >> continue to return to the while loop. > >> > >> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2") > > > >Oh wow, back to the big-bang-commit ^^' > > > >So is this a bug or not? It seems to me that the uint underflows to > >UINT_MAX, and then wrapps around to 0 again through the ++.. > > > >I take the liberty of ++CCing Kees, since I heard him talk a lot about > >overflowing on Plumbers. > > > >If it's not a bug, I would not use "Fixes". If it is a bug, it should > >be backported to stable, agreed? There is a long thread about Fixes: tag at ksummit ML. https://lore.kernel.org/all/20240714192914.1e1d3448@gandalf.local.home/T/ My conclusions for it is that: 1. Fixes: != Cc: stable. This is even somewhat stated at Documentation/process/stable-kernel-rules.rst when it defines additional rules for Cc: stable; 2. As result of (1), all Cc: stable need fixes, but not all fixes: need a Cc: stable. Btw, I double-checked it with a -stable maintainer (Greg); 3. It seems that most of people at ksummit discussion (including me) use Fixes: when the patch is not doing an improvement. > >Plus, is there a report-link somewhere by Coverty that could be linked > >with "Closes: "? Coverity issues are not publicly visible (and IMO it shouldn't). We should not add closes: to something that only the ones with access to it may see. > Yeah, this is "avoid currently harmless overflow" fix. It is just avoiding depending on the wrapping behavior, which is an improvement but not really a "bug fix"; more a code style that will keep future work of making the kernel wrapping-safe. It is a fix in the sense that it solves an issue reported by Coverity. > >> if (!ready) fepriv->auto_sub_step++; > > > > But this change seems incomplete. The above line is no longer needed. Yes, this is now a dead code. > And I actually think this could be refractored to avoid needing "ready" at all? Yeah, it sounds a good idea to place the zig-zag drift calculus on a separate function, doing some cleanups in the process. I'll add it to my todo list. Thanks, Mauro
diff --git a/drivers/media/dvb-core/dvb_frontend.c b/drivers/media/dvb-core/dvb_frontend.c index d48f48fda87c..c9283100332a 100644 --- a/drivers/media/dvb-core/dvb_frontend.c +++ b/drivers/media/dvb-core/dvb_frontend.c @@ -443,8 +443,8 @@ static int dvb_frontend_swzigzag_autotune(struct dvb_frontend *fe, int check_wra default: fepriv->auto_step++; - fepriv->auto_sub_step = -1; /* it'll be incremented to 0 in a moment */ - break; + fepriv->auto_sub_step = 0; + continue; } if (!ready) fepriv->auto_sub_step++;
fepriv->auto_sub_step is unsigned. Setting it to -1 is just a trick to avoid calling continue, as reported by Coverity. It relies to have this code just afterwards: if (!ready) fepriv->auto_sub_step++; Simplify the code by simply setting it to zero and use continue to return to the while loop. Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2") Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org> --- drivers/media/dvb-core/dvb_frontend.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)