| Message ID | 93ec19d1-3b74-4644-9f67-b88c08e79752@kili.mountain (mailing list archive) |
|---|---|
| State | Under Review |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | counter: rz-mtu3-cnt: Unlock on error in rz_mtu3_count_write() | expand |
On Wed, Apr 19, 2023 at 05:23:55PM +0300, Dan Carpenter wrote: > The return -ERANGE error paths need to call mutex_unlock(&priv->lock); > before returning. > > Fixes: 25d21447d896 ("counter: Add Renesas RZ/G2L MTU3a counter driver") > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> > --- > drivers/counter/rz-mtu3-cnt.c | 20 ++++++++++++-------- > 1 file changed, 12 insertions(+), 8 deletions(-) > > diff --git a/drivers/counter/rz-mtu3-cnt.c b/drivers/counter/rz-mtu3-cnt.c > index a371bab68499..aeadce5e2853 100644 > --- a/drivers/counter/rz-mtu3-cnt.c > +++ b/drivers/counter/rz-mtu3-cnt.c > @@ -358,19 +358,23 @@ static int rz_mtu3_count_ceiling_write(struct counter_device *counter, > switch (count->id) { > case RZ_MTU3_16_BIT_MTU1_CH: > case RZ_MTU3_16_BIT_MTU2_CH: > - if (ceiling > U16_MAX) > - return -ERANGE; > + if (ceiling > U16_MAX) { > + ret = -ERANGE; > + goto unlock; > + } > priv->mtu_16bit_max[ch_id] = ceiling; > break; > case RZ_MTU3_32_BIT_CH: > - if (ceiling > U32_MAX) > - return -ERANGE; > + if (ceiling > U32_MAX) { > + ret = -ERANGE; > + goto unlock; > + } > priv->mtu_32bit_max = ceiling; > break; > default: > /* should never reach this path */ > - mutex_unlock(&priv->lock); > - return -EINVAL; > + ret = -EINVAL; > + goto unlock; > } > > pm_runtime_get_sync(ch->dev); > @@ -381,9 +385,9 @@ static int rz_mtu3_count_ceiling_write(struct counter_device *counter, > > rz_mtu3_8bit_ch_write(ch, RZ_MTU3_TCR, RZ_MTU3_TCR_CCLR_TGRA); > pm_runtime_put(ch->dev); > +unlock: > mutex_unlock(&priv->lock); > - > - return 0; > + return ret; > } > > static void rz_mtu3_32bit_cnt_setting(struct counter_device *counter) > -- > 2.39.2 > The lock is acquired by rz_mtu3_lock_if_counter_is_valid(), so that function needs a sparse __acquires(&priv->lock) annotation too. William Breathitt Gray
On Wed, Apr 19, 2023 at 06:37:05PM +0300, Dan Carpenter wrote: > On Wed, Apr 19, 2023 at 10:36:49AM -0400, William Breathitt Gray wrote: > > The lock is acquired by rz_mtu3_lock_if_counter_is_valid(), so that > > function needs a sparse __acquires(&priv->lock) annotation too. > > I found this bug using Smatch. It's a competing static checker which > uses Sparse as a parser. I am the author of Smatch so I am naturally > biased. > > I don't think it's as simple as that. I don't think Sparse has > annotations for mutexes, only for spinlocks? Also it's really > complicated to annotate something as taking the lock on the success path > but not on the failure path. You have to set up a wrapper and use > __cond_lock(). > > Every other feature in Sparse is awesome, but for locking, it's better > to just use Smatch. > > regards, > dan carpenter Ah that's a fair point, I can see how involved that would be to set up a wrapper and handle the various paths correctly. The marginal benefit just doesn't seem worth the effort afterall. William Breathitt Gray
On Wed, Apr 19, 2023 at 10:36:49AM -0400, William Breathitt Gray wrote: > The lock is acquired by rz_mtu3_lock_if_counter_is_valid(), so that > function needs a sparse __acquires(&priv->lock) annotation too. I found this bug using Smatch. It's a competing static checker which uses Sparse as a parser. I am the author of Smatch so I am naturally biased. I don't think it's as simple as that. I don't think Sparse has annotations for mutexes, only for spinlocks? Also it's really complicated to annotate something as taking the lock on the success path but not on the failure path. You have to set up a wrapper and use __cond_lock(). Every other feature in Sparse is awesome, but for locking, it's better to just use Smatch. regards, dan carpenter
Hi Dan Carpenter, Thanks for the patch. > -----Original Message----- > From: Dan Carpenter <dan.carpenter@linaro.org> > Sent: Wednesday, April 19, 2023 3:24 PM > To: Biju Das <biju.das.jz@bp.renesas.com> > Cc: William Breathitt Gray <william.gray@linaro.org>; Lee Jones > <lee@kernel.org>; linux-iio@vger.kernel.org; linux-renesas- > soc@vger.kernel.org; kernel-janitors@vger.kernel.org > Subject: [PATCH] counter: rz-mtu3-cnt: Unlock on error in > rz_mtu3_count_write() > > The return -ERANGE error paths need to call mutex_unlock(&priv->lock); > before returning. > > Fixes: 25d21447d896 ("counter: Add Renesas RZ/G2L MTU3a counter driver") > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com> Cheers, Biju > --- > drivers/counter/rz-mtu3-cnt.c | 20 ++++++++++++-------- > 1 file changed, 12 insertions(+), 8 deletions(-) > > diff --git a/drivers/counter/rz-mtu3-cnt.c b/drivers/counter/rz-mtu3-cnt.c > index a371bab68499..aeadce5e2853 100644 > --- a/drivers/counter/rz-mtu3-cnt.c > +++ b/drivers/counter/rz-mtu3-cnt.c > @@ -358,19 +358,23 @@ static int rz_mtu3_count_ceiling_write(struct > counter_device *counter, > switch (count->id) { > case RZ_MTU3_16_BIT_MTU1_CH: > case RZ_MTU3_16_BIT_MTU2_CH: > - if (ceiling > U16_MAX) > - return -ERANGE; > + if (ceiling > U16_MAX) { > + ret = -ERANGE; > + goto unlock; > + } > priv->mtu_16bit_max[ch_id] = ceiling; > break; > case RZ_MTU3_32_BIT_CH: > - if (ceiling > U32_MAX) > - return -ERANGE; > + if (ceiling > U32_MAX) { > + ret = -ERANGE; > + goto unlock; > + } > priv->mtu_32bit_max = ceiling; > break; > default: > /* should never reach this path */ > - mutex_unlock(&priv->lock); > - return -EINVAL; > + ret = -EINVAL; > + goto unlock; > } > > pm_runtime_get_sync(ch->dev); > @@ -381,9 +385,9 @@ static int rz_mtu3_count_ceiling_write(struct > counter_device *counter, > > rz_mtu3_8bit_ch_write(ch, RZ_MTU3_TCR, RZ_MTU3_TCR_CCLR_TGRA); > pm_runtime_put(ch->dev); > +unlock: > mutex_unlock(&priv->lock); > - > - return 0; > + return ret; > } > > static void rz_mtu3_32bit_cnt_setting(struct counter_device *counter) > -- > 2.39.2
On Wed, Apr 19, 2023 at 05:23:55PM +0300, Dan Carpenter wrote: > The return -ERANGE error paths need to call mutex_unlock(&priv->lock); > before returning. > > Fixes: 25d21447d896 ("counter: Add Renesas RZ/G2L MTU3a counter driver") > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> The changes in this patch are for rz_mtu3_count_ceiling_write(), so the title of this patch should be fixed. > --- > drivers/counter/rz-mtu3-cnt.c | 20 ++++++++++++-------- > 1 file changed, 12 insertions(+), 8 deletions(-) > > diff --git a/drivers/counter/rz-mtu3-cnt.c b/drivers/counter/rz-mtu3-cnt.c > index a371bab68499..aeadce5e2853 100644 > --- a/drivers/counter/rz-mtu3-cnt.c > +++ b/drivers/counter/rz-mtu3-cnt.c > @@ -358,19 +358,23 @@ static int rz_mtu3_count_ceiling_write(struct counter_device *counter, > switch (count->id) { > case RZ_MTU3_16_BIT_MTU1_CH: > case RZ_MTU3_16_BIT_MTU2_CH: > - if (ceiling > U16_MAX) > - return -ERANGE; > + if (ceiling > U16_MAX) { > + ret = -ERANGE; > + goto unlock; > + } > priv->mtu_16bit_max[ch_id] = ceiling; > break; > case RZ_MTU3_32_BIT_CH: > - if (ceiling > U32_MAX) > - return -ERANGE; > + if (ceiling > U32_MAX) { > + ret = -ERANGE; > + goto unlock; > + } > priv->mtu_32bit_max = ceiling; > break; > default: > /* should never reach this path */ > - mutex_unlock(&priv->lock); > - return -EINVAL; > + ret = -EINVAL; > + goto unlock; > } Normally, I like "goto unlock" patterns, but I think in this context it makes the flow of code confusing with the mix-match of the switch cases; default case jumps with a goto, but RZ_MTU3_* cases passes will break, yet RZ_MUT3_* failures have a goto path. Rather than a "goto unlock" pattern, I'd prefer to see simply mutex_lock() called for these ceiling checks. That also has the benefit of reducing the number of changes we have to make for this fix. William Breathitt Gray > > pm_runtime_get_sync(ch->dev); > @@ -381,9 +385,9 @@ static int rz_mtu3_count_ceiling_write(struct counter_device *counter, > > rz_mtu3_8bit_ch_write(ch, RZ_MTU3_TCR, RZ_MTU3_TCR_CCLR_TGRA); > pm_runtime_put(ch->dev); > +unlock: > mutex_unlock(&priv->lock); > - > - return 0; > + return ret; > } > > static void rz_mtu3_32bit_cnt_setting(struct counter_device *counter) > -- > 2.39.2 >
On Thu, Apr 20, 2023 at 10:17:21AM -0400, William Breathitt Gray wrote: > On Wed, Apr 19, 2023 at 05:23:55PM +0300, Dan Carpenter wrote: > > The return -ERANGE error paths need to call mutex_unlock(&priv->lock); > > before returning. > > > > Fixes: 25d21447d896 ("counter: Add Renesas RZ/G2L MTU3a counter driver") > > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> > > The changes in this patch are for rz_mtu3_count_ceiling_write(), so the > title of this patch should be fixed. > Huh. That's weird... I don't know why I got that wrong. > > --- > > drivers/counter/rz-mtu3-cnt.c | 20 ++++++++++++-------- > > 1 file changed, 12 insertions(+), 8 deletions(-) > > > > diff --git a/drivers/counter/rz-mtu3-cnt.c b/drivers/counter/rz-mtu3-cnt.c > > index a371bab68499..aeadce5e2853 100644 > > --- a/drivers/counter/rz-mtu3-cnt.c > > +++ b/drivers/counter/rz-mtu3-cnt.c > > @@ -358,19 +358,23 @@ static int rz_mtu3_count_ceiling_write(struct counter_device *counter, > > switch (count->id) { > > case RZ_MTU3_16_BIT_MTU1_CH: > > case RZ_MTU3_16_BIT_MTU2_CH: > > - if (ceiling > U16_MAX) > > - return -ERANGE; > > + if (ceiling > U16_MAX) { > > + ret = -ERANGE; > > + goto unlock; > > + } > > priv->mtu_16bit_max[ch_id] = ceiling; > > break; > > case RZ_MTU3_32_BIT_CH: > > - if (ceiling > U32_MAX) > > - return -ERANGE; > > + if (ceiling > U32_MAX) { > > + ret = -ERANGE; > > + goto unlock; > > + } > > priv->mtu_32bit_max = ceiling; > > break; > > default: > > /* should never reach this path */ > > - mutex_unlock(&priv->lock); > > - return -EINVAL; > > + ret = -EINVAL; > > + goto unlock; > > } > > Normally, I like "goto unlock" patterns, but I think in this context it > makes the flow of code confusing with the mix-match of the switch cases; > default case jumps with a goto, but RZ_MTU3_* cases passes will break, > yet RZ_MUT3_* failures have a goto path. Rather than a "goto unlock" > pattern, I'd prefer to see simply mutex_lock() called for these ceiling > checks. That also has the benefit of reducing the number of changes we > have to make for this fix. Sure. I'll resend. regards, dan carpenter
diff --git a/drivers/counter/rz-mtu3-cnt.c b/drivers/counter/rz-mtu3-cnt.c index a371bab68499..aeadce5e2853 100644 --- a/drivers/counter/rz-mtu3-cnt.c +++ b/drivers/counter/rz-mtu3-cnt.c @@ -358,19 +358,23 @@ static int rz_mtu3_count_ceiling_write(struct counter_device *counter, switch (count->id) { case RZ_MTU3_16_BIT_MTU1_CH: case RZ_MTU3_16_BIT_MTU2_CH: - if (ceiling > U16_MAX) - return -ERANGE; + if (ceiling > U16_MAX) { + ret = -ERANGE; + goto unlock; + } priv->mtu_16bit_max[ch_id] = ceiling; break; case RZ_MTU3_32_BIT_CH: - if (ceiling > U32_MAX) - return -ERANGE; + if (ceiling > U32_MAX) { + ret = -ERANGE; + goto unlock; + } priv->mtu_32bit_max = ceiling; break; default: /* should never reach this path */ - mutex_unlock(&priv->lock); - return -EINVAL; + ret = -EINVAL; + goto unlock; } pm_runtime_get_sync(ch->dev); @@ -381,9 +385,9 @@ static int rz_mtu3_count_ceiling_write(struct counter_device *counter, rz_mtu3_8bit_ch_write(ch, RZ_MTU3_TCR, RZ_MTU3_TCR_CCLR_TGRA); pm_runtime_put(ch->dev); +unlock: mutex_unlock(&priv->lock); - - return 0; + return ret; } static void rz_mtu3_32bit_cnt_setting(struct counter_device *counter)
The return -ERANGE error paths need to call mutex_unlock(&priv->lock); before returning. Fixes: 25d21447d896 ("counter: Add Renesas RZ/G2L MTU3a counter driver") Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> --- drivers/counter/rz-mtu3-cnt.c | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-)