Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753801AbbGBIvA (ORCPT ); Thu, 2 Jul 2015 04:51:00 -0400 Received: from mail-la0-f49.google.com ([209.85.215.49]:35349 "EHLO mail-la0-f49.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752590AbbGBIuv (ORCPT ); Thu, 2 Jul 2015 04:50:51 -0400 MIME-Version: 1.0 In-Reply-To: <1435497454-10464-6-git-send-email-sergei@s15v.net> References: <1435497454-10464-1-git-send-email-sergei@s15v.net> <1435497454-10464-6-git-send-email-sergei@s15v.net> Date: Thu, 2 Jul 2015 10:50:47 +0200 Message-ID: Subject: Re: [PATCH RFC 5/5] kdbus: improve tests on incrementing quota From: David Herrmann To: Sergei Zviagintsev Cc: Greg Kroah-Hartman , Daniel Mack , David Herrmann , Djalal Harouni , linux-kernel Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3851 Lines: 108 Hi On Sun, Jun 28, 2015 at 3:17 PM, Sergei Zviagintsev 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 > --- > 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/