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 Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id 5D4A9C4345F for ; Wed, 17 Apr 2024 19:34:21 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id E75386B008C; Wed, 17 Apr 2024 15:34:20 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id E25786B0095; Wed, 17 Apr 2024 15:34:20 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id CED706B009A; Wed, 17 Apr 2024 15:34:20 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0015.hostedemail.com [216.40.44.15]) by kanga.kvack.org (Postfix) with ESMTP id AD5036B008C for ; Wed, 17 Apr 2024 15:34:20 -0400 (EDT) Received: from smtpin23.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay07.hostedemail.com (Postfix) with ESMTP id 5A757160F73 for ; Wed, 17 Apr 2024 19:34:20 +0000 (UTC) X-FDA: 82020025080.23.2CD4877 Received: from mail-ed1-f51.google.com (mail-ed1-f51.google.com [209.85.208.51]) by imf04.hostedemail.com (Postfix) with ESMTP id 85B7A40013 for ; Wed, 17 Apr 2024 19:34:18 +0000 (UTC) Authentication-Results: imf04.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b="ka/00Nvl"; dmarc=pass (policy=reject) header.from=google.com; spf=pass (imf04.hostedemail.com: domain of zokeefe@google.com designates 209.85.208.51 as permitted sender) smtp.mailfrom=zokeefe@google.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1713382458; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=Fyl6TqOQKc7y6AuWZSULZsHIQoiSS+3/2ZOsE3/WMtU=; b=Nk2So0SE/G2fsm7AIbmeixoYRI92139k5Lu7IsVQBvLHAqGADATK9hUangwwVLSgOaooAh GIAQunggfxFOCkiRVjWLxrABgr1Xm34NTDZ2PiLL1IB4YxutR43WSfAJbiPWpV4jXkV8f2 QL1rqlYff5M96H9N4qXBcmAammZbIxI= ARC-Authentication-Results: i=1; imf04.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b="ka/00Nvl"; dmarc=pass (policy=reject) header.from=google.com; spf=pass (imf04.hostedemail.com: domain of zokeefe@google.com designates 209.85.208.51 as permitted sender) smtp.mailfrom=zokeefe@google.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1713382458; a=rsa-sha256; cv=none; b=CeTp3TkbR9PCVG+LrkLpRzUwgkBJQZQ3S2oZcdMljLw9MsRuDXx9AaEaboipccDjnxsTAX m/LSM2NC1Wfn6T7vxU4GnuGCaYoRiHim4uh5sT8W3w5u6XuzjM767RZSElrXQ3XjskSrJW bHffJg6v5ioiIlZCzEfiKB/ScaGSVV0= Received: by mail-ed1-f51.google.com with SMTP id 4fb4d7f45d1cf-56e5174ffc2so2762a12.1 for ; Wed, 17 Apr 2024 12:34:18 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20230601; t=1713382457; x=1713987257; darn=kvack.org; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=Fyl6TqOQKc7y6AuWZSULZsHIQoiSS+3/2ZOsE3/WMtU=; b=ka/00NvlkIcvxpnsrQ1+Tf3DsPoynxNERXFimrmR+Uk2cam+M1NBhtEP6oJVVtKEKF LLzR9EO+HefJzkwncrDLAY7f8qmj24lpGrb9vwHiQjHWvsHh5DDWhTER4dCe0FB7y394 bJ6B3kA6YyQeC3U0b9mhI6+NBMWLimcxxVwL2LOtNWPj65VzeckOiczngjbJOpo8JGnV dkd9sqDjgWIBhdDD+1X4eY5KvwDhmAGuUZqaYljdPY3kkouj8MLJ+MT2yuEnswV/AF4c W4QHmMfubJlLatv1OepnarkK/6tIuvq3rGTcAWKlzt5+Mqc7hGw7Pr/KfJN7Drw1GMz6 WFdg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1713382457; x=1713987257; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=Fyl6TqOQKc7y6AuWZSULZsHIQoiSS+3/2ZOsE3/WMtU=; b=KCa884RYTh+4VWF6Htr0CmzeGI3fjMmJVmUIIheRDqw553Qp1Hy61wuI0BKHBf4PzU 5bz3jbYD+Ci+jf8Iz9b1Yfx3EIjR464f96Td0nImzY+92onk632Fxv2kLewn9ZLy9tEN 42umgrIPHkGkTLUCRndbuYjqdjv1JnQwFe2V0VYqeaGuccruHmWA74voBPs9/QYpH4qV ZMYu88dMibUgEjknvItJsK8kHeHvbJ5b+cdgZyyJMCrswGI1DbkoaVzxAxPzxTCqqiTl LJSxU0zC/mjGSpw78UEeMaA2xFGxxFGtLjiUwulZ3zhYsE1KJrxeMtumEmP1hMQpSbBC I7Hw== X-Forwarded-Encrypted: i=1; AJvYcCU57/apkQnOMJUvvuEUyZd+nD/nEk8Vg6To2RV8P0OxSOuvQ5bQgpLJE41/Gcd2dhIczNBe4HxDm4OpSaqUbLBZuio= X-Gm-Message-State: AOJu0YwzdotTegXfAqDWi9XryeYNo3laP43BRuB1+H76PjBn25tNNUnJ VtmbmvRGOWd4LieNooaJTU5Mo8IkPu3Tf/xgzvKtxecbbem6Sw3TxTx1Bz0NICJcPjKES6OjFNZ 3gACj/otStU1bltmSzFfM2Ov0H+dCdHuGUHjK X-Google-Smtp-Source: AGHT+IF3BxF61WJZfffzn5eL5CSWhQKAZF34LWBLquyLGLML0FvYcr/UD2V+jp/gbT9bVgfJHdkvmNN7KBf/Xf/oMBk= X-Received: by 2002:a05:6402:12d2:b0:571:b3fa:bf81 with SMTP id k18-20020a05640212d200b00571b3fabf81mr29493edx.2.1713382456805; Wed, 17 Apr 2024 12:34:16 -0700 (PDT) MIME-Version: 1.0 References: <20240118181954.1415197-1-zokeefe@google.com> <20240417111001.fa2eg5gp6t2wiwco@quack3> In-Reply-To: <20240417111001.fa2eg5gp6t2wiwco@quack3> From: "Zach O'Keefe" Date: Wed, 17 Apr 2024 12:33:39 -0700 Message-ID: Subject: Re: [PATCH] mm/writeback: fix possible divide-by-zero in wb_dirty_limits(), again To: Jan Kara Cc: Andrew Morton , linux-mm@kvack.org, linux-kernel@vger.kernel.org, Maxim Patlasov , stable@vger.kernel.org Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Rspamd-Server: rspam04 X-Rspamd-Queue-Id: 85B7A40013 X-Rspam-User: X-Stat-Signature: 74yrxz717qe3xwripfxip458g9iggj3q X-HE-Tag: 1713382458-316318 X-HE-Meta: U2FsdGVkX19PEfiwT82O4/ziTKJdkW+iuMzCqMVVYVHq1uKuRlfcQ4zknscf5mpPyqjXIwwFcKMm9UOpaG3HyCz2ixmtj3KfivXN3GFqvyOSf+/AEGC9JKoE3TZow/y/rvKKop0RC5RjRFLFKXy/6DE/MkGrENHuuwaOR4BcKCo8mBLHjemc7ZiA+oAoHdALr5eAw0b42GYaR7jma53lWLbgGirBYlyFeueBPlyZPXNYfF0gWfzncLL/JkD8uMqmFzzEvvYxNvgveKGStPWZ6t49a4cLKFCdt9RxUMZkVd3HLTmr3Aba4qAhc1oymWFTmkbNgdB0OiCaNGYk5spT0Mz7xJeqWI5SKRc64yWYDp8ChgtclL6pW+5GzSEP1ygOXkHF3dLay+2pHpva2BlrKD984Vadl7badXqPYZuwj+4jthAQuC5VuP2e6ia3SWjYmFnSb4VvNVqCcIsnvZpcZoKRDWcBg4cfQJOk0L3s/uQIxos16/An8SkSJS2QOx7mUv9UzNkhu2icvZNXido9jlF3377NnaIzvlCeFehR7TX2Sjv+Idie1nDsR6yaqFKDI9pBIe6ZjReVaJdVo9NFsYyjQSPftuSawWTod0xYIOja+abQeP8IV6YgMMyU1li01/pOwJXdvSW28fmYOTH1oPdj54mEMDNIwMD9PezoNkrFqtuB077YGuRK3JQIKRTG8y0Zx98/DDziMgPyIqH9fFjV+ZDjIbxHkeAn2Qsfy5/pWqFx8FBAo8GrHYHo+GF4x7yIe/Aeu8BtgI2tCjS9Y6oEkawadp1aQBmz2AAfd0ZgzCoOQm/WmVMuuDpKhwfc+AcLAGiB92e5d3iZXzN8/DWaJrvukHVCxYDbb8NFbnHV/SNa6P0GKQA/gh8q3eHeVHqD8/QlCEdRM5dnxJN2SoNYvtuJ+3h49V8gIyxZk3sQ9mZoV9FSqeos+IHElI0d8msRfZRTYM9hd9XOidS GDxXHPy3 E6QQ8VugVv1495FkDKnh1fBnExHImCtnEY2uqp9Vlnjqf2DpjwZRZX0Ad/5SlgFYk8Gwde9OizjXe5gSDktgFh78pRL7N07gErPGxPXerKjhHOc6P3QOAElQl5Iu357bSkhiB5HSxYCYvQbO1poMXhSvZpa8puqVj4xbJTVzVoClwUYpQNjQ9R10ThwPKWUl3axg5sLd0+cODr2nHKkKSDO2xZVE/IPB3Aj76GjTq7XgcFm4VBJEpgozHMgOK/7r8zwWP8EzCXvmU1YGVlmombmCy1LNdIklsrgl/pH4GDBbNrT0h6Bh/8ulN3TGQC27XIymuDgvpfZFdLyjFNrU030PSV4P2DNW/PujlzOpZeWy9G1c3JviKv6udbaEBFR2/nHA9WHvl7MRkD/fD5tGlA/9Djmuqxi9gmTEXhAah496bHW8Id+a6lI09JINC1FhwAtKc3vM4JoBOKhNgDLKwwU9Ddg== X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: List-Subscribe: List-Unsubscribe: On Wed, Apr 17, 2024 at 4:10=E2=80=AFAM Jan Kara wrote: > > On Thu 18-01-24 10:19:53, Zach O'Keefe wrote: > > (struct dirty_throttle_control *)->thresh is an unsigned long, but is > > passed as the u32 divisor argument to div_u64(). On architectures wher= e > > unsigned long is 64 bytes, the argument will be implicitly truncated. > > > > Use div64_u64() instead of div_u64() so that the value used in the "is > > this a safe division" check is the same as the divisor. > > > > Also, remove redundant cast of the numerator to u64, as that should > > happen implicitly. > > > > This would be difficult to exploit in memcg domain, given the > > ratio-based arithmetic domain_drity_limits() uses, but is much easier i= n > > global writeback domain with a BDI_CAP_STRICTLIMIT-backing device, usin= g > > e.g. vm.dirty_bytes=3D(1<<32)*PAGE_SIZE so that dtc->thresh =3D=3D (1<<= 32) > > > > Fixes: f6789593d5ce ("mm/page-writeback.c: fix divide by zero in bdi_di= rty_limits()") > > Cc: Maxim Patlasov > > Cc: > > Signed-off-by: Zach O'Keefe > > I've come across this change today and it is broken in several ways: Thanks for picking up on this, Jan. > > diff --git a/mm/page-writeback.c b/mm/page-writeback.c > > index cd4e4ae77c40a..02147b61712bc 100644 > > --- a/mm/page-writeback.c > > +++ b/mm/page-writeback.c > > @@ -1638,7 +1638,7 @@ static inline void wb_dirty_limits(struct dirty_t= hrottle_control *dtc) > > */ > > dtc->wb_thresh =3D __wb_calc_thresh(dtc); > > dtc->wb_bg_thresh =3D dtc->thresh ? > > - div_u64((u64)dtc->wb_thresh * dtc->bg_thresh, dtc->thresh= ) : 0; > > + div64_u64(dtc->wb_thresh * dtc->bg_thresh, dtc->thresh) := 0; > > Firstly, the removed (u64) cast from the multiplication will introduce a > multiplication overflow on 32-bit archs if wb_thresh * bg_thresh >=3D 1<<= 32 > (which is actually common - the default settings with 4GB of RAM will > trigger this). [..] True, and embarrassing given I was looking at this code with a 32-bit focus. Well spotted. > [..] Secondly, the div64_u64() is unnecessarily expensive on > 32-bit archs. We have div64_ul() in case we want to be safe & cheap. A last-minute change vs just casting the initial "dtc->thresh ?" check. It did look expensive, but figured its existence implied it should be used. I must have missed div64_ul(). > Thirdly, if thresholds are larger than 1<<32 pages, then dirty balancing = is > going to blow up in many other spectacular ways - consider only the > multiplication on this line - it will not necessarily fit into u64 anymor= e. > The whole dirty limiting code is interspersed with assumptions that limit= s > are actually within u32 and we do our calculations in unsigned longs to > avoid worrying about overflows (with occasional typing to u64 to make it > more interesting because people expected those entities to overflow 32 bi= ts > even on 32-bit archs). Which is lame I agree but so far people don't seem > to be setting limits to 16TB or more. And I'm not really worried about > security here since this is global-root-only tunable and that has much > better ways to DoS the system. > > So overall I'm all for cleaning up this code but in a sensible way please= . > E.g. for these overflow issues at least do it one function at a time so > that we can sensibly review it. > > Andrew, can you please revert this patch until we have a better fix? So f= ar > it does more harm than good... Thanks! Shall we just roll-forward with a suitable fix? I think all the original code actually "needed" was to cast the ternary predicate, like: ---8<--- diff --git a/mm/page-writeback.c b/mm/page-writeback.c index fba324e1a010..ca1bfc0c9bdd 100644 --- a/mm/page-writeback.c +++ b/mm/page-writeback.c @@ -1637,8 +1637,8 @@ static inline void wb_dirty_limits(struct dirty_throttle_control *dtc) * at some rate <=3D (write_bw / 2) for bringing down wb_dirty. */ dtc->wb_thresh =3D __wb_calc_thresh(dtc); - dtc->wb_bg_thresh =3D dtc->thresh ? - div64_u64(dtc->wb_thresh * dtc->bg_thresh, dtc->thresh) : 0= ; + dtc->wb_bg_thresh =3D (u32)dtc->thresh ? + div_u64((u64)dtc->wb_thresh * dtc->bg_thresh, dtc->thresh) = : 0; /* * In order to avoid the stacked BDI deadlock we need ---8<--- Thanks, and apologize for the inconvenience Zach > Honza > -- > Jan Kara > SUSE Labs, CR