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 55402C63797 for ; Tue, 17 Jan 2023 07:09:19 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 966466B0071; Tue, 17 Jan 2023 02:09:18 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 8EF086B0073; Tue, 17 Jan 2023 02:09:18 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 78F656B0074; Tue, 17 Jan 2023 02:09:18 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0017.hostedemail.com [216.40.44.17]) by kanga.kvack.org (Postfix) with ESMTP id 653786B0071 for ; Tue, 17 Jan 2023 02:09:18 -0500 (EST) Received: from smtpin01.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay10.hostedemail.com (Postfix) with ESMTP id 3D0F8C0733 for ; Tue, 17 Jan 2023 07:09:18 +0000 (UTC) X-FDA: 80363414796.01.4BE76C6 Received: from szxga01-in.huawei.com (szxga01-in.huawei.com [45.249.212.187]) by imf30.hostedemail.com (Postfix) with ESMTP id 77BBC8000E for ; Tue, 17 Jan 2023 07:09:15 +0000 (UTC) Authentication-Results: imf30.hostedemail.com; dkim=none; dmarc=pass (policy=quarantine) header.from=huawei.com; spf=pass (imf30.hostedemail.com: domain of mawupeng1@huawei.com designates 45.249.212.187 as permitted sender) smtp.mailfrom=mawupeng1@huawei.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1673939356; 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; bh=/9joiqab3OsHl2YYKFhUBw9aNpMqk7oWfgwBvvceBVo=; b=kI/HF9BJHdewju+PumwQ3zsVphQY8dPpGUXbP20MVhtpdy4s3FymoczEKHiOGJKza597rM 1B4a5bGi0HwJir5zN59/Wd/iTLbq20biXVh0qkfyTWOMHgLTkEpnJ8Rm91IsW7sJ09YZ5E lLpYO5fwgwR7m2qVBNycRoCx1YQfvbc= ARC-Authentication-Results: i=1; imf30.hostedemail.com; dkim=none; dmarc=pass (policy=quarantine) header.from=huawei.com; spf=pass (imf30.hostedemail.com: domain of mawupeng1@huawei.com designates 45.249.212.187 as permitted sender) smtp.mailfrom=mawupeng1@huawei.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1673939356; a=rsa-sha256; cv=none; b=IJsrXivGdG7uh0iyFZ2nlLx+sabicG44E3ZDN/A4ZR71y1Iy8prQdMIi7oDIhzhF5obdJ4 4yrTqDCv4E8xGiILHUmsSkN+7lVD+h2Nq5Oe4EddboPKy/fORfyDdiQqp+usPanJSPV4Q6 Q8efKU0QGRYF+EfbRPv5oevsIuNkAGc= Received: from dggpemm500014.china.huawei.com (unknown [172.30.72.56]) by szxga01-in.huawei.com (SkyGuard) with ESMTP id 4Nx0ML0N1sznVfn; Tue, 17 Jan 2023 15:07:26 +0800 (CST) Received: from [10.174.178.120] (10.174.178.120) by dggpemm500014.china.huawei.com (7.185.36.153) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2375.34; Tue, 17 Jan 2023 15:09:08 +0800 Message-ID: <5c83fae3-8eae-6303-4656-8da40a2c12b1@huawei.com> Date: Tue, 17 Jan 2023 15:09:08 +0800 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101 Thunderbird/102.6.1 From: mawupeng Subject: Re: [PATCH v2 1/4] mm/mlock: return EINVAL if len overflows for mlock/munlock To: CC: , , , , References: <20230116115813.2956935-1-mawupeng1@huawei.com> <20230116115813.2956935-2-mawupeng1@huawei.com> <20230116125126.ed715ddf00ff4ffa2952ca29@linux-foundation.org> Content-Language: en-US In-Reply-To: <20230116125126.ed715ddf00ff4ffa2952ca29@linux-foundation.org> Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 7bit X-Originating-IP: [10.174.178.120] X-ClientProxiedBy: dggems704-chm.china.huawei.com (10.3.19.181) To dggpemm500014.china.huawei.com (7.185.36.153) X-CFilter-Loop: Reflected X-Rspamd-Queue-Id: 77BBC8000E X-Stat-Signature: fdu4izodo1amyjnobopizzspih8rgm7t X-Rspam-User: X-Rspamd-Server: rspam08 X-HE-Tag: 1673939355-473635 X-HE-Meta: U2FsdGVkX1+xFnBXhlXpiHfvGUI7edlitGfv5O9thF3deAd4hskiKP7P7ypnx2wOo/ZCr9mUZaOialZh5cgi3aYv6BDitK1JFVqTTkjY4h7n5TnNqdzoGH2KF1xehSTjapxnJ4yK5IDnxe6J2CG+8FY79aHbLUu+zkCOnfQUc9VLsvVwrYe/bqnU42Gq4HZF4FDb/TaMcsodI5II5NYgq+fczAIrG/kccud5InLGI12mwP3hn6SXVjkk1le5G89oELG7LeAqDtXtwBmwFq0JD+GE9DpWoJGcxSrF+vQUJvr4Be06sgVzd1hHaJVisu9zwpn2TZ9JUuZliHOA4wGGNgC0znsyakNQdYAxIEyo0xAKjTCap4jsAJEKoDzdH5yMlU4JqSh8w1K40I6MOujZN5tb+CTW4nW+CSjO2cKC6xt805EJBakTE+koCTgSM66VY/rhIIcp+3PNhIj81xPWDExCcEntb4HT4OfNAKrZ8+LIN9u+lurGAKVNjtQXbOeUYOjL0swDX47hjbLG/Eif9gf/oze5/KYPwmk+T0DcjBmwCn9Im/AM1F9W5IVUuz3Nh3VsqpasLEeUlumfq6i1jKUMWiHoS2QFY82dCLOSFOX3KJW1Y1jZ4YoLdWq4J58qBcF5V5q4H6nvM1f7miGmXxck1liSpho7eH5cV5HbYT9VB9b8LKmmI9F+R2ktf89ERY2E/qfylEm+q86Pk0rwTuBHQa1cMXk2uSsn1m7kw3tN6SZg4yUp1Yy5CpWk2BRTRsBGHpjpufYeVv4L8LOLW+SIzRPyzmxhWAnrB2qDW/dliZpPLJDpMDjO+k5QKMEszniXn3Wf0YM= 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: On 2023/1/17 4:51, Andrew Morton wrote: > On Mon, 16 Jan 2023 19:58:10 +0800 Wupeng Ma wrote: > >> From: Ma Wupeng >> >> While testing mlock, we have a problem if the len of mlock is ULONG_MAX. >> The return value of mlock is zero. But nothing will be locked since the >> len in do_mlock overflows to zero due to the following code in mlock: >> >> len = PAGE_ALIGN(len + (offset_in_page(start))); >> >> The same problem happens in munlock. >> >> Add new check and return -EINVAL to fix this overflowing scenarios since >> they are absolutely wrong. >> >> ... >> >> --- a/mm/mlock.c >> +++ b/mm/mlock.c >> @@ -569,6 +569,7 @@ static __must_check int do_mlock(unsigned long start, size_t len, vm_flags_t fla >> unsigned long locked; >> unsigned long lock_limit; >> int error = -ENOMEM; >> + size_t old_len = len; > > I'm not sure that "old_len" is a good identifier. It reads to me like > "the length of the old mlocked region" or something. > > I really don't like it when functions modify the values of the incoming > argument like this. It would be better to leave `len' alone and create > a new_len or something. Thanks for your reviewing. You do have a point in saying that. > >> start = untagged_addr(start); >> >> @@ -578,6 +579,9 @@ static __must_check int do_mlock(unsigned long start, size_t len, vm_flags_t fla >> len = PAGE_ALIGN(len + (offset_in_page(start))); >> start &= PAGE_MASK; >> >> + if (old_len != 0 && len == 0) >> + return -EINVAL; > > It would be clearer to do this immediately after calculating the new > value of `len'. Before going on to play with `start'. > > Can we do something like this? > > --- a/mm/mlock.c~a > +++ a/mm/mlock.c > @@ -575,7 +575,12 @@ static __must_check int do_mlock(unsigne > if (!can_do_mlock()) > return -EPERM; > > - len = PAGE_ALIGN(len + (offset_in_page(start))); > + if (len) { > + len = PAGE_ALIGN(len + (offset_in_page(start))); > + if (len == 0) /* overflow */ > + return -EINVAL; > + } > + > start &= PAGE_MASK; > > lock_limit = rlimit(RLIMIT_MEMLOCK); > _ It's really more appropriate to check like this, I will use this in the next patchset. > > That depends on how we handle len==0. afaict, mlock(len==0) will > presently burn a bunch of cpu cycles (not that we want to optimize this > case), do nothing then return 0? We can add and a new check in if len == 0, since the similar check appears in mbind, set_mempolicy_home_node, msync. The origin len == 0 check for mlock/munlock can be found in apply_vma_lock_flags, We can move this check to here to avoid burn a bunch of cpu cycles. do_mlock apply_vma_lock_flags end = start + len; if (end == start) return 0; Can we do something like this? diff --git a/mm/mlock.c b/mm/mlock.c index 7032f6dd0ce1..50a33abc1a2e 100644 --- a/mm/mlock.c +++ b/mm/mlock.c @@ -478,8 +478,6 @@ static int apply_vma_lock_flags(unsigned long start, size_t len, end = start + len; if (end < start) return -EINVAL; - if (end == start) - return 0; vma = mas_walk(&mas); if (!vma) return -ENOMEM; @@ -575,7 +573,12 @@ static __must_check int do_mlock(unsigned long start, size_t len, vm_flags_t fla if (!can_do_mlock()) return -EPERM; + if (!len) + return 0; len = PAGE_ALIGN(len + (offset_in_page(start))); + if (len == 0) + return -EINVAL; + start &= PAGE_MASK; lock_limit = rlimit(RLIMIT_MEMLOCK); @@ -632,10 +635,14 @@ SYSCALL_DEFINE3(mlock2, unsigned long, start, size_t, len, int, flags) SYSCALL_DEFINE2(munlock, unsigned long, start, size_t, len) { int ret; - start = untagged_addr(start); + if (!len) + return 0; len = PAGE_ALIGN(len + (offset_in_page(start))); + if (len == 0) + return -EINVAL; + start &= PAGE_MASK; if (mmap_write_lock_killable(current->mm)) >