Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752604AbdGaQEU (ORCPT ); Mon, 31 Jul 2017 12:04:20 -0400 Received: from mail-oi0-f68.google.com ([209.85.218.68]:35490 "EHLO mail-oi0-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752316AbdGaQET (ORCPT ); Mon, 31 Jul 2017 12:04:19 -0400 MIME-Version: 1.0 In-Reply-To: <1501515117.2466.9.camel@wdc.com> References: <20170731065016.2947796-1-arnd@arndb.de> <1501515117.2466.9.camel@wdc.com> From: Arnd Bergmann Date: Mon, 31 Jul 2017 18:04:17 +0200 X-Google-Sender-Auth: ISYMo-MAiYWx4mGd2H3b_oblvio Message-ID: Subject: Re: [PATCH] infiniband: avoid overflow warning To: Bart Van Assche Cc: "monis@mellanox.com" , "linux-kernel@vger.kernel.org" , "keescook@chromium.org" , "linux-rdma@vger.kernel.org" , "parav@mellanox.com" , "Michal.Kalderon@cavium.com" , "sean.hefty@intel.com" , "danielmicay@gmail.com" , "Ariel.Elior@cavium.com" , "hal.rosenstock@gmail.com" , "davem@davemloft.net" , "dledford@redhat.com" , "noaos@mellanox.com" 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: 2802 Lines: 56 On Mon, Jul 31, 2017 at 5:32 PM, Bart Van Assche wrote: > On Mon, 2017-07-31 at 09:30 +0200, Arnd Bergmann wrote: >> On Mon, Jul 31, 2017 at 9:08 AM, Moni Shoua wrote: >> > On Mon, Jul 31, 2017 at 9:50 AM, Arnd Bergmann wrote: >> > > --- a/include/rdma/ib_addr.h >> > > +++ b/include/rdma/ib_addr.h >> > > @@ -172,7 +172,8 @@ static inline int rdma_ip2gid(struct sockaddr *addr, union ib_gid *gid) >> > > (struct in6_addr *)gid); >> > > break; >> > > case AF_INET6: >> > > - memcpy(gid->raw, &((struct sockaddr_in6 *)addr)->sin6_addr, 16); >> > > + *(struct in6_addr *)&gid->raw = >> > > + ((struct sockaddr_in6 *)addr)->sin6_addr; >> > > break; >> > > default: >> > > return -EINVAL; >> > >> > what happens if you replace 16 with sizeof(struct in6_addr)? >> >> Same thing: the problem is that gcc already knows the size of the structure we >> pass in here, and it is in fact shorter. >> >> I also tried changing the struct sockaddr pointer to a sockaddr_storage pointer, >> without success. Other approaches that do work are: >> >> - mark addr_event() as "noinline" to prevent gcc from seeing the true >> size of the >> inetaddr_event stack object in rdma_ip2gid(). I considered this a little ugly. >> >> - change inetaddr_event to put a larger structure on the stack, using >> sockaddr_storage or sockaddr_in6. This would be less efficient. >> >> - define a union of sockaddr_in and sockaddr_in6, and use that as the argument >> to rdma_ip2gid/rdma_gid2ip, and change all callers to use that union type. >> This is probably the cleanest approach as it gets rid of a lot of questionable >> type casts, but it's a relatively large patch and also slightly less >> efficient as we have >> to zero more stack storage in some cases. > > > So inetaddr_event() assigns AF_INET so .sin_family and gcc warns about code > that is only executed if .sin_family == AF_INET6? Since this warning is the > result of incorrect interprocedural analysis by gcc, shouldn't this be > reported as a bug to the gcc authors? I think the interprocedural analysis here is just a little worse than it could be, but it's not actually correct. It's not gcc that prints the warning (if it did, then I'd agree it would be a gcc bug) but the warning is triggered intentionally by the fortified version of memcpy in include/linux/string.h. The problem as I understand it is that gcc cannot guarantee that it tracks the value of addr->sa_family at least as far as the size of the stack object, and it has no strict reason to do so, so the inlined rdma_ip2gid() will still contain both cases. Arnd