Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753244AbbGEKAp (ORCPT ); Sun, 5 Jul 2015 06:00:45 -0400 Received: from mail-la0-f48.google.com ([209.85.215.48]:34734 "EHLO mail-la0-f48.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750844AbbGEKAn (ORCPT ); Sun, 5 Jul 2015 06:00:43 -0400 MIME-Version: 1.0 In-Reply-To: <20150702134500.GS17917@localhost.localdomain> References: <1435497454-10464-1-git-send-email-sergei@s15v.net> <1435497454-10464-6-git-send-email-sergei@s15v.net> <20150702134500.GS17917@localhost.localdomain> Date: Sat, 4 Jul 2015 13:42:31 +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: 3017 Lines: 79 Hi On Thu, Jul 2, 2015 at 3:45 PM, 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 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 -- 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/