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

[Sergei Zviagintsev <sergei@s15v.net>]

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?

> 
> > +
> > +       if (WARN_ON(quota->msgs > KDBUS_CONN_MAX_MSGS) ||
> > +           quota->msgs == KDBUS_CONN_MAX_MSGS)
> >                 return -ENOBUFS;
> 
> This one I'd keep as it was. I don't really see the point in adding a WARN_ON().

I've addressed this above.

> 
> > -       if (quota->fds + fds < quota->fds ||
> > -           quota->fds + fds > KDBUS_CONN_MAX_FDS_PER_USER)
> > +       if (WARN_ON(quota->fds > KDBUS_CONN_MAX_FDS_PER_USER) ||
> > +           KDBUS_CONN_MAX_FDS_PER_USER - quota->fds < fds)
> >                 return -EMFILE;
> 
> Not sure the WARN_ON is needed, but this one looks fine to me.

I have the same question here as in the first WARN_ON issue above. If we
drop WARN_ON, shouldn't we drop the whole 'quota->fds >
KDBUS_CONN_MAX_FDS_PER_USER' test, assuming that it would never happen?
Because if we drop WARN_ON but leave the test, it would look ambiguous
as we check for a bug, but do not address it with some bug reporting
code.

> 
> Thanks
> David
> 
> >         quota->memory += memory;
> > --
> > 1.8.3.1
> >