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: <ZYL3Sg85PyTOvjiW@smile.fi.intel.com>
Date: Wed, 20 Dec 2023 16:16:43 +0200
From: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To: Qu Wenruo <wqu@...e.com>
Cc: Alexey Dobriyan <adobriyan@...il.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	linux-btrfs@...r.kernel.org,
	Christophe JAILLET <christophe.jaillet@...adoo.fr>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/2] lib/strtox: introduce kstrtoull_suffix() helper

On Wed, Dec 20, 2023 at 08:31:09PM +1030, Qu Wenruo wrote:
> On 2023/12/20 20:24, Alexey Dobriyan wrote:
> > > Just as mentioned in the comment of memparse(), the simple_stroull()
> > > usage can lead to overflow all by itself.
> > 
> > which is the root cause...
> > 
> > I don't like one char suffixes. They are easy to integrate but then the
> > _real_ suffixes are "MiB", "GiB", etc.
> > 
> > If you care only about memparse(), then using _parse_integer() can be
> > arranged. I don't see why not.
> 
> Well, personally speaking I don't think we should even support the suffix at
> all, at least for the only two usage inside btrfs.
> 
> But unfortunately I'm not the one to do the final call, and the final call
> is to keep the suffix behavior...
> 
> And indeed using _parse_integer() with _parse_interger_fixup_radix() would
> be better, as we don't need to extend the _kstrtoull() code base.


My comment on the first patch got vanished due to my MTA issues, but I'll try
to summarize my point here.


First of all, I do not like the naming, it's too vague. What kind of suffix?
Do we suppose to have suffix in the input? What will be the behaviour w/o
suffix?  And so on...

Second, if it's a problem in memparse(), just fix it and that's all.

Third, as Alexey said, we have metric and byte suffixes and they are different.
Supporting one without the other is just adding to the existing confusion.

Last, but not least, we do NOT accept new code in the lib/ without test cases.

So, that said here is my formal NAK for this series (at least in this form).

P.S> The Subject should start with either kstrtox: or lib/kstrtox.c.

-- 
With Best Regards,
Andy Shevchenko



Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ