linux-kernel - Re: [PATCH v1 1/2] lib/test_bitmap: Correct test data offsets for 32-bit

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20200109102722.GL32742@smile.fi.intel.com>
Date:   Thu, 9 Jan 2020 12:27:22 +0200
From:   Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To:     Yury Norov <yury.norov@...il.com>
Cc:     Rasmus Villemoes <linux@...musvillemoes.dk>,
        linux-kernel@...r.kernel.org,
        Andrew Morton <akpm@...ux-foundation.org>,
        Guenter Roeck <linux@...ck-us.net>
Subject: Re: [PATCH v1 1/2] lib/test_bitmap: Correct test data offsets for
 32-bit

On Wed, Jan 08, 2020 at 12:43:07PM -0800, Yury Norov wrote:
> On Wed, Jan 08, 2020 at 10:26:54PM +0200, Andy Shevchenko wrote:
> > On Wed, Jan 08, 2020 at 11:24:37AM -0800, Yury Norov wrote:
> > > On Wed, Jan 08, 2020 at 08:46:10PM +0200, Andy Shevchenko wrote:
> > > > On 32-bit platform the size of long is only 32 bits which makes wrong offset
> > > > in the array of 64 bit size.
> > > > 
> > > > Calculate offset based on BITS_PER_LONG.
> > > > 
> > > > Fixes: 30544ed5de43 ("lib/bitmap: introduce bitmap_replace() helper")
> > > > Reported-by: Guenter Roeck <linux@...ck-us.net>
> > > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
> > 
> > > >  	unsigned int nbits = 64;
> > > > +	unsigned int step = DIV_ROUND_UP(nbits, BITS_PER_LONG);
> > > 
> > > Step is already defined in this file:
> > >         #define step (sizeof(u64) / sizeof(unsigned long))
> > 
> > ...and later undefined.
> > 
> > > to avoid the same problem in other test cases. Introducing another variant of 
> > > it looks messy.
> > 
> > I don't see any problem.
> 
> The problem is that you reimplement the functionality instead of
> reuse.

I may not reuse by the reason I mentioned above. The definition of step works
only for 64 bits, we may modify test case for any amount of bits to be tested.

I'll rename the variable to something else to reduce confusion.

>  
> > > >  	DECLARE_BITMAP(bmap, 1024);
> > > >  
> > > >  	bitmap_zero(bmap, 1024);
> > > > -	bitmap_replace(bmap, &exp2[0], &exp2[1], exp2_to_exp3_mask, nbits);
> > > > +	bitmap_replace(bmap, &exp2[0 * step], &exp2[1 * step], exp2_to_exp3_mask, nbits);
> > > >  	expect_eq_bitmap(bmap, exp3_0_1, nbits);
> > > 
> > > If nbits is always 64, why don't you pass 64 directly?
> > 
> > We may use any setting. For now it's 64, but nothing prevents us to extend to,
> > let's say, 75.
> > 
> > -- 
> > With Best Regards,
> > Andy Shevchenko
> > 

-- 
With Best Regards,
Andy Shevchenko