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:   Sat, 20 May 2023 21:24:07 +0300
From:   Alexey Dobriyan <adobriyan@gmail.com>
To:     Andrew Morton <akpm@linux-foundation.org>
Cc:     linux-kernel@vger.kernel.org
Subject: Re: [PATCH] fix mult_frac() multiple argument evaluation bug
Message-ID: <571c9de8-313a-49ed-b5e1-8d4948e062b0@p183>
References: <f522ad25-f899-4526-abc4-da35868b6a8b@p183>
 <20230519153622.f95f74819ecf467c5291de7d@linux-foundation.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20230519153622.f95f74819ecf467c5291de7d@linux-foundation.org>
Precedence: bulk

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.