Message ID | Y2iHl5CuqyR2vEc8@qemulion |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a5d:6687:0:0:0:0:0 with SMTP id l7csp1825369wru; Sun, 6 Nov 2022 20:22:23 -0800 (PST) X-Google-Smtp-Source: AMsMyM7jj//4d1h2dcaqJbetoOf7DvsJ7JsvrsI6N/p/ljqGST7wQGkOTxGGMK9v8qP1APqvma9n X-Received: by 2002:a17:907:2c6b:b0:7ad:c587:bc5b with SMTP id ib11-20020a1709072c6b00b007adc587bc5bmr40737439ejc.425.1667794943345; Sun, 06 Nov 2022 20:22:23 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1667794943; cv=none; d=google.com; s=arc-20160816; b=QxA2Mk1CFjUCbFfOgMR6TR0pOmmgKiQC+GFnabfGjxe7kt0azxmK82PpSmdMbwyl/3 JFXLzMIC9Ly3IB/csZb6p+zu6j1s9G6xszUswc2GznTGnXi3idHE13zozB4Jc/jr60Zo b7LXkGWQTTdRh1mSxecfsBulBNSc1vhCblOJx/A1eWTtWwNYDDirboHZKWWbyzwBTguV 6A6MluxeufwT/Dl5BKDDbvdNPe+4T30q9rJJTUSpZPvLnGpwZjIK488sjUhq8BUeBRnN u7O/eC3wu1BKEwRrLKyjCIgnQ74i8Rq6ZNlFtPEo3R6MYQd+YQQ2Vc//mYe6tYo1JrA0 CsIQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-disposition:mime-version:message-id :subject:to:from:date:dkim-signature; bh=7889CEY3CmETelk4gC9j0mps6oNGWpqB91Jcx11D8p4=; b=ZxPhGv1HokS2MfvQdpb2VLRTUluHQY9+Hh10vKAypIk0m/hB+oGBhY/EJaJVLt61r+ O1ZsZeaTt2qMRDBINXf/DsRDzAfsdx1p20pB1BILUyPgY42eQVhOouhKdeQgSMDlwJTN ePqOoTqdRqGUHt1QeHrEd7Clq9Zrva/Hm3pw3g3Xb/Ublu0DptQwf+SMsA/5+cMOQHJm Bp0b8lVPqfDxcpwbtuDbRee87tSokgUER1uulT19o1RNyley9gWOppp264iZtfRKAHjt qCF8oowL4+dZb8UDl1qfsu1aKqijLy78nGDRlTGkCPm0q73PrxQLeZJpo15sAv/ElYHn c0rg== ARC-Authentication-Results: i=1; mx.google.com; dkim=fail header.i=@mailo.com header.s=mailo header.b=lUxg3iWh; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=mailo.com Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id d21-20020a1709061f5500b0077d1df3967asi5369450ejk.563.2022.11.06.20.21.59; Sun, 06 Nov 2022 20:22:23 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; dkim=fail header.i=@mailo.com header.s=mailo header.b=lUxg3iWh; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=mailo.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S230364AbiKGEU4 (ORCPT <rfc822;hjfbswb@gmail.com> + 99 others); Sun, 6 Nov 2022 23:20:56 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:39148 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S230050AbiKGEUz (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Sun, 6 Nov 2022 23:20:55 -0500 Received: from msg-1.mailo.com (msg-1.mailo.com [213.182.54.11]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id DA0BBBF63 for <linux-kernel@vger.kernel.org>; Sun, 6 Nov 2022 20:20:53 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=mailo.com; s=mailo; t=1667794846; bh=K3l8aNi6tXs7C8Soq/7V0kdkH5J0anIcX/pV2o0Kziw=; h=X-EA-Auth:Date:From:To:Subject:Message-ID:MIME-Version: Content-Type; b=lUxg3iWhpHnG3ikfJSQzwYSovFfvAw3NpUdOynMm4xi2RiZmFyNGUi5hy7ppcVG6S gQe8Kk9wgKORPyOUmMC7+WgngxNcrPVTN9FX0eNoie+zyno9nGFONvEVLRzTyMWpsa RB4nXHCfFVwvYfArT7/i2v0CVsRbFNZjcWrVuLIA= Received: by b-4.in.mailobj.net [192.168.90.14] with ESMTP via ip-206.mailobj.net [213.182.55.206] Mon, 7 Nov 2022 05:20:46 +0100 (CET) X-EA-Auth: L+KOezgXoKLy0w2UXz8vniZWzgycWzMAhm0HNXPw+zfIJjT+A3gOmLt8vkjNrcji/Wnj8+y5HcJAcYaK21QUFxcJgWXobTLM Date: Mon, 7 Nov 2022 09:50:39 +0530 From: Deepak R Varma <drv@mailo.com> To: Greg Kroah-Hartman <gregkh@linuxfoundation.org>, linux-staging@lists.linux.dev, linux-kernel@vger.kernel.org Subject: [PATCH v2] staging: most: video: use min_t() for comparison and assignment Message-ID: <Y2iHl5CuqyR2vEc8@qemulion> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,SPF_HELO_NONE,SPF_PASS autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1748808639479979231?= X-GMAIL-MSGID: =?utf-8?q?1748809750263346572?= |
Series |
[v2] staging: most: video: use min_t() for comparison and assignment
|
|
Commit Message
Deepak R Varma
Nov. 7, 2022, 4:20 a.m. UTC
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(-)
--
2.34.1
Comments
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
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 > >
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
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 > >
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");