Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1175839 > unrolled thread

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

Started byDavid Herrmann <dh.herrmann@gmail.com>
First post2015-07-02 11:00 +0200
Last post2015-07-05 10:30 +0200
Articles 5 — 3 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH RFC 5/5] kdbus: improve tests on incrementing quota David Herrmann <dh.herrmann@gmail.com> - 2015-07-02 11:00 +0200
    Re: [PATCH RFC 5/5] kdbus: improve tests on incrementing quota Sergei Zviagintsev <sergei@s15v.net> - 2015-07-02 15:50 +0200
      Re: [PATCH RFC 5/5] kdbus: improve tests on incrementing quota Djalal Harouni <tixxdz@opendz.org> - 2015-07-02 16:20 +0200
        Re: [PATCH RFC 5/5] kdbus: improve tests on incrementing quota David Herrmann <dh.herrmann@gmail.com> - 2015-07-05 11:10 +0200
      Re: [PATCH RFC 5/5] kdbus: improve tests on incrementing quota Sergei Zviagintsev <sergei@s15v.net> - 2015-07-05 10:30 +0200

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

FromDavid Herrmann <dh.herrmann@gmail.com>
Date2015-07-02 11:00 +0200
SubjectRe: [PATCH RFC 5/5] kdbus: improve tests on incrementing quota
Message-ID<pHIvE-qX-5@gated-at.bofh.it>
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.

>  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.

> +
> +       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().

> -       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.

Thanks
David

>         quota->memory += memory;
> --
> 1.8.3.1
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1175992

FromSergei Zviagintsev <sergei@s15v.net>
Date2015-07-02 15:50 +0200
Message-ID<pHN2i-3lz-17@gated-at.bofh.it>
In reply to#1175839
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
> >
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1176030

FromDjalal Harouni <tixxdz@opendz.org>
Date2015-07-02 16:20 +0200
Message-ID<pHNvl-3Pj-35@gated-at.bofh.it>
In reply to#1175992
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1176918

FromDavid Herrmann <dh.herrmann@gmail.com>
Date2015-07-05 11:10 +0200
Message-ID<pIO5Z-W8-29@gated-at.bofh.it>
In reply to#1176030
Hi

On Thu, Jul 2, 2015 at 7:47 PM, Sergei Zviagintsev <sergei@s15v.net> wrote:
> 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.

Exactly, that's why the commit got reverted. The U32_MAX check is
*not* about overflows of pool-memory, but rather about overflows in
quota-accounting.

Thanks
David
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1176850

FromSergei Zviagintsev <sergei@s15v.net>
Date2015-07-05 10:30 +0200
Message-ID<pINtg-tz-17@gated-at.bofh.it>
In reply to#1175992
Hi,

On Sat, Jul 04, 2015 at 01:42:31PM +0200, David Herrmann wrote:
> Hi
> 
> On Thu, Jul 2, 2015 at 3:45 PM, Sergei Zviagintsev <sergei@s15v.net> 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?
> 
> I don't see why. This code does not care whether quota->msgs is bigger
> than MAX_MSGS. Sure, it does not happen in current code, but this
> code-path really doesn't care whether that case can happen or not. All
> it does, it verify that it is smaller. Hence, we use ">=".
> 
> Furthermore, I usually prefer being rather safe than sorry. WARN_ON()s
> are usually not free, but ">=" is for free, if we already have a
> condition.

ok, thank you for explanation!
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web