diff mbox series

fix mult_frac() multiple argument evaluation bug

Message ID	f522ad25-f899-4526-abc4-da35868b6a8b@p183
State	New
Headers	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; Date: Fri, 19 May 2023 23:24:54 +0300 From: Alexey Dobriyan <adobriyan@gmail.com> To: akpm@linux-foundation.org Cc: linux-kernel@vger.kernel.org Subject: [PATCH] fix mult_frac() multiple argument evaluation bug Message-ID: <f522ad25-f899-4526-abc4-da35868b6a8b@p183> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Precedence: bulk
Series	fix mult_frac() multiple argument evaluation bug \| fix mult_frac() multiple argument evaluation bug

Commit Message

Alexey Dobriyan May 19, 2023, 8:24 p.m. UTC

  mult_frac() evaluates _all_ arguments multiple times in the body.

Clarify comment while I'm at it.

Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
---

 include/linux/math.h |   22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

Comments

Andrew Morton May 19, 2023, 10:36 p.m. UTC | #1

On Fri, 19 May 2023 23:24:54 +0300 Alexey Dobriyan <adobriyan@gmail.com> wrote:

> mult_frac() evaluates _all_ arguments multiple times in the body.
> 
> --- a/include/linux/math.h
> +++ b/include/linux/math.h
> @@ -118,17 +118,17 @@ __STRUCT_FRACT(s32)
>  __STRUCT_FRACT(u32)
>  #undef __STRUCT_FRACT
>  
> -/*
> - * Multiplies an integer by a fraction, while avoiding unnecessary
> - * overflow or loss of precision.
> - */
> -#define mult_frac(x, numer, denom)(			\
> -{							\
> -	typeof(x) quot = (x) / (denom);			\
> -	typeof(x) rem  = (x) % (denom);			\
> -	(quot * (numer)) + ((rem * (numer)) / (denom));	\
> -}							\
> -)
> +/* Calculate "x * n / d" without unnecessary overflow or loss of precision. */
> +#define mult_frac(x, n, d)	\
> +({				\
> +	typeof(x) x_ = (x);	\
> +	typeof(n) n_ = (n);	\
> +	typeof(d) d_ = (d);	\
> +				\
> +	typeof(x) q = x_ / d_;	\
> +	typeof(x) r = x_ % d_;	\
> +	q * n_ + r * n_ / d_;	\
> +})
>  

I like, but the compiler doesn't.  x86_64 allmodconfig, gcc-12.2.0.

In file included from ./include/linux/math64.h:6,
                 from ./include/linux/time.h:6,
                 from ./include/linux/ktime.h:24,
                 from ./include/linux/timer.h:6,
                 from drivers/target/target_core_iblock.c:16:
drivers/target/target_core_iblock.c: In function 'iblock_configure_device':
drivers/target/target_core_iblock.c:127:73: error: passing argument 1 of 'queue_max_hw_sectors' makes pointer from integer without a cast [-Werror=int-conversion]
  127 |         dev->dev_attrib.hw_max_sectors = mult_frac(queue_max_hw_sectors(q),
      |                                                                         ^
      |                                                                         |
      |                                                                         unsigned int
./include/linux/math.h:129:16: note: in definition of macro 'mult_frac'
  129 |         typeof(x) r = x_ % d_;  \
      |                ^
In file included from drivers/target/target_core_iblock.c:18:
./include/linux/blkdev.h:1112:77: note: expected 'const struct request_queue *' but argument is of type 'unsigned int'
 1112 | static inline unsigned int queue_max_hw_sectors(const struct request_queue *q)
      |                                                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~^
cc1: all warnings being treated as errors
make[3]: *** [scripts/Makefile.build:252: drivers/target/target_core_iblock.o] Error 1
make[3]: *** Waiting for unfinished jobs....
make[2]: *** [scripts/Makefile.build:494: drivers/target] Error 2
make[2]: *** Waiting for unfinished jobs....
make[1]: *** [scripts/Makefile.build:494: drivers] Error 2
make: *** [Makefile:2026: .] Error 2

Alexey Dobriyan May 20, 2023, 6:24 p.m. UTC | #2

On Fri, May 19, 2023 at 03:36:22PM -0700, Andrew Morton wrote:
> On Fri, 19 May 2023 23:24:54 +0300 Alexey Dobriyan <adobriyan@gmail.com> wrote:
> 
> > mult_frac() evaluates _all_ arguments multiple times in the body.
> > 
> > --- a/include/linux/math.h
> > +++ b/include/linux/math.h
> > @@ -118,17 +118,17 @@ __STRUCT_FRACT(s32)
> >  __STRUCT_FRACT(u32)
> >  #undef __STRUCT_FRACT
> >  
> > -/*
> > - * Multiplies an integer by a fraction, while avoiding unnecessary
> > - * overflow or loss of precision.
> > - */
> > -#define mult_frac(x, numer, denom)(			\
> > -{							\
> > -	typeof(x) quot = (x) / (denom);			\
> > -	typeof(x) rem  = (x) % (denom);			\
> > -	(quot * (numer)) + ((rem * (numer)) / (denom));	\
> > -}							\
> > -)
> > +/* Calculate "x * n / d" without unnecessary overflow or loss of precision. */
> > +#define mult_frac(x, n, d)	\
> > +({				\
> > +	typeof(x) x_ = (x);	\
> > +	typeof(n) n_ = (n);	\
> > +	typeof(d) d_ = (d);	\
> > +				\
> > +	typeof(x) q = x_ / d_;	\
> > +	typeof(x) r = x_ % d_;	\
> > +	q * n_ + r * n_ / d_;	\
> > +})
> >  
> 
> I like, but the compiler doesn't.  x86_64 allmodconfig, gcc-12.2.0.
> 
> In file included from ./include/linux/math64.h:6,
>                  from ./include/linux/time.h:6,
>                  from ./include/linux/ktime.h:24,
>                  from ./include/linux/timer.h:6,
>                  from drivers/target/target_core_iblock.c:16:
> drivers/target/target_core_iblock.c: In function 'iblock_configure_device':
> drivers/target/target_core_iblock.c:127:73: error: passing argument 1 of 'queue_max_hw_sectors' makes pointer from integer without a cast [-Werror=int-conversion]
>   127 |         dev->dev_attrib.hw_max_sectors = mult_frac(queue_max_hw_sectors(q),
>       |                                                                         ^
>       |                                                                         |
>       |                                                                         unsigned int
> ./include/linux/math.h:129:16: note: in definition of macro 'mult_frac'
>   129 |         typeof(x) r = x_ % d_;  \
>       |                ^
> In file included from drivers/target/target_core_iblock.c:18:
> ./include/linux/blkdev.h:1112:77: note: expected 'const struct request_queue *' but argument is of type 'unsigned int'
>  1112 | static inline unsigned int queue_max_hw_sectors(const struct request_queue *q)
>       |                                                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~^

In my defence, original macro has the same bug:

	#define M(a)			\
	({				\
		typeof(a) b = ...;	\
		typeof(a) c = ...;	\

Now, first typeof can use f(b) because second b doesn't exist yet.
But second typeof can not because second b already exists with potentially
incompatible type.

diff mbox series

Patch

--- a/include/linux/math.h
+++ b/include/linux/math.h
@@ -118,17 +118,17 @@  __STRUCT_FRACT(s32)
 __STRUCT_FRACT(u32)
 #undef __STRUCT_FRACT
 
-/*
- * Multiplies an integer by a fraction, while avoiding unnecessary
- * overflow or loss of precision.
- */
-#define mult_frac(x, numer, denom)(			\
-{							\
-	typeof(x) quot = (x) / (denom);			\
-	typeof(x) rem  = (x) % (denom);			\
-	(quot * (numer)) + ((rem * (numer)) / (denom));	\
-}							\
-)
+/* Calculate "x * n / d" without unnecessary overflow or loss of precision. */
+#define mult_frac(x, n, d)	\
+({				\
+	typeof(x) x_ = (x);	\
+	typeof(n) n_ = (n);	\
+	typeof(d) d_ = (d);	\
+				\
+	typeof(x) q = x_ / d_;	\
+	typeof(x) r = x_ % d_;	\
+	q * n_ + r * n_ / d_;	\
+})
 
 #define sector_div(a, b) do_div(a, b)