Message ID | your-ad-here.call-01697881440-ext-2458@work.hours |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:ce89:0:b0:403:3b70:6f57 with SMTP id p9csp203691vqx; Sat, 21 Oct 2023 02:44:26 -0700 (PDT) X-Google-Smtp-Source: AGHT+IGKRAnokrzn4vvTgsnGOSyPJuHc7s9E2wjR1Cpt1knphIhWdUmPb0nANtCsaGTqoCpxIcJv X-Received: by 2002:a17:90b:4f8e:b0:25e:a8ab:9157 with SMTP id qe14-20020a17090b4f8e00b0025ea8ab9157mr3747109pjb.22.1697881466638; Sat, 21 Oct 2023 02:44:26 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1697881466; cv=none; d=google.com; s=arc-20160816; b=Op9Ic3KjbBKulwppLIRwhT4IMojHU9NEvsReq7KamdEbblycLGH1EQeCOWptc+lrHp 8KORkL9FkWaHhXaEOLv17v/21qk9b2K7QaXXWB6oaU8UWb69JrMtaqUBlE6rEG42LTP5 +rJdRnY7D4j97T69IjoYj9EON5WUO+jWweHu0n+SVK29ZZWIDN9iGgpkpdEAaJphg+E0 JdrFrINoLZarnU4V7wGnE+b45GX0bfhzF4SEoxppjp4RF0MLORh2jhqIvcFQ+lPU/dMf El0UKBjGHGE1FPg3uq0r7HsKQKEAi3hovTUgnWdRmfvfdHPQwimrU9uM/K9JxbUYufgU yqJg== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:mime-version:content-disposition:message-id :subject:cc:to:from:date:dkim-signature; bh=wiznSzPHeDvgCYmBpLCxQSz2kUh0Y5cwbWCJqEw6UC4=; fh=HctlXVf76a1tVLb0/cR7cQHAW0y3x88J1KMnFU3fYqA=; b=JiLmhBBjmOJOZaTfhUIy1nfpWN17Gj6iC0x/IjSj9O2eyx7WO6eMdUJtKJUWH039s+ vq4TTebaXB9TgUk0JY7CfxpYOkmKI6YlaONeeD507n8G6oaYRFpmmpqK/g36FiA85ulB psP/qKzGnyc3yBdDedng87x0KRbYQAPWcABL0pHtQ1OcP4UJdVEjSfqw+lqjZTeZHibR o6pdFD6hO5NmHFTsQItKFVvTGqByI+3rmsp9A03bHZUZXa8FtFbwMep/jsNQSplbRIex /q6y98PFl5ut2JHjuvx4GAOenoMEb6uURUdGKdRPznVEza/AwU1AQ72w4zd4eT3vnia/ WkhQ== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@ibm.com header.s=pp1 header.b=B++XRKSF; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.35 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=REJECT sp=NONE dis=NONE) header.from=ibm.com Received: from groat.vger.email (groat.vger.email. [23.128.96.35]) by mx.google.com with ESMTPS id mj19-20020a17090b369300b0027748734bb9si5849023pjb.148.2023.10.21.02.44.26 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sat, 21 Oct 2023 02:44:26 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.35 as permitted sender) client-ip=23.128.96.35; Authentication-Results: mx.google.com; dkim=pass header.i=@ibm.com header.s=pp1 header.b=B++XRKSF; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.35 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=REJECT sp=NONE dis=NONE) header.from=ibm.com Received: from out1.vger.email (depot.vger.email [IPv6:2620:137:e000::3:0]) by groat.vger.email (Postfix) with ESMTP id 90A2A8078C81; Sat, 21 Oct 2023 02:44:24 -0700 (PDT) X-Virus-Status: Clean X-Virus-Scanned: clamav-milter 0.103.10 at groat.vger.email Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229980AbjJUJoM (ORCPT <rfc822;a1648639935@gmail.com> + 26 others); Sat, 21 Oct 2023 05:44:12 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:37534 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229574AbjJUJoK (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Sat, 21 Oct 2023 05:44:10 -0400 Received: from mx0a-001b2d01.pphosted.com (mx0a-001b2d01.pphosted.com [148.163.156.1]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 5CCF213E; Sat, 21 Oct 2023 02:44:09 -0700 (PDT) Received: from pps.filterd (m0353727.ppops.net [127.0.0.1]) by mx0a-001b2d01.pphosted.com (8.17.1.19/8.17.1.19) with ESMTP id 39L9gNAp028149; Sat, 21 Oct 2023 09:44:07 GMT DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ibm.com; h=date : from : to : cc : subject : message-id : content-type : mime-version; s=pp1; bh=wiznSzPHeDvgCYmBpLCxQSz2kUh0Y5cwbWCJqEw6UC4=; b=B++XRKSFFkRhrwMFPwI7jYlIAUaBh+ZtowErnbxY77jeVtPjU7UK1UoC/XHpObzaR7xP TwTN5Xnq1+7G/dONwKE+gMpOdfrl8IYNoUTirTcoPptgZ9QsPiww51guYwi1uj9SXG3m hEC+MtD531KNTi40qxOdAy9GrNanVf6rU0xMUjrHZD7cA0ZsXIh68IzMpD/6e5oed5+4 5Mwjap40CtfCo64cvNNKffI7gsVbo14uz1vfEf6xOCS6QOMGha8aILYVHmfspW01n3lf o2zfzK4k/Db2GoBRaUgfYg1E3ET2napegbcadpmXxsRGZQsWAHF7DADfEJ56nlE+COeN pw== Received: from ppma21.wdc07v.mail.ibm.com (5b.69.3da9.ip4.static.sl-reverse.com [169.61.105.91]) by mx0a-001b2d01.pphosted.com (PPS) with ESMTPS id 3tvc88r1e3-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Sat, 21 Oct 2023 09:44:06 +0000 Received: from pps.filterd (ppma21.wdc07v.mail.ibm.com [127.0.0.1]) by ppma21.wdc07v.mail.ibm.com (8.17.1.19/8.17.1.19) with ESMTP id 39L5rstq024179; Sat, 21 Oct 2023 09:44:05 GMT Received: from smtprelay05.fra02v.mail.ibm.com ([9.218.2.225]) by ppma21.wdc07v.mail.ibm.com (PPS) with ESMTPS id 3tuc29akkr-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Sat, 21 Oct 2023 09:44:05 +0000 Received: from smtpav02.fra02v.mail.ibm.com (smtpav02.fra02v.mail.ibm.com [10.20.54.101]) by smtprelay05.fra02v.mail.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id 39L9i2eU000512 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Sat, 21 Oct 2023 09:44:02 GMT Received: from smtpav02.fra02v.mail.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id EE6CB2004E; Sat, 21 Oct 2023 09:44:01 +0000 (GMT) Received: from smtpav02.fra02v.mail.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id 9C82620043; Sat, 21 Oct 2023 09:44:01 +0000 (GMT) Received: from localhost (unknown [9.179.5.188]) by smtpav02.fra02v.mail.ibm.com (Postfix) with ESMTPS; Sat, 21 Oct 2023 09:44:01 +0000 (GMT) Date: Sat, 21 Oct 2023 11:44:00 +0200 From: Vasily Gorbik <gor@linux.ibm.com> To: Linus Torvalds <torvalds@linux-foundation.org> Cc: Heiko Carstens <hca@linux.ibm.com>, Alexander Gordeev <agordeev@linux.ibm.com>, linux-kernel@vger.kernel.org, linux-s390@vger.kernel.org Subject: [GIT PULL] s390 fixes for 6.6-rc7 Message-ID: <your-ad-here.call-01697881440-ext-2458@work.hours> Content-Type: text/plain; charset=utf-8 Content-Disposition: inline X-TM-AS-GCONF: 00 X-Proofpoint-GUID: cJbZbqwRyhTlqxB2eXYiZ6KhBivFH6Ep X-Proofpoint-ORIG-GUID: cJbZbqwRyhTlqxB2eXYiZ6KhBivFH6Ep X-Proofpoint-UnRewURL: 0 URL was un-rewritten MIME-Version: 1.0 X-Proofpoint-Virus-Version: vendor=baseguard engine=ICAP:2.0.272,Aquarius:18.0.980,Hydra:6.0.619,FMLib:17.11.176.26 definitions=2023-10-20_10,2023-10-19_01,2023-05-22_02 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 impostorscore=0 lowpriorityscore=0 phishscore=0 spamscore=0 clxscore=1011 mlxscore=0 adultscore=0 priorityscore=1501 suspectscore=0 bulkscore=0 mlxlogscore=602 malwarescore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2310170001 definitions=main-2310210088 X-Spam-Status: No, score=-0.8 required=5.0 tests=DKIM_SIGNED,DKIM_VALID, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE, SPF_PASS autolearn=unavailable autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on groat.vger.email Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (groat.vger.email [0.0.0.0]); Sat, 21 Oct 2023 02:44:24 -0700 (PDT) X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1780357756964414107 X-GMAIL-MSGID: 1780357756964414107 |
Series |
[GIT,PULL] s390 fixes for 6.6-rc7
|
|
Pull-request
git://git.kernel.org/pub/scm/linux/kernel/git/s390/linux.git tags/s390-6.6-4Message
Vasily Gorbik
Oct. 21, 2023, 9:44 a.m. UTC
Hello Linus, please pull s390 fixes for 6.6-rc7. Thank you, Vasily The following changes since commit 5c95bf274665cc9f5126e4a48a9da51114f7afd2: s390/cert_store: fix string length handling (2023-09-19 13:25:44 +0200) are available in the Git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/s390/linux.git tags/s390-6.6-4 for you to fetch changes up to c1ae1c59c8c6e0b66a718308c623e0cb394dab6b: s390/pci: fix iommu bitmap allocation (2023-10-19 16:35:41 +0200) ---------------------------------------------------------------- s390 updates for 6.6-rc7 - Fix IOMMU bitmap allocation in s390 PCI to avoid out of bounds access when IOMMU pages aren't a multiple of 64. - Fix kasan crashes when accessing DCSS mapping in memory holes by adding corresponding kasan zero shadow mappings. - Fix a memory leak in css_alloc_subchannel in case dma_set_coherent_mask fails. ---------------------------------------------------------------- Dinghao Liu (1): s390/cio: fix a memleak in css_alloc_subchannel Niklas Schnelle (1): s390/pci: fix iommu bitmap allocation Vasily Gorbik (1): s390/kasan: handle DCSS mapping in memory holes arch/s390/boot/vmem.c | 7 ++++++- arch/s390/pci/pci_dma.c | 15 +++++++++++++-- drivers/s390/cio/css.c | 6 ++++-- 3 files changed, 23 insertions(+), 5 deletions(-)
Comments
On Sat, 21 Oct 2023 at 02:44, Vasily Gorbik <gor@linux.ibm.com> wrote: > > please pull s390 fixes for 6.6-rc7. Pulled. HOWEVER. > - Fix IOMMU bitmap allocation in s390 PCI to avoid out of bounds access > when IOMMU pages aren't a multiple of 64. Please don't do this kind of thing. And I quote: static unsigned long *bitmap_vzalloc(size_t bits, gfp_t flags) { size_t n = BITS_TO_LONGS(bits); size_t bytes; if (unlikely(check_mul_overflow(n, sizeof(unsigned long), &bytes))) return NULL; return vzalloc(bytes); } the above overflow handling is *not* "defensive and good programming". The above is just "unreadable mindless boiler plate". Seriously, you're taking a 'size_t' of number of bits, turning it into number of longs, and you're then turning *that* into number of bytes, AND YOU ADD OVERFLOW CHECKING?!??!!! Now, to make matters worse, the above calculation can actually overflow in theory - but not in the place where you added the protection! Because the "longs to bytes" sure as hell can't overflow. We know that, because the number of longs is guaranteed to have a much smaller range, since it came from a calculation of bits. But what can actually overflow? BITS_TO_LONGS(bits) will overflow, and turn ~0ul to 0, because it does the __KERNEL_DIV_ROUND_UP thing, which is the simplistic #define __KERNEL_DIV_ROUND_UP(n, d) (((n) + (d) - 1) / (d)) so that code added overflow protection that doesn't make sense, in all the wrong places. You need to verify the sanity of the number of bits first anyway. Of course, in your use-case, the number of bits is also not unlimited, because the source is zdev->iommu_pages = zdev->iommu_size >> PAGE_SHIFT; so it turns out that no, the BITS_TO_LONGS() won't overflow either, but at least in some other situations - and only looking at that bitmap_vzalloc() in a vacuum - it *could* have. Now, I will argue that you always need range checking on the number of bits *anyway* for other reasons - trying to just blindly allocate some random amount of memory isn't acceptable, so there should to be some range checking before anyway. But that code is wrong, because the overflow is simply not an issue. Adding overflow handling code is literally only actively misleading, making the code harder to read, for no reason, and making people *think* it's being careful when it is anything *but* careful. I suspect that the compiler actually sees "that is stupid" and turns the overflow into just a single left-shift again because it has seen the (bigger) right-shift and knows it cannot overflow, but the problem I'm ranting against is mindlessly adding boiler plate code that makes the code harder to read for *humans*. If you *do* want to add proper overflow handling, you'd need to either fix BITS_TO_LONGS() some way (which is actually non-trivial since it needs to be able to stay a constant and only use the argument once), or you do something like if (!bits) return ZERO_SIZE_PTR; longs = BITS_TO_LONG(bits); if (!longs) return NULL; return vzalloc(longs * sizeof(long)); and I'd suggest maybe we should (a) do the above checking in our bitmap_alloc() routines (b) also change our bitmap_alloc() routines to take 'size_t' instead of 'unsigned int' bit counts (c) and finally, add that vzalloc() case, but simply using kvmalloc_array(n, size, flags | __GFP_ZERO); instead. (And yes, kvmalloc_array() will actually then also do that check_mul_overflow() thing, but now it's not pointless boiler plate any more, now it's actually meaningful for *other* cases than the bitmap allocation one that cannot overflow). Hmm? Linus
The pull request you sent on Sat, 21 Oct 2023 11:44:00 +0200:
> git://git.kernel.org/pub/scm/linux/kernel/git/s390/linux.git tags/s390-6.6-4
has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/4d7b04c0cda365f190c4a8f7fddc535b93aae9f9
Thank you!
Just re-quoting my suggestion here and adding Andy and Dmitry, who did the original bitmap_alloc() helper interfaces a few years ago. Also adding Kees in case he has any hardening suggestions, since this is about (incorrect) overflow handling. Kees: see my rant about mindlessly doing overflow handling in the wrong place in https://lore.kernel.org/all/CAHk-=wgTUz1bdY6zvsN4ED0arCLE8Sb==1GH8d0sjm5bu7zesQ@mail.gmail.com/ in case you or somebody has a better idea for BITS_TO_LONG handling than just "you need to check for zero before and after". Linus On Sat, 21 Oct 2023 at 10:56, Linus Torvalds <torvalds@linux-foundation.org> wrote: > > If you *do* want to add proper overflow handling, you'd need to either > fix BITS_TO_LONGS() some way (which is actually non-trivial since it > needs to be able to stay a constant and only use the argument once), > or you do something like > > if (!bits) > return ZERO_SIZE_PTR; > longs = BITS_TO_LONG(bits); > if (!longs) > return NULL; > return vzalloc(longs * sizeof(long)); > > and I'd suggest maybe we should > > (a) do the above checking in our bitmap_alloc() routines > > (b) also change our bitmap_alloc() routines to take 'size_t' instead > of 'unsigned int' bit counts > > (c) and finally, add that vzalloc() case, but simply using > > kvmalloc_array(n, size, flags | __GFP_ZERO); > > instead.
On Sat, Oct 21, 2023 at 11:08:31AM -0700, Linus Torvalds wrote: > in case you or somebody has a better idea for BITS_TO_LONG handling > than just "you need to check for zero before and after". > > On Sat, 21 Oct 2023 at 10:56, Linus Torvalds > <torvalds@linux-foundation.org> wrote: > > > > If you *do* want to add proper overflow handling, you'd need to either > > fix BITS_TO_LONGS() some way (which is actually non-trivial since it > > needs to be able to stay a constant and only use the argument once), > > or you do something like > > > > if (!bits) > > return ZERO_SIZE_PTR; > > longs = BITS_TO_LONG(bits); > > if (!longs) > > return NULL; > > return vzalloc(longs * sizeof(long)); This might work. BITS_TO_<TYPE>(bits) utilizes __KERNEL_DIV_ROUND_UP, which may potentially result in an overflow condition when bits > ULONG_MAX - sizeof(<TYPE>) * 8 + 1. To resolve this issue, avoid using the overflow-prone __KERNEL_DIV_ROUND_UP. To meet the requirements of BITS_TO<TYPE>(bits) for remaining constant and preventing side effects from multiple argument uses, employ __is_constexpr to differentiate between constant and non-constant cases, employing a helper function in the latter. In the constant case, this ensures compatibility with constructs like DECLARE_BITMAP. While in the non-constant case, the __bits_to_elem_count function could be optimized for potentially improved code generation by compilers, though this might come at the expense of readability and visual consistency between the constant and non-constant cases. I could further investigate if this approach, in general, appears acceptable. --- include/linux/bitops.h | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/include/linux/bitops.h b/include/linux/bitops.h index 2ba557e067fe..72be25d4b95d 100644 --- a/include/linux/bitops.h +++ b/include/linux/bitops.h @@ -15,11 +15,21 @@ # define aligned_byte_mask(n) (~0xffUL << (BITS_PER_LONG - 8 - 8*(n))) #endif +static inline unsigned long __bits_to_elem_count(size_t nr, size_t sz) +{ + return nr / sz + (nr % sz ? 1 : 0); +} + +#define BITS_TO_ELEM_COUNT(nr, sz) \ + __builtin_choose_expr(__is_constexpr(nr), \ + (nr) / sz + ((nr) % sz ? 1 : 0), \ + __bits_to_elem_count((nr), sz)) + #define BITS_PER_TYPE(type) (sizeof(type) * BITS_PER_BYTE) -#define BITS_TO_LONGS(nr) __KERNEL_DIV_ROUND_UP(nr, BITS_PER_TYPE(long)) -#define BITS_TO_U64(nr) __KERNEL_DIV_ROUND_UP(nr, BITS_PER_TYPE(u64)) -#define BITS_TO_U32(nr) __KERNEL_DIV_ROUND_UP(nr, BITS_PER_TYPE(u32)) -#define BITS_TO_BYTES(nr) __KERNEL_DIV_ROUND_UP(nr, BITS_PER_TYPE(char)) +#define BITS_TO_LONGS(nr) BITS_TO_ELEM_COUNT(nr, BITS_PER_TYPE(long)) +#define BITS_TO_U64(nr) BITS_TO_ELEM_COUNT(nr, BITS_PER_TYPE(u64)) +#define BITS_TO_U32(nr) BITS_TO_ELEM_COUNT(nr, BITS_PER_TYPE(u32)) +#define BITS_TO_BYTES(nr) BITS_TO_ELEM_COUNT(nr, BITS_PER_TYPE(char)) extern unsigned int __sw_hweight8(unsigned int w); extern unsigned int __sw_hweight16(unsigned int w);
On Sun, 22 Oct 2023 at 06:18, Vasily Gorbik <gor@linux.ibm.com> wrote: > > This might work. Hmm. Yes. But let's fix __KERNEL_DIV_ROUND_UP itself while at it. (And perhaps move it out of the odd location it is in now - its in <uapi/linux/const.h> for some unfathomable reason) And maybe we could do a helper like #define __if_constexpr(x, a, b) \ __builtin_choose_expr(__is_constexpr(x), a, b) since that is one of the main reasons for that __is_constexpr macro (and _that_ makes sense in the const.h header file). Linus
On Sat, Oct 21, 2023 at 10:56:29AM -0700, Linus Torvalds wrote: > On Sat, 21 Oct 2023 at 02:44, Vasily Gorbik <gor@linux.ibm.com> wrote: > > - Fix IOMMU bitmap allocation in s390 PCI to avoid out of bounds access > > when IOMMU pages aren't a multiple of 64. > But that code is wrong, because the overflow is simply not an issue. > Adding overflow handling code is literally only actively misleading, > making the code harder to read, for no reason, and making people > *think* it's being careful when it is anything *but* careful. Right, I should have done a better job reviewing this patch when picking it up. Please consider a follow-up patch (in reply) that cleans up unnecessary and misleading overflow handling. There is no real benefit in getting it into linux-next because the upcoming conversion to use the common code DMA API on s390 Link: https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?h=next-20231020&id=c76c067e488c eliminates arch/s390/pci/pci_dma.c entirely and, therefore, addresses the original problem in another way. That's why this fix is only relevant for the current v6.6 and stable backports and is kept as simple as possible. Let me know if you prefer the regular way, and I should include this follow-up patch in my pull request later this week. > If you *do* want to add proper overflow handling, you'd need to either > fix BITS_TO_LONGS() some way (which is actually non-trivial since it > needs to be able to stay a constant and only use the argument once), Looking into that. Let's see if handling overflow in __KERNEL_DIV_ROUND_UP turns out to be doable.