Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753319AbbGBRrf (ORCPT ); Thu, 2 Jul 2015 13:47:35 -0400 Received: from out5-smtp.messagingengine.com ([66.111.4.29]:58350 "EHLO out5-smtp.messagingengine.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752942AbbGBRr1 (ORCPT ); Thu, 2 Jul 2015 13:47:27 -0400 X-Sasl-enc: cYbAFx+fVA1Lz7Y9FiOdYi9YGMwb8tgUIX4OhlN6X2rH 1435859246 Date: Thu, 2 Jul 2015 20:47:25 +0300 From: Sergei Zviagintsev To: Djalal Harouni Cc: David Herrmann , Greg Kroah-Hartman , Daniel Mack , David Herrmann , linux-kernel Subject: Re: [PATCH RFC 5/5] kdbus: improve tests on incrementing quota Message-ID: <20150702174725.GV17917@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> <20150702141341.GA5452@dztty> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20150702141341.GA5452@dztty> 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: 3269 Lines: 87 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 -- 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/