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 2E93BC4345F for ; Wed, 17 Apr 2024 11:10:07 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 85DB36B0087; Wed, 17 Apr 2024 07:10:06 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 80DD66B0088; Wed, 17 Apr 2024 07:10:06 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 6D5866B0089; Wed, 17 Apr 2024 07:10:06 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0016.hostedemail.com [216.40.44.16]) by kanga.kvack.org (Postfix) with ESMTP id 521176B0087 for ; Wed, 17 Apr 2024 07:10:06 -0400 (EDT) Received: from smtpin23.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay06.hostedemail.com (Postfix) with ESMTP id 0ABDCA1423 for ; Wed, 17 Apr 2024 11:10:06 +0000 (UTC) X-FDA: 82018754412.23.713B5B9 Received: from smtp-out1.suse.de (smtp-out1.suse.de [195.135.223.130]) by imf10.hostedemail.com (Postfix) with ESMTP id 80C1AC0005 for ; Wed, 17 Apr 2024 11:10:03 +0000 (UTC) Authentication-Results: imf10.hostedemail.com; dkim=pass header.d=suse.cz header.s=susede2_rsa header.b=Em8TZBwG; dkim=pass header.d=suse.cz header.s=susede2_ed25519 header.b=Swm0bX9A; dkim=pass header.d=suse.cz header.s=susede2_rsa header.b=Em8TZBwG; dkim=pass header.d=suse.cz header.s=susede2_ed25519 header.b=Swm0bX9A; spf=pass (imf10.hostedemail.com: domain of jack@suse.cz designates 195.135.223.130 as permitted sender) smtp.mailfrom=jack@suse.cz; dmarc=none ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1713352204; 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: in-reply-to:in-reply-to:references:references:dkim-signature; bh=2qaQbAuymIyWXl7clH/m9soo1SrBAkUCeIKbNu703u8=; b=JmyWzQgBlfv4k7yi3ybn+EloRr8XhIApcQhuKswf1kVdGF9aXwOj7JeRUFtfpJA7NCqj1E r3lVi1fqJLwCn8OU0Pe/yVoRtLzyxadUmUKyx3rrP4ir1tThQAiCRVbY//5aoap9hMhI08 e6sln0xIBVfB21yQbNt9U8EMie6Nzbs= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1713352204; a=rsa-sha256; cv=none; b=yzbwD8smJeQ0LzBmEbP5GrqNnpEaYHRnSja2SIGDCM2cklcjFGtNsc9Ov8YDOP5O/9JHgh yTLSUXNPA5p0Hc0w7LIZItAGk8Nii9uaLqMzG8fWN97vdg9G97I8wmljwUWtg+MaG7XB2i 5GGvdRwGigXsj1CGXc/0PNfuF4p7arg= ARC-Authentication-Results: i=1; imf10.hostedemail.com; dkim=pass header.d=suse.cz header.s=susede2_rsa header.b=Em8TZBwG; dkim=pass header.d=suse.cz header.s=susede2_ed25519 header.b=Swm0bX9A; dkim=pass header.d=suse.cz header.s=susede2_rsa header.b=Em8TZBwG; dkim=pass header.d=suse.cz header.s=susede2_ed25519 header.b=Swm0bX9A; spf=pass (imf10.hostedemail.com: domain of jack@suse.cz designates 195.135.223.130 as permitted sender) smtp.mailfrom=jack@suse.cz; dmarc=none Received: from imap1.dmz-prg2.suse.org (imap1.dmz-prg2.suse.org [10.150.64.97]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) by smtp-out1.suse.de (Postfix) with ESMTPS id C2A8433B67; Wed, 17 Apr 2024 11:10:01 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.cz; s=susede2_rsa; t=1713352201; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=2qaQbAuymIyWXl7clH/m9soo1SrBAkUCeIKbNu703u8=; b=Em8TZBwGsY3v2ac1dac2wkMo6/bVVz1QGJnCr/wV9+xkbk4ee7vqW8WAbuVvioDPZVH3IL p7+vmfE9vRncjj5l3tHv4FYm/p0gDFLJ9s85BbUGThSE9kRgfEqEEpBgu44qhXuWaotQrJ +MfwWf7RXK+KUbtGTUrVJ4OhUIjJMIY= DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=suse.cz; s=susede2_ed25519; t=1713352201; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=2qaQbAuymIyWXl7clH/m9soo1SrBAkUCeIKbNu703u8=; b=Swm0bX9AnQEPsCqCQVNr3NmWbcAYZ93kf67JeTeWR+wrnSK1hLH3moDJuw6je9vqkjosHA AHX9F4HufK9+egAw== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.cz; s=susede2_rsa; t=1713352201; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=2qaQbAuymIyWXl7clH/m9soo1SrBAkUCeIKbNu703u8=; b=Em8TZBwGsY3v2ac1dac2wkMo6/bVVz1QGJnCr/wV9+xkbk4ee7vqW8WAbuVvioDPZVH3IL p7+vmfE9vRncjj5l3tHv4FYm/p0gDFLJ9s85BbUGThSE9kRgfEqEEpBgu44qhXuWaotQrJ +MfwWf7RXK+KUbtGTUrVJ4OhUIjJMIY= DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=suse.cz; s=susede2_ed25519; t=1713352201; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=2qaQbAuymIyWXl7clH/m9soo1SrBAkUCeIKbNu703u8=; b=Swm0bX9AnQEPsCqCQVNr3NmWbcAYZ93kf67JeTeWR+wrnSK1hLH3moDJuw6je9vqkjosHA AHX9F4HufK9+egAw== Received: from imap1.dmz-prg2.suse.org (localhost [127.0.0.1]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) by imap1.dmz-prg2.suse.org (Postfix) with ESMTPS id B5C1C1384C; Wed, 17 Apr 2024 11:10:01 +0000 (UTC) Received: from dovecot-director2.suse.de ([2a07:de40:b281:106:10:150:64:167]) by imap1.dmz-prg2.suse.org with ESMTPSA id 45JdLAmuH2bAfgAAD6G6ig (envelope-from ); Wed, 17 Apr 2024 11:10:01 +0000 Received: by quack3.suse.cz (Postfix, from userid 1000) id 5911AA082E; Wed, 17 Apr 2024 13:10:01 +0200 (CEST) Date: Wed, 17 Apr 2024 13:10:01 +0200 From: Jan Kara To: Andrew Morton Cc: linux-mm@kvack.org, Zach O'Keefe , linux-kernel@vger.kernel.org, Maxim Patlasov , stable@vger.kernel.org Subject: Re: [PATCH] mm/writeback: fix possible divide-by-zero in wb_dirty_limits(), again Message-ID: <20240417111001.fa2eg5gp6t2wiwco@quack3> References: <20240118181954.1415197-1-zokeefe@google.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20240118181954.1415197-1-zokeefe@google.com> X-Rspamd-Server: rspam12 X-Rspamd-Queue-Id: 80C1AC0005 X-Rspam-User: X-Stat-Signature: 7upfoycakufeonkb1gb8n5i5w5eiuffx X-HE-Tag: 1713352203-203474 X-HE-Meta: U2FsdGVkX19C9uBVt04QIjFVnQKCtit7lx7YiHntSNFxS0PrfBn96KwNJxFIF8F2afQVpksmYx53rRjDYRRawg6zsQu2TXnpqPt84yqw/m4HQRAaV05Ys4J+0k/wjpMbneHGFWSsC/xF5enmSAPoDGDApWuLLu8UxhXZcDOcIo+le+s3JKWpwa6K86ENJwDoD68T69dv59rOsfz+wREIyGBMCHb4nEFUN+VjF7gbRO23f4tef3w+vB5wZ4HtU1CwuqrDgFYRuqYJ4NJLuQXqpGTuqTH03eEzccx9+mv+hzoTyiJxXxnkfzTmAisTxa7t7p4uBBcTmcC+iaIOSJx1DSlGJH4NZRqCLnEtS0Md/6BNcxhndnxqgmMdsQoMrdx4P6ozN+znqNojWW29EOD2t8OGH5GS6sM77yoUWlDqcxY6p+neqc1/kRLWE0gYfWrlSXpaPoDqL5xiOIDdTxwFbJn2Ah3ShkXq58jWDuHNi+aShseOktrCItUhYn90zb6L/LKWgXDbTLXh2Ix+4tj3gj/nmVtgMTpeJCiyjLwTdEOY0wVem1OIDxU3GrI72Io/D+fw+36omzPAC2tukHoGBn6rpPCPgewWJ9dGibIf/kv4TxXqgbWIykNeb1bS4pP+r3/6jj9wFIeAPwr0VmAPKi/PxZ7e1FypI5kFvelAla33uh+AUEZYf11KEFqEkDAfUnVY/+V72SRf9Q5RLr20oGrB4vHVMRHFLkCCnts5BgxbbgvlRg2MTu9OIeNCPofAvbqtZOmQhz6massOlptV4Md8OWAgZabXrXF0hv9/kSONLM76mMh82wFuqBPNlBoG80XsZ6MyucFrKTT2exKnGEyc1RcjvV/cxqGJz7GRIq/e9JrWnRuT8bE/chSnCurwmrDi40VKKjB2XkV8wTr3bBurXzygfuzzHFDEXzfRH0RNNdJg0hGTqsPGJuXZ+O+0nLwrwntyuxvXHeEJVKX 1M5MiKNv JN5Lwy7OudDPWoNpEUuZajYQZxQu0RhbJAi3o2TIFTaPxMhZix72gpxbu7KCiXsUpG+iKAkqA/S/Y0D8zO5feAmH+7jC+O41g/IqgP4HoHFad6huA4zA1Vitroe8CNQf8DLkb85UsUr88IXAdgFOYN9seGhZoXSdrzS5nB9ObOGubUzRMgro1ZQv7TeNrb3tpjCe9WHcc6Le7lKpKMlfnf9Dwt++vtF04SpeXLB9eNa3p8F08HcEGzX9RyocYwTUlTuLO8TegMlsji+9LoiKAcL6xyHjdyWBu5gmm6CipNjHztpj1VgJl0KpqtG4yUZvdpV6w5GUH4aQ6OU2p6dVU06SeCzxM4icGkbKp0Vo7Mgm7/vW4tObVb1jLr83R2uaoboXMw5SjyW8QA2YV8wEemvtgOa/cbHj+tC1DYlBbcR/QboqjSAEe8XO19igaoI2hR16ZPHbj41Sh3UiqPTQrEFbt7A== 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 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 where > 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 in > global writeback domain with a BDI_CAP_STRICTLIMIT-backing device, using > e.g. vm.dirty_bytes=(1<<32)*PAGE_SIZE so that dtc->thresh == (1<<32) > > Fixes: f6789593d5ce ("mm/page-writeback.c: fix divide by zero in bdi_dirty_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: > 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_throttle_control *dtc) > */ > dtc->wb_thresh = __wb_calc_thresh(dtc); > dtc->wb_bg_thresh = 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 >= 1<<32 (which is actually common - the default settings with 4GB of RAM will trigger this). Secondly, the div64_u64() is unnecessarily expensive on 32-bit archs. We have div64_ul() in case we want to be safe & cheap. 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 anymore. The whole dirty limiting code is interspersed with assumptions that limits 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 bits 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 far it does more harm than good... Thanks! Honza -- Jan Kara SUSE Labs, CR