Hi,
Main points here are to use conventional types, rewrite tests in obvious
way, add couple of new WARN_ONs and do a bit more work in the others.
Keeping in mind the number of mistakes I made while preparing this
patchset, I decided to mark it as RFC :) I'd appreciate any feedback on
these.
Sergei Zviagintsev (5):
kdbus: fix typos in kdbus_conn_quota_inc()
kdbus: use standard kernel types in struct kdbus_quota
kdbus: do explicit overflow check in kdbus_conn_quota_inc()
kdbus: handle WARN_ON cases properly when decrementing quota
kdbus: improve tests on incrementing quota
ipc/kdbus/connection.c | 44 ++++++++++++++++++++++++++++++--------------
1 file changed, 30 insertions(+), 14 deletions(-)
--
1.8.3.1
Signed-off-by: Sergei Zviagintsev <[email protected]>
---
ipc/kdbus/connection.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/ipc/kdbus/connection.c b/ipc/kdbus/connection.c
index 9993753d11de..df072487e23c 100644
--- a/ipc/kdbus/connection.c
+++ b/ipc/kdbus/connection.c
@@ -646,7 +646,7 @@ int kdbus_conn_quota_inc(struct kdbus_conn *c, struct kdbus_user *u,
* allocation schemes. Furthermore, resource utilization should be
* maximized, so only minimal resources stay reserved. However, we need
* to adapt to a dynamic number of users, as we cannot know how many
- * users will talk to a connection. Therefore, the current allocations
+ * users will talk to a connection. Therefore, the current allocation
* works like this:
* We limit the number of bytes in a destination's pool per sending
* user. The space available for a user is 33% of the unused pool space
@@ -688,7 +688,7 @@ int kdbus_conn_quota_inc(struct kdbus_conn *c, struct kdbus_user *u,
/*
* Pool owner slices are un-accounted slices; they can claim more
- * than 50% of the queue. However, the slice we're dealing with here
+ * than 50% of the queue. However, the slices we're dealing with here
* belong to the incoming queue, hence they are 'accounted' slices
* to which the 50%-limit applies.
*/
--
1.8.3.1
uint{8,16,32}_t -> u{8,16,32}
Signed-off-by: Sergei Zviagintsev <[email protected]>
---
ipc/kdbus/connection.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/ipc/kdbus/connection.c b/ipc/kdbus/connection.c
index df072487e23c..af044f93c14f 100644
--- a/ipc/kdbus/connection.c
+++ b/ipc/kdbus/connection.c
@@ -606,9 +606,9 @@ bool kdbus_conn_has_name(struct kdbus_conn *conn, const char *name)
}
struct kdbus_quota {
- uint32_t memory;
- uint16_t msgs;
- uint8_t fds;
+ u32 memory;
+ u16 msgs;
+ u8 fds;
};
/**
--
1.8.3.1
Replace the use of max() with explicit and obvious overflow check.
Signed-off-by: Sergei Zviagintsev <[email protected]>
---
ipc/kdbus/connection.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/ipc/kdbus/connection.c b/ipc/kdbus/connection.c
index af044f93c14f..1d44e280eff0 100644
--- a/ipc/kdbus/connection.c
+++ b/ipc/kdbus/connection.c
@@ -668,9 +668,11 @@ int kdbus_conn_quota_inc(struct kdbus_conn *c, struct kdbus_user *u,
id = u ? u->id : KDBUS_USER_KERNEL_ID;
if (id >= c->n_quota) {
- unsigned int users;
+ unsigned int users = KDBUS_ALIGN8(id) + 8;
+
+ if (users < id) /* overflow */
+ users = id;
- users = max(KDBUS_ALIGN8(id) + 8, id);
quota = krealloc(c->quota, users * sizeof(*quota),
GFP_KERNEL | __GFP_ZERO);
if (!quota)
--
1.8.3.1
If we spotted inconsistency, fix it by setting values to zero.
Signed-off-by: Sergei Zviagintsev <[email protected]>
---
ipc/kdbus/connection.c | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/ipc/kdbus/connection.c b/ipc/kdbus/connection.c
index 1d44e280eff0..12e32de310f5 100644
--- a/ipc/kdbus/connection.c
+++ b/ipc/kdbus/connection.c
@@ -742,9 +742,15 @@ void kdbus_conn_quota_dec(struct kdbus_conn *c, struct kdbus_user *u,
if (!WARN_ON(quota->msgs == 0))
--quota->msgs;
- if (!WARN_ON(quota->memory < memory))
+
+ if (WARN_ON(quota->memory < memory))
+ quota->memory = 0;
+ else
quota->memory -= memory;
- if (!WARN_ON(quota->fds < fds))
+
+ if (WARN_ON(quota->fds < fds))
+ quota->fds = 0;
+ else
quota->fds -= fds;
}
--
1.8.3.1
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.
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 <[email protected]>
---
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;
+
+ if (WARN_ON(quota->msgs > KDBUS_CONN_MAX_MSGS) ||
+ quota->msgs == KDBUS_CONN_MAX_MSGS)
return -ENOBUFS;
- 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;
quota->memory += memory;
--
1.8.3.1
Hi
On Sun, Jun 28, 2015 at 3:17 PM, Sergei Zviagintsev <[email protected]> wrote:
> Signed-off-by: Sergei Zviagintsev <[email protected]>
> ---
> ipc/kdbus/connection.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
(I'm a fan of putting the actual typo-corrections into the
commit-msg-body, like: "allocations -> allocation" and "slice ->
slices", which makes reviewing such patches a lot easier.)
Reviewed-by: David Herrmann <[email protected]>
Thanks
David
> diff --git a/ipc/kdbus/connection.c b/ipc/kdbus/connection.c
> index 9993753d11de..df072487e23c 100644
> --- a/ipc/kdbus/connection.c
> +++ b/ipc/kdbus/connection.c
> @@ -646,7 +646,7 @@ int kdbus_conn_quota_inc(struct kdbus_conn *c, struct kdbus_user *u,
> * allocation schemes. Furthermore, resource utilization should be
> * maximized, so only minimal resources stay reserved. However, we need
> * to adapt to a dynamic number of users, as we cannot know how many
> - * users will talk to a connection. Therefore, the current allocations
> + * users will talk to a connection. Therefore, the current allocation
> * works like this:
> * We limit the number of bytes in a destination's pool per sending
> * user. The space available for a user is 33% of the unused pool space
> @@ -688,7 +688,7 @@ int kdbus_conn_quota_inc(struct kdbus_conn *c, struct kdbus_user *u,
>
> /*
> * Pool owner slices are un-accounted slices; they can claim more
> - * than 50% of the queue. However, the slice we're dealing with here
> + * than 50% of the queue. However, the slices we're dealing with here
> * belong to the incoming queue, hence they are 'accounted' slices
> * to which the 50%-limit applies.
> */
> --
> 1.8.3.1
>
Hi
On Sun, Jun 28, 2015 at 3:17 PM, Sergei Zviagintsev <[email protected]> wrote:
> uint{8,16,32}_t -> u{8,16,32}
>
> Signed-off-by: Sergei Zviagintsev <[email protected]>
> ---
> ipc/kdbus/connection.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
Reviewed-by: David Herrmann <[email protected]>
Thanks
David
> diff --git a/ipc/kdbus/connection.c b/ipc/kdbus/connection.c
> index df072487e23c..af044f93c14f 100644
> --- a/ipc/kdbus/connection.c
> +++ b/ipc/kdbus/connection.c
> @@ -606,9 +606,9 @@ bool kdbus_conn_has_name(struct kdbus_conn *conn, const char *name)
> }
>
> struct kdbus_quota {
> - uint32_t memory;
> - uint16_t msgs;
> - uint8_t fds;
> + u32 memory;
> + u16 msgs;
> + u8 fds;
> };
>
> /**
> --
> 1.8.3.1
>
Hi
On Sun, Jun 28, 2015 at 3:17 PM, Sergei Zviagintsev <[email protected]> wrote:
> Replace the use of max() with explicit and obvious overflow check.
>
> Signed-off-by: Sergei Zviagintsev <[email protected]>
> ---
> ipc/kdbus/connection.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/ipc/kdbus/connection.c b/ipc/kdbus/connection.c
> index af044f93c14f..1d44e280eff0 100644
> --- a/ipc/kdbus/connection.c
> +++ b/ipc/kdbus/connection.c
> @@ -668,9 +668,11 @@ int kdbus_conn_quota_inc(struct kdbus_conn *c, struct kdbus_user *u,
>
> id = u ? u->id : KDBUS_USER_KERNEL_ID;
> if (id >= c->n_quota) {
> - unsigned int users;
> + unsigned int users = KDBUS_ALIGN8(id) + 8;
> +
> + if (users < id) /* overflow */
> + users = id;
>
> - users = max(KDBUS_ALIGN8(id) + 8, id);
To me, the max() looks fine. I mean, it should be obvious that it
checks for an overflow, right? Otherwise, I'd prefer adding a comment
instead of the explicit conditional.
Thanks
David
> quota = krealloc(c->quota, users * sizeof(*quota),
> GFP_KERNEL | __GFP_ZERO);
> if (!quota)
> --
> 1.8.3.1
>
Hi
On Sun, Jun 28, 2015 at 3:17 PM, Sergei Zviagintsev <[email protected]> wrote:
> If we spotted inconsistency, fix it by setting values to zero.
Why?
I prefer doing nothing and just bailing out on programming errors.
Setting it to 0 just complicates debugging and drops possibly useful
information ('what was the counter value at that time?').
Thanks
David
> Signed-off-by: Sergei Zviagintsev <[email protected]>
> ---
> ipc/kdbus/connection.c | 10 ++++++++--
> 1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/ipc/kdbus/connection.c b/ipc/kdbus/connection.c
> index 1d44e280eff0..12e32de310f5 100644
> --- a/ipc/kdbus/connection.c
> +++ b/ipc/kdbus/connection.c
> @@ -742,9 +742,15 @@ void kdbus_conn_quota_dec(struct kdbus_conn *c, struct kdbus_user *u,
>
> if (!WARN_ON(quota->msgs == 0))
> --quota->msgs;
> - if (!WARN_ON(quota->memory < memory))
> +
> + if (WARN_ON(quota->memory < memory))
> + quota->memory = 0;
> + else
> quota->memory -= memory;
> - if (!WARN_ON(quota->fds < fds))
> +
> + if (WARN_ON(quota->fds < fds))
> + quota->fds = 0;
> + else
> quota->fds -= fds;
> }
>
> --
> 1.8.3.1
>
Hi
On Sun, Jun 28, 2015 at 3:17 PM, Sergei Zviagintsev <[email protected]> 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 <[email protected]>
> ---
> 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
>
Hi,
On Thu, Jul 02, 2015 at 10:31:48AM +0200, David Herrmann wrote:
> Hi
>
> On Sun, Jun 28, 2015 at 3:17 PM, Sergei Zviagintsev <[email protected]> wrote:
> > Signed-off-by: Sergei Zviagintsev <[email protected]>
> > ---
> > ipc/kdbus/connection.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
>
> (I'm a fan of putting the actual typo-corrections into the
> commit-msg-body, like: "allocations -> allocation" and "slice ->
> slices", which makes reviewing such patches a lot easier.)
Thank you for the comment. I will resend this with adjusted commit
message along with other patches in v2.
>
> Reviewed-by: David Herrmann <[email protected]>
Should I include this Reviewed-by tag into v2 of this patch myself, or
you will perform review process again?
Hi
On Thu, Jul 2, 2015 at 11:51 AM, Sergei Zviagintsev <[email protected]> wrote:
> On Thu, Jul 02, 2015 at 10:31:48AM +0200, David Herrmann wrote:
>>
>> Reviewed-by: David Herrmann <[email protected]>
>
> Should I include this Reviewed-by tag into v2 of this patch myself, or
> you will perform review process again?
Yes, please include it.
Thanks
David
Hi,
On Thu, Jul 02, 2015 at 10:37:05AM +0200, David Herrmann wrote:
> Hi
>
> On Sun, Jun 28, 2015 at 3:17 PM, Sergei Zviagintsev <[email protected]> wrote:
> > If we spotted inconsistency, fix it by setting values to zero.
>
> Why?
>
> I prefer doing nothing and just bailing out on programming errors.
> Setting it to 0 just complicates debugging and drops possibly useful
> information ('what was the counter value at that time?').
Thank you for commenting on this! Now it seems to be clear for me.
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 <[email protected]> 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 <[email protected]>
> > ---
> > 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
> >
Hi,
On Thu, Jul 02, 2015 at 10:35:30AM +0200, David Herrmann wrote:
> Hi
>
> On Sun, Jun 28, 2015 at 3:17 PM, Sergei Zviagintsev <[email protected]> wrote:
> > Replace the use of max() with explicit and obvious overflow check.
> >
> > Signed-off-by: Sergei Zviagintsev <[email protected]>
> > ---
> > ipc/kdbus/connection.c | 6 ++++--
> > 1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/ipc/kdbus/connection.c b/ipc/kdbus/connection.c
> > index af044f93c14f..1d44e280eff0 100644
> > --- a/ipc/kdbus/connection.c
> > +++ b/ipc/kdbus/connection.c
> > @@ -668,9 +668,11 @@ int kdbus_conn_quota_inc(struct kdbus_conn *c, struct kdbus_user *u,
> >
> > id = u ? u->id : KDBUS_USER_KERNEL_ID;
> > if (id >= c->n_quota) {
> > - unsigned int users;
> > + unsigned int users = KDBUS_ALIGN8(id) + 8;
> > +
> > + if (users < id) /* overflow */
> > + users = id;
> >
> > - users = max(KDBUS_ALIGN8(id) + 8, id);
>
> To me, the max() looks fine. I mean, it should be obvious that it
> checks for an overflow, right? Otherwise, I'd prefer adding a comment
> instead of the explicit conditional.
Well, at first sight I assumed that there was some clever algorithm
which later had been quick-fixed and so on...
I am not sure about commenting current variant, as CodingStyle
tells us to write obvious code instead of explanations. And if it is
already obvious, then it doesn't need a comment, right? :)
In another letter of this series I mentioned commits af8e2f750985
and ac5c385cc67a from out-of-tree kdbus repo, which show that not all of
these overflow things are obvious for maintainer, not talking about
someone who isn't familiar with the code.
>
> Thanks
> David
>
> > quota = krealloc(c->quota, users * sizeof(*quota),
> > GFP_KERNEL | __GFP_ZERO);
> > if (!quota)
> > --
> > 1.8.3.1
> >
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 <[email protected]> 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 <[email protected]>
> > > ---
> > > 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
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
Hi
On Thu, Jul 2, 2015 at 3:45 PM, Sergei Zviagintsev <[email protected]> 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 <[email protected]> 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.
[...]
>>
>> > - 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.
Same as above.
Thanks
David
Hi
On Thu, Jul 2, 2015 at 7:47 PM, Sergei Zviagintsev <[email protected]> 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
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 <[email protected]> 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 <[email protected]> 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!
Hi
On Sun, Jun 28, 2015 at 3:17 PM, Sergei Zviagintsev <[email protected]> wrote:
> Hi,
>
> Main points here are to use conventional types, rewrite tests in obvious
> way, add couple of new WARN_ONs and do a bit more work in the others.
>
> Keeping in mind the number of mistakes I made while preparing this
> patchset, I decided to mark it as RFC :) I'd appreciate any feedback on
> these.
>
>
> Sergei Zviagintsev (5):
> kdbus: fix typos in kdbus_conn_quota_inc()
> kdbus: use standard kernel types in struct kdbus_quota
> kdbus: do explicit overflow check in kdbus_conn_quota_inc()
> kdbus: handle WARN_ON cases properly when decrementing quota
> kdbus: improve tests on incrementing quota
I applied #1 and #2 now. If you resend, please drop those from the set.
Thanks!
David
Hi David,
On Mon, Jul 06, 2015 at 08:48:26PM +0200, David Herrmann wrote:
> Hi
>
> On Sun, Jun 28, 2015 at 3:17 PM, Sergei Zviagintsev <[email protected]> wrote:
> > Hi,
> >
> > Main points here are to use conventional types, rewrite tests in obvious
> > way, add couple of new WARN_ONs and do a bit more work in the others.
> >
> > Keeping in mind the number of mistakes I made while preparing this
> > patchset, I decided to mark it as RFC :) I'd appreciate any feedback on
> > these.
> >
> >
> > Sergei Zviagintsev (5):
> > kdbus: fix typos in kdbus_conn_quota_inc()
> > kdbus: use standard kernel types in struct kdbus_quota
> > kdbus: do explicit overflow check in kdbus_conn_quota_inc()
> > kdbus: handle WARN_ON cases properly when decrementing quota
> > kdbus: improve tests on incrementing quota
>
> I applied #1 and #2 now. If you resend, please drop those from the set.
As I see, neither Greg's tree nor out-of-tree kdbus repo
(i.e. https://github.com/systemd/kdbus) contains them. Am I missing
something?
>
> Thanks!
> David
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
On Tue, Jul 07, 2015 at 07:29:41PM +0300, Sergei Zviagintsev wrote:
> Hi David,
>
> On Mon, Jul 06, 2015 at 08:48:26PM +0200, David Herrmann wrote:
> > Hi
> >
> > On Sun, Jun 28, 2015 at 3:17 PM, Sergei Zviagintsev <[email protected]> wrote:
> > > Hi,
> > >
> > > Main points here are to use conventional types, rewrite tests in obvious
> > > way, add couple of new WARN_ONs and do a bit more work in the others.
> > >
> > > Keeping in mind the number of mistakes I made while preparing this
> > > patchset, I decided to mark it as RFC :) I'd appreciate any feedback on
> > > these.
> > >
> > >
> > > Sergei Zviagintsev (5):
> > > kdbus: fix typos in kdbus_conn_quota_inc()
> > > kdbus: use standard kernel types in struct kdbus_quota
> > > kdbus: do explicit overflow check in kdbus_conn_quota_inc()
> > > kdbus: handle WARN_ON cases properly when decrementing quota
> > > kdbus: improve tests on incrementing quota
> >
> > I applied #1 and #2 now. If you resend, please drop those from the set.
>
> As I see, neither Greg's tree nor out-of-tree kdbus repo
> (i.e. https://github.com/systemd/kdbus) contains them. Am I missing
> something?
David has sent me a pull request for these, give me a few days to catch
up on my patches before I pull it in.
thanks,
greg k-h