From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752574AbbKIORD (ORCPT ); Mon, 9 Nov 2015 09:17:03 -0500 Received: from mail-lf0-f47.google.com ([209.85.215.47]:36842 "EHLO mail-lf0-f47.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752283AbbKIORA (ORCPT ); Mon, 9 Nov 2015 09:17:00 -0500 From: Rasmus Villemoes To: Hannes Frederic Sowa Cc: Linus Torvalds , David Miller , Andrew Morton , Network Development , Linux Kernel Mailing List Subject: Re: [GIT] Networking Organization: D03 References: <20151027.233235.1641084823622810663.davem@davemloft.net> <1446030209.105419.422375497.14563933@webmail.messagingengine.com> <87pozzvzj6.fsf@rasmusvillemoes.dk> <1447070962.399769.433652241.038FDB6A@webmail.messagingengine.com> X-Hashcash: 1:20:151109:torvalds@linux-foundation.org::QU5UFZZA7fXPPW41:000000000000000000000000000000000p8V X-Hashcash: 1:20:151109:linux-kernel@vger.kernel.org::++Zd/ZnPpE/3N1eE:0000000000000000000000000000000003Xk/ X-Hashcash: 1:20:151109:hannes@stressinduktion.org::nBfSNGkE7uM/P6RJ:000000000000000000000000000000000004diz X-Hashcash: 1:20:151109:netdev@vger.kernel.org::urcNQPQJJ/JzrKfw:0000000000000000000000000000000000000006W8J X-Hashcash: 1:20:151109:akpm@linux-foundation.org::u/NMIqHhzed6lOXg:0000000000000000000000000000000000007cVk X-Hashcash: 1:20:151109:davem@davemloft.net::55tvA4TIEQNm2DRV:000000000000000000000000000000000000000000E+ZM Date: Mon, 09 Nov 2015 15:16:56 +0100 In-Reply-To: <1447070962.399769.433652241.038FDB6A@webmail.messagingengine.com> (Hannes Frederic Sowa's message of "Mon, 09 Nov 2015 13:09:22 +0100") Message-ID: <87a8qnz293.fsf@rasmusvillemoes.dk> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.3 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, Nov 09 2015, Hannes Frederic Sowa wrote: > Hi, > > On Wed, Oct 28, 2015, at 15:27, Rasmus Villemoes wrote: >> >> I agree - proper overflow checking can be really hard. Quick, assuming a >> and b have the same unsigned integer type, is 'a+b> check overflow? Of course not (hint: promotion rules). And as you say, >> it gets even more complicated for signed types. >> >> A few months ago I tried posting a complete set of fallbacks for older >> compilers (https://lkml.org/lkml/2015/7/19/358), but nothing really >> happened. Now I know where Linus stands, so I guess I can just delete >> that branch. > > I actually like your approach of being type agnostic a bit more (in > comparison to static inline functions), mostly because of one specific > reason: > > The type agnostic __builtin_*_overflow function even do the correct > things if you deal with types smaller than int. Imagine e.g. you want to > add to unsigned chars a and b, If you read my mail again you'll see that I mentioned exactly this :-) so obviously I agree that this is a nice part of it. > unsigned char a, b; > if (a + b < a) > goto overflow; > else > a += b; > > The overflow condition will never trigger, as the comparisons will > always be done in the integer domain and a + b < a is never true. I > actually think that this is easy to overlook and the functions should > handle that. Yes. While people very rarely use local u8 or u16 variables for computations, I think one could imagine a and b being struct members, which for one reason or another happens to be of a type narrower than int (which would also make the issue much harder to spot since the struct definition is far away). Something like combine_packets(struct foo *a, const struct foo *b) { if (a->len + b->len < a->len) return -EOVERFLOW; /* ensure a->payload is big enough...*/ memcpy(a->payload + a->len, b->payload, b->len); a->len += b->len; ... } which, depending on details, would either lead to memory corruption or loss of parts of the packets. I haven't actually found any instance of this in the kernel, but that doesn't mean it couldn't get introduced (or that it doesn't exist). Aside: It turns out clang is smart enough to optimize away the broken overflow check, but gcc isn't. Neither issue a warning, despite the intention being rather clear. Rasmus