Re: [PATCH v3 2/4] lib/string_helpers.c: protect string_get_size() against blk_size=0

[James Bottomley <jbottomley@odin.com>]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 2258 bytes --]

On Fri, 2015-10-30 at 11:41 +0100, Vitaly Kuznetsov wrote:
> James Bottomley <jbottomley@odin.com> writes:
> 
> > On Fri, 2015-10-30 at 01:32 +0200, Andy Shevchenko wrote:
> >> On Fri, Oct 30, 2015 at 1:00 AM, James Bottomley <jbottomley@odin.com> wrote:
> >> > On Thu, 2015-10-29 at 17:30 +0100, Vitaly Kuznetsov wrote:
> >> >> Division by zero happens if blk_size=0 is supplied to string_get_size().
> >> >> Add WARN_ON() and set size to 0 to report '0 B'.
> >> >>
> >> >> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> >> >> ---
> >> >>  lib/string_helpers.c | 5 +++++
> >> >>  1 file changed, 5 insertions(+)
> >> >>
> >> >> diff --git a/lib/string_helpers.c b/lib/string_helpers.c
> >> >> index f6c27dc..ff3575b 100644
> >> >> --- a/lib/string_helpers.c
> >> >> +++ b/lib/string_helpers.c
> >> >> @@ -50,6 +50,11 @@ void string_get_size(u64 size, u32 blk_size, const enum string_size_units units,
> >> >>
> >> >>       tmp[0] = '\0';
> >> >>       i = 0;
> >> >> +
> >> >> +     /* Calling string_get_size() with blk_size=0 is wrong! */
> >> >> +     if (WARN_ON(!blk_size))
> >> >
> >> > Get rid of the WARN_ON; it's the standard thing to do for a partially
> >> > connected device.  Seeing zero is standard in a whole variety of
> >> > situations.  SCSI shims the zero but most other drivers don't.
> >> 
> >> For *block* size? It will crash the kernel. I've checked, it wasn't
> >> changed from the beginning (b9f28d863594).
> >
> > The standard signal for a drive error in capacity is zero size and zero
> > block size.  We have to take that case as standard without emitting
> > scary warnings.
> 
> Ok, but what if size != 0? Is WARN_ON() justified in this case?

It's an arithmentic routine whose job is to multiply two numbers, not
second guess the subsystem that gave it the numbers.  Just on general
architectural principles the only time it's allowed to dump a stack
trace without confusing people is when the arithmetic operation it has
been asked to do would produce an illegal result (which for two sixty
four bit numbers multiplying to a 128 bit one is never).

James

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥