[PATCH v2] staging: most: video: use min_t() for comparison and assignment

[PATCH v2] staging: most: video: use min_t() for comparison and assignment

[Deepak R Varma]

Simplify code by using min_t helper macro for logical evaluation
and value assignment. Use the _t variant of min macro since the
variable types are not same.
This issue is identified by coccicheck using the minmax.cocci file.

Signed-off-by: Deepak R Varma <drv@mailo.com>
---

Changes in v2:
   1. Revise patch description. No functional change.

 drivers/staging/most/video/video.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/most/video/video.c b/drivers/staging/most/video/video.c
index ffa97ef21ea5..d5cc7eea3b52 100644
--- a/drivers/staging/most/video/video.c
+++ b/drivers/staging/most/video/video.c
@@ -173,7 +173,7 @@ static ssize_t comp_vdev_read(struct file *filp, char __user *buf,
 	while (count > 0 && data_ready(mdev)) {
 		struct mbo *const mbo = get_top_mbo(mdev);
 		int const rem = mbo->processed_length - fh->offs;
-		int const cnt = rem < count ? rem : count;
+		int const cnt = min_t(int, rem, count);

 		if (copy_to_user(buf, mbo->virt_address + fh->offs, cnt)) {
 			v4l2_err(&mdev->v4l2_dev, "read: copy_to_user failed\n");
--
2.34.1

Re: [PATCH v2] staging: most: video: use min_t() for comparison and assignment

[Dan Carpenter]

On Mon, Nov 07, 2022 at 09:50:39AM +0530, Deepak R Varma wrote:
> Simplify code by using min_t helper macro for logical evaluation
> and value assignment. Use the _t variant of min macro since the
> variable types are not same.
> This issue is identified by coccicheck using the minmax.cocci file.
> 
> Signed-off-by: Deepak R Varma <drv@mailo.com>
> ---
> 
> Changes in v2:
>    1. Revise patch description. No functional change.
> 
>  drivers/staging/most/video/video.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/most/video/video.c b/drivers/staging/most/video/video.c
> index ffa97ef21ea5..d5cc7eea3b52 100644
> --- a/drivers/staging/most/video/video.c
> +++ b/drivers/staging/most/video/video.c
> @@ -173,7 +173,7 @@ static ssize_t comp_vdev_read(struct file *filp, char __user *buf,
>  	while (count > 0 && data_ready(mdev)) {
>  		struct mbo *const mbo = get_top_mbo(mdev);
>  		int const rem = mbo->processed_length - fh->offs;
> -		int const cnt = rem < count ? rem : count;
> +		int const cnt = min_t(int, rem, count);

TL;DR use size_t instead of int.

Using "int" here is wrong.  size_t is unsigned long meaning that it has
64 bits to use to represent positive values.  (Let's ignore 32 bit
arches).  You have chopped it down to say that it now has 31 bits for
positives and if BIT(31) is set then treat it as negative.  Everything
which is larger than INT_MAX will be broken.

Fortunately, in this code the value of count will never go higher than
"INT_MAX - PAGE_SIZE" because Linus understands that it's easy to
introduce bugs like this.

regards,
dan carpenter

Re: [PATCH v2] staging: most: video: use min_t() for comparison and assignment

[Deepak R Varma]

On Mon, Nov 07, 2022 at 04:20:27PM +0300, Dan Carpenter wrote:
> On Mon, Nov 07, 2022 at 09:50:39AM +0530, Deepak R Varma wrote:
> > Simplify code by using min_t helper macro for logical evaluation
> > and value assignment. Use the _t variant of min macro since the
> > variable types are not same.
> > This issue is identified by coccicheck using the minmax.cocci file.
> >
> > Signed-off-by: Deepak R Varma <drv@mailo.com>
> > ---
> >
> > Changes in v2:
> >    1. Revise patch description. No functional change.
> >
> >  drivers/staging/most/video/video.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/staging/most/video/video.c b/drivers/staging/most/video/video.c
> > index ffa97ef21ea5..d5cc7eea3b52 100644
> > --- a/drivers/staging/most/video/video.c
> > +++ b/drivers/staging/most/video/video.c
> > @@ -173,7 +173,7 @@ static ssize_t comp_vdev_read(struct file *filp, char __user *buf,
> >  	while (count > 0 && data_ready(mdev)) {
> >  		struct mbo *const mbo = get_top_mbo(mdev);
> >  		int const rem = mbo->processed_length - fh->offs;
> > -		int const cnt = rem < count ? rem : count;
> > +		int const cnt = min_t(int, rem, count);
>
> TL;DR use size_t instead of int.

Hi Dan,
Thank you for reviewing the patch. Please see my queries inline.

>
> Using "int" here is wrong.  size_t is unsigned long meaning that it has
> 64 bits to use to represent positive values.  (Let's ignore 32 bit
> arches).  You have chopped it down to say that it now has 31 bits for
> positives and if BIT(31) is set then treat it as negative.  Everything
> which is larger than INT_MAX will be broken.

I did worry about the truncation int might cause to the size_t variable,
however, as the result is being assigned to an int, I decided to go for int to
be the typecast for min_t.

Also, won't size_t will force the int rem to be treated as unsigned value which
will impact the comparison when rem indeed is negative. If rem will never be
-ve, my worry will be void.

Can you please correct if my understanding is wrong?

Thanks,
./drv

>
> Fortunately, in this code the value of count will never go higher than
> "INT_MAX - PAGE_SIZE" because Linus understands that it's easy to
> introduce bugs like this.
>
> regards,
> dan carpenter
>
>

Re: [PATCH v2] staging: most: video: use min_t() for comparison and assignment

[Dan Carpenter]

On Mon, Nov 07, 2022 at 08:40:27PM +0530, Deepak R Varma wrote:
> On Mon, Nov 07, 2022 at 04:20:27PM +0300, Dan Carpenter wrote:
> > On Mon, Nov 07, 2022 at 09:50:39AM +0530, Deepak R Varma wrote:
> > > Simplify code by using min_t helper macro for logical evaluation
> > > and value assignment. Use the _t variant of min macro since the
> > > variable types are not same.
> > > This issue is identified by coccicheck using the minmax.cocci file.
> > >
> > > Signed-off-by: Deepak R Varma <drv@mailo.com>
> > > ---
> > >
> > > Changes in v2:
> > >    1. Revise patch description. No functional change.
> > >
> > >  drivers/staging/most/video/video.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/staging/most/video/video.c b/drivers/staging/most/video/video.c
> > > index ffa97ef21ea5..d5cc7eea3b52 100644
> > > --- a/drivers/staging/most/video/video.c
> > > +++ b/drivers/staging/most/video/video.c
> > > @@ -173,7 +173,7 @@ static ssize_t comp_vdev_read(struct file *filp, char __user *buf,
> > >  	while (count > 0 && data_ready(mdev)) {
> > >  		struct mbo *const mbo = get_top_mbo(mdev);
> > >  		int const rem = mbo->processed_length - fh->offs;
> > > -		int const cnt = rem < count ? rem : count;
> > > +		int const cnt = min_t(int, rem, count);
> >
> > TL;DR use size_t instead of int.
> 
> Hi Dan,
> Thank you for reviewing the patch. Please see my queries inline.
> 
> >
> > Using "int" here is wrong.  size_t is unsigned long meaning that it has
> > 64 bits to use to represent positive values.  (Let's ignore 32 bit
> > arches).  You have chopped it down to say that it now has 31 bits for
> > positives and if BIT(31) is set then treat it as negative.  Everything
> > which is larger than INT_MAX will be broken.
> 
> I did worry about the truncation int might cause to the size_t variable,
> however, as the result is being assigned to an int, I decided to go for int to
> be the typecast for min_t.

Let's ignore that other layers prevent "count" from being greater than
INT_MAX.  mbo->processed_length is a u16.  Also if "fh->offs" is more
than mbo->processed_length that's a separate bug and we are already
screwed.

So that means rem is a relatively small number.  A small number can
easily fit in "int cnt".  So we are eating a big pie ("count") but we
are taking small bites ("cnt").  Everything works fine.

But if we chop the pie in half or treat it as negative pie then the
math breaks.

> 
> Also, won't size_t will force the int rem to be treated as unsigned value which
> will impact the comparison when rem indeed is negative. If rem will never be
> -ve, my worry will be void.

Is "-ve" the TikTok way of abbreviating negative?  Am I old?

The small bites are always positive.  But if we are eating negative
pie then we take negative size bites.  min_t() should almost always use
unsigned types.  Everything else is a headache.  I have often wondered
why people do it but I think it's because of the 80 character rule and
the word "int" is shorter than "unsigned long".

regards,
dan carpenter

Re: [PATCH v2] staging: most: video: use min_t() for comparison and assignment

[Deepak R Varma]

On Mon, Nov 07, 2022 at 06:21:47PM +0300, Dan Carpenter wrote:
> On Mon, Nov 07, 2022 at 08:40:27PM +0530, Deepak R Varma wrote:
> > On Mon, Nov 07, 2022 at 04:20:27PM +0300, Dan Carpenter wrote:
> > > On Mon, Nov 07, 2022 at 09:50:39AM +0530, Deepak R Varma wrote:
> > > > Simplify code by using min_t helper macro for logical evaluation
> > > > and value assignment. Use the _t variant of min macro since the
> > > > variable types are not same.
> > > > This issue is identified by coccicheck using the minmax.cocci file.
> > > >
> > > > Signed-off-by: Deepak R Varma <drv@mailo.com>
> > > > ---
> > > >
> > > > Changes in v2:
> > > >    1. Revise patch description. No functional change.
> > > >
> > > >  drivers/staging/most/video/video.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/staging/most/video/video.c b/drivers/staging/most/video/video.c
> > > > index ffa97ef21ea5..d5cc7eea3b52 100644
> > > > --- a/drivers/staging/most/video/video.c
> > > > +++ b/drivers/staging/most/video/video.c
> > > > @@ -173,7 +173,7 @@ static ssize_t comp_vdev_read(struct file *filp, char __user *buf,
> > > >  	while (count > 0 && data_ready(mdev)) {
> > > >  		struct mbo *const mbo = get_top_mbo(mdev);
> > > >  		int const rem = mbo->processed_length - fh->offs;
> > > > -		int const cnt = rem < count ? rem : count;
> > > > +		int const cnt = min_t(int, rem, count);
> > >
> > > TL;DR use size_t instead of int.
> >
> > Hi Dan,
> > Thank you for reviewing the patch. Please see my queries inline.
> >
> > >
> > > Using "int" here is wrong.  size_t is unsigned long meaning that it has
> > > 64 bits to use to represent positive values.  (Let's ignore 32 bit
> > > arches).  You have chopped it down to say that it now has 31 bits for
> > > positives and if BIT(31) is set then treat it as negative.  Everything
> > > which is larger than INT_MAX will be broken.
> >
> > I did worry about the truncation int might cause to the size_t variable,
> > however, as the result is being assigned to an int, I decided to go for int to
> > be the typecast for min_t.
>
> Let's ignore that other layers prevent "count" from being greater than
> INT_MAX.  mbo->processed_length is a u16.  Also if "fh->offs" is more
> than mbo->processed_length that's a separate bug and we are already
> screwed.

Yes, "u16 - u32" looks wrong on the face, but it should always evaluat
positive, else we would have had other issues down the line.

>
> So that means rem is a relatively small number.  A small number can
> easily fit in "int cnt".  So we are eating a big pie ("count") but we
> are taking small bites ("cnt").  Everything works fine.
>
> But if we chop the pie in half or treat it as negative pie then the
> math breaks.

That makes perfect sense. Thank you so much Dan for the explanation.

>
> >
> > Also, won't size_t will force the int rem to be treated as unsigned value which
> > will impact the comparison when rem indeed is negative. If rem will never be
> > -ve, my worry will be void.
>
> Is "-ve" the TikTok way of abbreviating negative?  Am I old?

No no... sorry. My fingers were catching up with the thoughts. :)

>
> The small bites are always positive.  But if we are eating negative
> pie then we take negative size bites.  min_t() should almost always use
> unsigned types.  Everything else is a headache.  I have often wondered
> why people do it but I think it's because of the 80 character rule and
> the word "int" is shorter than "unsigned long".

Understood. I will send in a revision with size_t type for min_t.
Thank you once again for the detailed answer. Much appreciate.

./drv

>
> regards,
> dan carpenter
>
>