lkml.org 
[lkml]   [2015]   [Jul]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH RFC 5/5] kdbus: improve tests on incrementing quota
From
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
>


\
 
 \ /
  Last update: 2015-07-02 11:01    [W:0.146 / U:0.220 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site