linux-kernel - Re: [PATCH v2 2/3] lib/string_helpers.c: don't lose precision in string_get

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] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 27 Oct 2015 18:24:16 +0200
From:	Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To:	Vitaly Kuznetsov <vkuznets@...hat.com>
Cc:	Andrew Morton <akpm@...ux-foundation.org>,
	Rasmus Villemoes <linux@...musvillemoes.dk>,
	Ulf Hansson <ulf.hansson@...aro.org>,
	James Bottomley <JBottomley@...n.com>,
	Kees Cook <keescook@...omium.org>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 2/3] lib/string_helpers.c: don't lose precision in
 string_get_size()

On Tue, 2015-10-27 at 17:16 +0100, Vitaly Kuznetsov wrote:
> Andy Shevchenko <andriy.shevchenko@...ux.intel.com> writes:

[]

> > +	if (!blk_size) {
> > > +		WARN_ON(1);
> > 
> > Hmm... Isn't it too strong? WARN_ONCE() might reduce a noise. Or
> > even
> > pr_warn_once/ratelimited().
> 
> Nobody is supposed to call string_get_size() with blk_size = 0, if
> someone does that - it is a bug and that's what WARN_ON is supposed
> to
> report. I'm OK with changing it to WARN_ONCE() but I don't see a big
> difference - nobody's calling string_get_size() in a loop, one/two
> calls
> per one storage device is expected.

I'm fine with WARN_ONCE() if there is no objection.

> 
> > 
> > > +		size = 0;
> > > +		goto out;
> > >  	}
> > 
> > What about doing it before if (!size) ?
> > 
> > Like 
> > 
> > if (!blk_size) {
> >  pr_warn_once(); /* or WARN_ONCE() ? */
> >  /* Override size to follow error path */
> >  size = 0;
> > }
> >  
> > if (!size)
> 
> To be honest I don't see a big difference but I'm fine with the
> change :-)

Maybe Rasmus can judge me.

> > > -	remainder %= divisor[units];
> > > +	remainder -= (remainder / divisor[units]) *
> > > divisor[units];
> > 
> > I'm sorry I didn't get what the purpose of change here.
> > 
> > (Yes, I was thinking about u64 on 32-bit architecture, but % and /
> > are
> > working in the similar way aren't they?)
> 
> Thanks for noticing, there is no functional change here, it just made
> the code easier to understand (for me only?). I'm OK with reverting
> it
> to '%='.

For me is the opposite. remainder = remainder % divisor[units] will
work as well, but why change the code at all.

-- 
Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Intel Finland Oy

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/