From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-5.0 required=3.0 tests=BAYES_00,DKIM_ADSP_CUSTOM_MED, DKIM_SIGNED,DKIM_VALID,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS, URIBL_BLOCKED,USER_AGENT_SANE_1 autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 4E12FC2D0A3 for ; Mon, 9 Nov 2020 12:35:00 +0000 (UTC) Received: from merlin.infradead.org (merlin.infradead.org [205.233.59.134]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id C35E7206E5 for ; Mon, 9 Nov 2020 12:34:59 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="ZANXixC9"; dkim=fail reason="signature verification failed" (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="IE+kPuHY" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org C35E7206E5 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=gmail.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=merlin.20170209; h=Sender:Content-Transfer-Encoding: Content-Type:Cc:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References:Message-ID: Subject:To:From:Date:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=Ot+ub6JpmH+EgE94udftcB768Ql5TIuJJd+eGCo8jfY=; b=ZANXixC9tLchEZdBLX87qeqpq v+IMt/85CRr50imTMhDGlu0U8Cw5O0l5/9Q6ipzOhFJWF/8lmZdXRnneUMC/B+8igmojQ1zxxJAUu xifh3x5BGRs/M/sma989fOR/fDLpbkQ6M0rc5UPvdPl+9gqwKqr42I9O9Us9rmyH561Nzl6qTMA1P GAZGFjICDQUDh0cY43dgO0BRNIlq7/LWXc8LzI/qgR0EWfyJ/36boqAYYRnYD+ARCpsebwIYWULuV XkyHsfJGhoTdkMcWG1DN7VHVVQt9xozuIhwILas7yocOzaHhnAZt9s6SRVDgk3jBPXqaJNm2UAuPS QDlU91c0A==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1kc6NV-0003lf-3D; Mon, 09 Nov 2020 12:34:33 +0000 Received: from mail-pg1-x541.google.com ([2607:f8b0:4864:20::541]) by merlin.infradead.org with esmtps (Exim 4.92.3 #3 (Red Hat Linux)) id 1kc6NS-0003kV-5H for linux-arm-kernel@lists.infradead.org; Mon, 09 Nov 2020 12:34:31 +0000 Received: by mail-pg1-x541.google.com with SMTP id f27so3649306pgl.1 for ; Mon, 09 Nov 2020 04:34:28 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=76lz9d6Ii6PSYL2HYWk36/bgrPK9Hw3WkIKIrFJBsLw=; b=IE+kPuHYp/jcZKjdVvYSKxkhdqKUpcIFOBE5yX2koIZYHk8ZWHQzKMfgvT/OrmU2/J Eud0TDB8QDKA/z2pHm6VdA1Q4xrST8xASR5URzIWygHQp3a7xpZ8zACj5PH/WogPn87F QxbDzek8tgp6qNxnKs0HkQmU54SnDi6s5U6w4GfjL2a4rdZeIZXDM87SB0Zo5pJZnyyR km02YilQ0oRMR7+H78s8qp1Ec7Cud+iNE+rGozPALZmj38DZgsyxHRjfZ9wNHraxAHEO ZvSvFB+/p2wXm1R9/1hG5rcGq6US/iSUknzkuJtZJexW/rdM9mbOHOw5HSWsq7Zy4Z6V 7l7g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=76lz9d6Ii6PSYL2HYWk36/bgrPK9Hw3WkIKIrFJBsLw=; b=nvco0R7NL+uuBV8oCvPokBQMZ4iVoylnX48fBOj+3w/mx2+fTceiAzpJaoIZLvSMaM wEn4kXgO0NXJqcDLZs+Nep9cLilso2UG6pylRL9ZGVl6/PkxQ7IzXmTZnpxtQ2IlIFO4 hkhBbu009Ec94/0CoQPWgpzvXvknvbP18yoGDSmodXL5I3inIsUCFAb5JyZoLp4IlS4T Ow2X6Vo7PXPMy744RCzeVHmrFdYjn7pTQzwvnnVG5wk5xB5jPtye5EjbUUnGhUXZmI1T Rje2gpQi+pO/kmjtagiWt2X6PBzu6Qyc/RfYUBdFMz6eJ9AOiYb/voUzzPjuwlZB/6Ay jEaw== X-Gm-Message-State: AOAM531d82QDzSqs6wZsejKX/7qkbFvIULRsk/7m0A2uk+BRef5yXjH0 r3/7AAnlyjMIGulMMkTpYJIG9F6CvU4= X-Google-Smtp-Source: ABdhPJxl/vCsqHlmlJiHL6amNEf2zZqT92aOPveveVj4LTo1dT08gogXympMqcWcPc3wGR1HcGDfbQ== X-Received: by 2002:a62:54c2:0:b029:156:4e4c:ff49 with SMTP id i185-20020a6254c20000b02901564e4cff49mr13494150pfb.26.1604925266688; Mon, 09 Nov 2020 04:34:26 -0800 (PST) Received: from syed ([223.225.3.45]) by smtp.gmail.com with ESMTPSA id v18sm11297148pfn.35.2020.11.09.04.34.21 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Mon, 09 Nov 2020 04:34:25 -0800 (PST) Date: Mon, 9 Nov 2020 18:04:11 +0530 From: Syed Nayyar Waris To: Arnd Bergmann Subject: Re: [PATCH v12 4/4] gpio: xilinx: Utilize generic bitmap_get_value and _set_value Message-ID: <20201109123411.GA19869@syed> References: <15a044d3ba23f00c31fd09437bdd3e5924bb91cd.1603055402.git.syednwaris@gmail.com> <20201101150033.GA68138@shinobu> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.24 (2015-08-30) X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20201109_073430_253927_16D38C9E X-CRM114-Status: GOOD ( 47.68 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Bartosz Golaszewski , "linux-kernel@vger.kernel.org" , Linus Walleij , Michal Simek , William Breathitt Gray , "open list:GPIO SUBSYSTEM" , Andrew Morton , Andy Shevchenko , Linux ARM Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Sun, Nov 01, 2020 at 09:08:29PM +0100, Arnd Bergmann wrote: > On Sun, Nov 1, 2020 at 4:00 PM William Breathitt Gray > wrote: > > > > On Thu, Oct 29, 2020 at 11:44:47PM +0100, Arnd Bergmann wrote: > > > On Sun, Oct 18, 2020 at 11:44 PM Syed Nayyar Waris wrote: > > > > > > > > This patch reimplements the xgpio_set_multiple() function in > > > > drivers/gpio/gpio-xilinx.c to use the new generic functions: > > > > bitmap_get_value() and bitmap_set_value(). The code is now simpler > > > > to read and understand. Moreover, instead of looping for each bit > > > > in xgpio_set_multiple() function, now we can check each channel at > > > > a time and save cycles. > > > > > > This now causes -Wtype-limits warnings in linux-next with gcc-10: > > > > Hi Arnd, > > > > What version of gcc-10 are you running? I'm having trouble generating > > these warnings so I suspect I'm using a different version than you. > > I originally saw it with the binaries from > https://mirrors.edge.kernel.org/pub/tools/crosstool/, but I have > also been able to reproduce it with a minimal test case on the > binaries from godbolt.org, see https://godbolt.org/z/Wq8q4n > > > Let me first verify that I understand the problem correctly. The issue > > is the possibility of a stack smash in bitmap_set_value() when the value > > of start + nbits is larger than the length of the map bitmap memory > > region. This is because index (or index + 1) could be outside the range > > of the bitmap memory region passed in as map. Is my understanding > > correct here? > > Yes, that seems to be the case here. > > > In xgpio_set_multiple(), the variables width[0] and width[1] serve as > > possible start and nbits values for the bitmap_set_value() calls. > > Because width[0] and width[1] are unsigned int variables, GCC considers > > the possibility that the value of width[0]/width[1] might exceed the > > length of the bitmap memory region named old and thus result in a stack > > smash. > > > > I don't know if invalid width values are actually possible for the > > Xilinx gpio device, but let's err on the side of safety and assume this > > is actually a possibility. We should verify that the combined value of > > gpio_width[0] + gpio_width[1] does not exceed 64 bits; we can add a > > check for this in xgpio_probe() when we grab the gpio_width values. > > > > However, we're still left with the GCC warnings because GCC is not smart > > enough to know that we've already checked the boundary and width[0] and > > width[1] are valid values. I suspect we can avoid this warning is we > > refactor bitmap_set_value() to increment map seperately and then set it: > > As I understand it, part of the problem is that gcc sees the possible > range as being constrained by the operations on 'start' and 'nbits', > in particular the shift in BIT_WORD() that put an upper bound on > the index, but then it sees that the upper bound is higher than the > upper bound of the array, i.e. element zero. > > I added a check > > if (start >= 64 || start + size >= 64) return; > > in the godbolt.org testcase, which does help limit the start > index appropriately, but it is not sufficient to let the compiler > see that the 'if (space >= nbits) ' condition is guaranteed to > be true for all values here. > > > static inline void bitmap_set_value(unsigned long *map, > > unsigned long value, > > unsigned long start, unsigned long nbits) > > { > > const unsigned long offset = start % BITS_PER_LONG; > > const unsigned long ceiling = round_up(start + 1, BITS_PER_LONG); > > const unsigned long space = ceiling - start; > > > > map += BIT_WORD(start); > > value &= GENMASK(nbits - 1, 0); > > > > if (space >= nbits) { > > *map &= ~(GENMASK(nbits - 1, 0) << offset); > > *map |= value << offset; > > } else { > > *map &= ~BITMAP_FIRST_WORD_MASK(start); > > *map |= value << offset; > > map++; > > *map &= ~BITMAP_LAST_WORD_MASK(start + nbits); > > *map |= value >> space; > > } > > } > > > > This avoids adding a costly conditional check inside bitmap_set_value() > > when almost all bitmap_set_value() calls will have static arguments with > > well-defined and obvious boundaries. > > > > Do you think this would be an acceptable solution to resolve your GCC > > warnings? > > Unfortunately, it does not seem to make a difference, as gcc still > knows that this compiles to the same result, and it produces the same > warning as before (see https://godbolt.org/z/rjx34r) > > Arnd Hi Arnd, Sharing a different version of bitmap_set_valuei() function. See below. Let me know if the below solution looks good to you and if it resolves the above compiler warning. @@ -1,5 +1,5 @@ static inline void bitmap_set_value(unsigned long *map, - unsigned long value, + unsigned long value, const size_t length, unsigned long start, unsigned long nbits) { const size_t index = BIT_WORD(start); @@ -7,6 +7,9 @@ static inline void bitmap_set_value(unsigned long *map, const unsigned long ceiling = round_up(start + 1, BITS_PER_LONG); const unsigned long space = ceiling - start; + if (index >= length) + return; + value &= GENMASK(nbits - 1, 0); if (space >= nbits) { @@ -15,6 +18,10 @@ static inline void bitmap_set_value(unsigned long *map, } else { map[index + 0] &= ~BITMAP_FIRST_WORD_MASK(start); map[index + 0] |= value << offset; + + if (index + 1 >= length) + return; + map[index + 1] &= ~BITMAP_LAST_WORD_MASK(start + nbits); map[index + 1] |= value >> space; } _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel