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 15:36:22 -0700
From:   Andrew Morton <akpm@linux-foundation.org>
To:     Alexey Dobriyan <adobriyan@gmail.com>
Cc:     linux-kernel@vger.kernel.org
Subject: Re: [PATCH] fix mult_frac() multiple argument evaluation bug
Message-Id: <20230519153622.f95f74819ecf467c5291de7d@linux-foundation.org>
In-Reply-To: <f522ad25-f899-4526-abc4-da35868b6a8b@p183>
References: <f522ad25-f899-4526-abc4-da35868b6a8b@p183>
Mime-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

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