Messages in this thread | | | Date | Thu, 2 Jul 2015 10:50:47 +0200 | Subject | Re: [PATCH RFC 5/5] kdbus: improve tests on incrementing quota | From | David Herrmann <> |
| |
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 >
| |