Message-ID: <20150702174725.GV17917@localhost.localdomain>
Date:	Thu, 2 Jul 2015 20:47:25 +0300
From:	Sergei Zviagintsev <sergei@...v.net>
To:	Djalal Harouni <tixxdz@...ndz.org>
Cc:	David Herrmann <dh.herrmann@...il.com>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Daniel Mack <daniel@...que.org>,
	David Herrmann <dh.herrmann@...glemail.com>,
	linux-kernel <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH RFC 5/5] kdbus: improve tests on incrementing quota

Hi Djalal,

On Thu, Jul 02, 2015 at 03:13:41PM +0100, Djalal Harouni wrote:
[...]
> > My journey with this piece of code began from spotting and immediately
> > "fixing" the overflow issue :) Then I decided to dig into the
> > out-of-tree repo to find the origin of this line. What I found were
> > commits af8e2f750985 and ac5c385cc67a in which Djalal "fixed" it as
> > well, but then reverted back to the original code.
> > 
> > Surely we can drop this explanation, but if one of kdbus maintainers
> > experienced difficulties in understanding this piece of code, wouldn't
> > one who sees this code in the first time have the same issues?
> Yes there was lot of work in this area to make sure that the quota
> accounting is correct! the previous commits the one that tried to clean
> things up and the revert were both correct :-) ,

I cannot agree that commit af8e2f750985 in out-of-tree repo was correct.
Lets see, it replaces

     (a) available - quota->memory < memory ||
     (b) quota->memory + memory > U32_MAX

with

     (a) available - quota->memory < memory ||
     (c) quota->memory + memory < quota->memory

Keeping in mind that quota->memory is u32, memory is size_t and
sizeof(size_t) >= sizeof(u32), type convertions in these expressions
would be as following:

    (b1) (size_t)quota->memory + (size_t)memory > (size_t)U32_MAX
    (c1) (size_t)quota->memory + (size_t)memory < (size_t)quota->memory

We have two cases here:

1) size_t is 64-bit

   In this case (b) checks for u32 overflow in quota->memory + memory,
   *but* (c) checks for size_t overflow.

2) size_t is 32-bit

   (b) wouldn't work in the case of overflow, but (a) prevents that
   overflow as available <= U32_MAX. (c) checks for u32 overflow in
   quota->memory + memory, but it's redundant as (a) does all the job.

So we see that after that change in af8e2f750985, if on 64-bit we
have available == U32_MAX + 2 and quota->memory + memory == U32_MAX + 1,
then we would have wrong data in quota->memory as neither of tests
protects it from overflow.

> there were guards
> before this code path in the pool and slice allocation that prevented
> the code to overflow, see the commit logs af8e2f750985 of ac5c385cc67a
> ;-)

I agree, but we are talking about one particular test in
kdbus_conn_quota_inc(), which should handle any kind of input data.

> 
> But later we had to optimize pool allocation and other kdbus paths for
> performance reasons, some future changes may affect this code path...

... and if, as you say, code around change, then we are in trouble in
kdbus_conn_quota_inc() with that wrong overflow test.

> So yeh it will be safer to keep the overflow check. For the comment yeh
> it is not needed since that whole section is for overflow checks.

I am returning to my original argument here. If we have tricky
expression which already led to mistake (as discussed above) lets
provide an explanation of it and prevent future mistakes from being
made!

> 
> Thank you!
> 
> -- 
> Djalal Harouni
> http://opendz.org
--
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/

Powered by blists - more mailing lists