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 4C587C369A4 for ; Tue, 8 Apr 2025 14:59:59 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id CB9D06B0011; Tue, 8 Apr 2025 10:59:56 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id C67196B0012; Tue, 8 Apr 2025 10:59:56 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id B13916B0022; Tue, 8 Apr 2025 10:59:56 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0013.hostedemail.com [216.40.44.13]) by kanga.kvack.org (Postfix) with ESMTP id 8ED366B0011 for ; Tue, 8 Apr 2025 10:59:56 -0400 (EDT) Received: from smtpin26.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay04.hostedemail.com (Postfix) with ESMTP id 06A7E1A1120 for ; Tue, 8 Apr 2025 14:59:58 +0000 (UTC) X-FDA: 83311186476.26.AF17D85 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by imf11.hostedemail.com (Postfix) with ESMTP id 64D3140002 for ; Tue, 8 Apr 2025 14:59:56 +0000 (UTC) Authentication-Results: imf11.hostedemail.com; dkim=pass header.d=redhat.com header.s=mimecast20190719 header.b=Yp34o9Ew; spf=pass (imf11.hostedemail.com: domain of bhe@redhat.com designates 170.10.133.124 as permitted sender) smtp.mailfrom=bhe@redhat.com; dmarc=pass (policy=quarantine) header.from=redhat.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1744124396; 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=pCzzTtjq/nn0G4yumyj10VB3i1Cs8+HpPixzKePPsUI=; b=EyY8SQfFRI0xF6VayCJX+yiI0F00selJX1Y+UrhwVnOKKHgq8tE5KCZ5oe4F1jQZMaY7mg yLJ/xtrTuztc+28xKUZSDfCXbvDiKro13UUHkG3JNyMWIqAcYQuUXOyTATG9XRGNUgel1t YttjPA1MwupH/NcVaAjLLZAJ0O3oQoY= ARC-Authentication-Results: i=1; imf11.hostedemail.com; dkim=pass header.d=redhat.com header.s=mimecast20190719 header.b=Yp34o9Ew; spf=pass (imf11.hostedemail.com: domain of bhe@redhat.com designates 170.10.133.124 as permitted sender) smtp.mailfrom=bhe@redhat.com; dmarc=pass (policy=quarantine) header.from=redhat.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1744124396; a=rsa-sha256; cv=none; b=BFcPelml430Ue7UJ3+YB9mzfj6m+zwMV7Z0VC4lO7VffH9vD80l3kxy5fQfX2MnlR7UJAh 4iAF+XIDWShRrGF2oJlBXCWM0QGExIHjOA7w7dotRCtGFdfb+FCY8oTjYbGyfqajdEz7gl BlW/pVcalCkyedHLivT6Xmy6WcmQQug= DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1744124395; h=from:from:reply-to:subject:subject: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=pCzzTtjq/nn0G4yumyj10VB3i1Cs8+HpPixzKePPsUI=; b=Yp34o9EwRzIvrUKpXSzNc7P0Ot474sclJHk6UU8usP8tNn1zXCRclZNvvLkBSzHURnGcQG UxYNcBIdWzKenhPldcMiLP84n4BSLhAcfkwlE9v4nzu5J4UpLKVSVDfWCGVricgwCyw+cD CDbqF/tWKAn5E1cw1Ooh7EPjglTZC0I= Received: from mx-prod-mc-02.mail-002.prod.us-west-2.aws.redhat.com (ec2-54-186-198-63.us-west-2.compute.amazonaws.com [54.186.198.63]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-496-3IDcwIyKNOeoWZ9-SbmMhA-1; Tue, 08 Apr 2025 10:59:51 -0400 X-MC-Unique: 3IDcwIyKNOeoWZ9-SbmMhA-1 X-Mimecast-MFC-AGG-ID: 3IDcwIyKNOeoWZ9-SbmMhA_1744124390 Received: from mx-prod-int-08.mail-002.prod.us-west-2.aws.redhat.com (mx-prod-int-08.mail-002.prod.us-west-2.aws.redhat.com [10.30.177.111]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by mx-prod-mc-02.mail-002.prod.us-west-2.aws.redhat.com (Postfix) with ESMTPS id C761619560B0; Tue, 8 Apr 2025 14:59:49 +0000 (UTC) Received: from localhost (unknown [10.72.112.61]) by mx-prod-int-08.mail-002.prod.us-west-2.aws.redhat.com (Postfix) with ESMTPS id 15CBB18009BC; Tue, 8 Apr 2025 14:59:47 +0000 (UTC) Date: Tue, 8 Apr 2025 22:59:41 +0800 From: Baoquan He To: David Hildenbrand Cc: linux-mm@kvack.org, akpm@linux-foundation.org, osalvador@suse.de, mingo@kernel.org, yanjun.zhu@linux.dev, linux-kernel@vger.kernel.org Subject: Re: [PATCH v3 1/3] mm/gup: fix wrongly calculated returned value in fault_in_safe_writeable() Message-ID: References: <20250407030306.411977-1-bhe@redhat.com> <20250407030306.411977-2-bhe@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Scanned-By: MIMEDefang 3.4.1 on 10.30.177.111 X-Rspamd-Queue-Id: 64D3140002 X-Rspam-User: X-Rspamd-Server: rspam02 X-Stat-Signature: ch9jik7t5s9s4464pki776mxyapm3o74 X-HE-Tag: 1744124396-129757 X-HE-Meta: U2FsdGVkX18IAylqOudXhCJsYA7YdrwRf8NgAWcKWrdzRNCS/3SJHencgtImChL6fTpnSwLOwCSFYknVhXmP7FQYO1UQD4/bUkrHkUSe+6/Bu9uUzV7bLA58OXumHT8SWOn/LvtUG//ODPTyVMY6ZayiVoV9TKiZ32u1wgEjdiIk1nhFC0ZT86cfBfA2jY1002kbdvZ0rGF09yafltv8iLkCy+9z2aQuwKqYLRESiSa/9pORWQT+jDoTm0Pr0nXsO4FePydBxBNQYpx/nZOvr6MJWUcZLgOTvHKecd8g9NIMr832VPrT7EZ5i57GBEU/SV0HUYUgoT6yXzb0/KzfIEY9nyZNt2BsRb5wyWrfNVqGgwzrRJzectk4QW1fA9QCxK4PpcwCV9/D2M7NRFb/QZRies/N3y6yyXYIHdLUpZWygoHsn4jCJp5E7OrdWy96Dv/klDOOAaGpyLw33rKVvYEe+EkKtxN73fy0+igsfnzPXg3hYXPOwQ2MKW9f8G1wUznttWBzfdJZSdJqRhcuVHOn9QJLvyJSvJvmE8fVEnfW9TejhQSgghnwYJvJxV5987xrPLjAVUXH6Jlzrr7FyWIKoR/LYb6Yh5cuR8b4ZS69NSuGA+jNFQG+5agqxaNB5Aafez1Lp4wK9RH6Nc5Ohj2n49oXlcEalFvJH42glmef1xb2fOCHEFIRufLsSvz6fGz4RWHNllIzfPhDm0BSfCCLvldoegEaz+ZkpZ9u5L3Om9vNBpYSGcUPIlzxBeW7rYI57x6a5SEHRCwPNARwSvhaqfDAPb/JSiZRI5u7j3e7v+eAjuHaxInIL0ryEJdn/MMk4eIPg/RMDuaUDWL2z4bjC2VMYCravZOv/ajDVgxwc4DEM8Bak9VKOszl7wwOD/Rl2GL081twe5J5Ct/Z3XDEkArd5cX/mgfW3XHh9y8X9v0tzvsOrK60hafxjs4tUY5rsvam9XV0N0bdnmk 8g7QQJ4O GbcRzgtJinWxOyxfSuGWUOqByVMEqDh/mAKzEK4kqCVA3n+TYLIEEE9EUfmiq9oZNi/HpyMwOT8SBPVMZGtVZOTUP554HZplasHBXLcd7KcEZK+77rSkFgEZ8GvAhiorMSVmjpDG8HykOYnUPqw9JzWKYDQ== 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 04/08/25 at 12:00pm, David Hildenbrand wrote: > On 08.04.25 11:52, David Hildenbrand wrote: > > On 07.04.25 05:03, Baoquan He wrote: > > > Not like fault_in_readable() or fault_in_writeable(), in > > > fault_in_safe_writeable() local variable 'start' is increased page > > > by page to loop till the whole address range is handled. However, > > > it mistakenly calcalates the size of handled range with 'uaddr - start'. > > > > > > Here fix the code bug in fault_in_safe_writeable(), and also adjusting > > > the codes in fault_in_readable() and fault_in_writeable() to use local > > > variable 'start' to loop so that codes in these three functions are > > > consistent. > > > > > > > I probably phrased it poorly in my other reply: the confusing part (to > > me) is adjusting "start". Maybe we should have unsigned long start,end,cur; > > > > Maybe we should really split the "fix" from the cleanups, and tag the > > fix with a Fixes:. > > > > I was wondering if these functions could be simplified a bit. But the > > overflow handling is a bit nasty. > > FWIW, maybe the following could work and clarify things. Just a thought. The code simplification looks great to me. I will make a patch to only contains the code bug fixing with Fixes so that it's eaiser to back port to stable kernel, and make another patch as below to refactor codes in fault_in_readable/writable/safe_writable(). Thanks for suggestion. > > > diff --git a/mm/gup.c b/mm/gup.c > index 92351e2fa876b..7a3f78a209f8b 100644 > --- a/mm/gup.c > +++ b/mm/gup.c > @@ -2223,30 +2223,23 @@ EXPORT_SYMBOL(fault_in_safe_writeable); > */ > size_t fault_in_readable(const char __user *uaddr, size_t size) > { > - const char __user *start = uaddr, *end; > + const unsigned long start = (unsigned long)uaddr; > + const unsigned long end = start + size; > + unsigned long cur; > volatile char c; > if (unlikely(size == 0)) > return 0; > if (!user_read_access_begin(uaddr, size)) > return size; > - if (!PAGE_ALIGNED(uaddr)) { > - unsafe_get_user(c, uaddr, out); > - uaddr = (const char __user *)PAGE_ALIGN((unsigned long)uaddr); > - } > - end = (const char __user *)PAGE_ALIGN((unsigned long)start + size); > - if (unlikely(end < start)) > - end = NULL; > - while (uaddr != end) { > - unsafe_get_user(c, uaddr, out); > - uaddr += PAGE_SIZE; > - } > - > -out: > + /* Stop once we overflow to 0. */ > + for (cur = start; cur && cur < end; cur = PAGE_ALIGN_DOWN(cur + PAGE_SIZE)) > + unsafe_get_user(c, (const char __user *)cur, out); > user_read_access_end(); > (void)c; > - if (size > uaddr - start) > - return size - (uaddr - start); > +out: > + if (size > cur - start) > + return size - (cur - start); > return 0; > } > EXPORT_SYMBOL(fault_in_readable); > > > -- > Cheers, > > David / dhildenb >