Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753051AbbGBNpG (ORCPT ); Thu, 2 Jul 2015 09:45:06 -0400 Received: from out5-smtp.messagingengine.com ([66.111.4.29]:58144 "EHLO out5-smtp.messagingengine.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752326AbbGBNpC (ORCPT ); Thu, 2 Jul 2015 09:45:02 -0400 X-Sasl-enc: 1BzPyoiUGBRKZiqiq6OegKC0kWU6owubgEhrG72aePh1 1435844701 Date: Thu, 2 Jul 2015 16:45:00 +0300 From: Sergei Zviagintsev To: David Herrmann Cc: Greg Kroah-Hartman , Daniel Mack , David Herrmann , Djalal Harouni , linux-kernel Subject: Re: [PATCH RFC 5/5] kdbus: improve tests on incrementing quota Message-ID: <20150702134500.GS17917@localhost.localdomain> References: <1435497454-10464-1-git-send-email-sergei@s15v.net> <1435497454-10464-6-git-send-email-sergei@s15v.net> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5464 Lines: 144 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 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 > > --- > > 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/