Re: [PATCH RFC 5/5] kdbus: improve tests on incrementing quota

[Djalal Harouni <tixxdz@opendz.org>]

Hi Sergei,

On Thu, Jul 02, 2015 at 04:45:00PM +0300, Sergei Zviagintsev wrote:
> Hi David,
> 
> Thank you for reviewing and providing comments on these all! I answered below.
> 
> On Thu, Jul 02, 2015 at 10:50:47AM +0200, David Herrmann wrote:
> > Hi
> > 
> > On Sun, Jun 28, 2015 at 3:17 PM, Sergei Zviagintsev <sergei@s15v.net> wrote:
> > >  1) Rewrite
> > >
> > >         quota->memory + memory > U32_MAX
> > >
> > >     as
> > >         U32_MAX - quota->memory < memory
> > >
> > >     and provide the comment on why we need that check.
> > >
> > >     We have no overflow issue in the original expression when size_t is
> > >     32-bit because the previous one (available - quota->memory < memory)
> > >     guarantees that quota->memory + memory doesn't exceed `available'
> > >     which is <= U32_MAX in that case.
> > >
> > >     But lets stay explicit rather than implicit, it would save us from
> > >     describing HOW the code works.
> > >
> > >  2) Add WARN_ON when quota->msgs > KDBUS_CONN_MAX_MSGS
> > >
> > >     This is somewhat inconsistent, so we need to properly report it.
> > 
> > I don't see the purpose of this WARN_ON(). Sure, ">" should never
> > happen, but that doesn't mean we have to add a WARN_ON. I'd just keep
> > the code as it is.
> 
> I agree on WARN_ON. The intention of this change was to provide
> consistency. Current code checks for 'quota->msgs > KDBUS_CONN_MAX_MSGS'
> having '>=' test. If this ever happens, it means that we have a bug, but
> silently ignore it.
> 
> If we agree that '>' case should never happen, isn't it better to
> place '==' instead of '>=' in the original test?
> 
> > 
> > >  3) Replace
> > >
> > >         quota->fds + fds < quota->fds ||
> > >         quota->fds + fds > KDBUS_CONN_MAX_FDS_PER_USER
> > >
> > >     with
> > >
> > >         KDBUS_CONN_MAX_FDS_PER_USER - quota->fds < fds
> > >
> > >     and add explicit WARN_ON in the case
> > >     quota->fds > KDBUS_CONN_MAX_FDS_PER_USER.
> > >
> > >     Reading the code one can assume that the first expression is
> > >     there to ensure that we won't have an overflow in quota->fds after
> > >     quota->fds += fds, but what it really does is testing for size_t
> > >     overflow in `quota->fds + fds' to be safe in the second expression
> > >     (as fds is size_t, quota->fds is converted to bigger type).
> > >
> > >     Rewrite it in more obvious way. KDBUS_CONN_MAX_FDS_PER_USER is
> > >     checked at compile time to fill in quota->fds type (there is
> > >     BUILD_BUG_ON), so no further checks for quota->fds overflow are
> > >     needed.
> > >
> > > Signed-off-by: Sergei Zviagintsev <sergei@s15v.net>
> > > ---
> > >  ipc/kdbus/connection.c | 18 +++++++++++++-----
> > >  1 file changed, 13 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/ipc/kdbus/connection.c b/ipc/kdbus/connection.c
> > > index 12e32de310f5..6556a0f9d44c 100644
> > > --- a/ipc/kdbus/connection.c
> > > +++ b/ipc/kdbus/connection.c
> > > @@ -701,13 +701,21 @@ int kdbus_conn_quota_inc(struct kdbus_conn *c, struct kdbus_user *u,
> > >         available = (available - accounted + quota->memory) / 3;
> > >
> > >         if (available < quota->memory ||
> > > -           available - quota->memory < memory ||
> > > -           quota->memory + memory > U32_MAX)
> > > +           available - quota->memory < memory)
> > >                 return -ENOBUFS;
> > > -       if (quota->msgs >= KDBUS_CONN_MAX_MSGS)
> > > +
> > > +       /*
> > > +        * available is size_t and thus it could be greater than U32_MAX.
> > > +        * Ensure that quota->memory won't overflow.
> > > +        */
> > > +       if (U32_MAX - quota->memory < memory)
> > > +               return -ENOBUFS;
> > 
> > Can you drop the comment and integrate it into the condition above? I
> > mean this whole section is about overflow checks, I don't see the
> > point of explaining one of them specially.
> 
> 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 :-) , 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
;-)

But later we had to optimize pool allocation and other kdbus paths for
performance reasons, some future changes may affect this code path...
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.

Thank you!

-- 
Djalal Harouni
http://opendz.org