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 77F44CD128A for ; Wed, 10 Apr 2024 01:30:22 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id CD2456B007B; Tue, 9 Apr 2024 21:30:21 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id C81DC6B0082; Tue, 9 Apr 2024 21:30:21 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id B70BB6B0083; Tue, 9 Apr 2024 21:30:21 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0010.hostedemail.com [216.40.44.10]) by kanga.kvack.org (Postfix) with ESMTP id 9A6A76B007B for ; Tue, 9 Apr 2024 21:30:21 -0400 (EDT) Received: from smtpin27.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay07.hostedemail.com (Postfix) with ESMTP id 36D451605E2 for ; Wed, 10 Apr 2024 01:30:21 +0000 (UTC) X-FDA: 81991891842.27.430EAEE Received: from szxga01-in.huawei.com (szxga01-in.huawei.com [45.249.212.187]) by imf30.hostedemail.com (Postfix) with ESMTP id B4F7680010 for ; Wed, 10 Apr 2024 01:30:17 +0000 (UTC) Authentication-Results: imf30.hostedemail.com; dkim=none; dmarc=pass (policy=quarantine) header.from=huawei.com; spf=pass (imf30.hostedemail.com: domain of wangkefeng.wang@huawei.com designates 45.249.212.187 as permitted sender) smtp.mailfrom=wangkefeng.wang@huawei.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1712712619; 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=oDIaKoiWzEdWF1QxmqwW6eLhA4dfHL8vie7rpW8qrNQ=; b=LVWypVi6fkL7duvqQhV/BZwNUjL4UcU6HUNQ9M8q6bKznUpI6C453ZDeYQkara4J48F2dT jUFpz0cldsGKAMGKXmMKBC3QaaoDmZyooDA/pVtcu95AtQiIR53JfwLmd9uQm08+qDApy/ sRQjuEwqwAeg/IMPASK2xzf/7aeBNqI= 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 wangkefeng.wang@huawei.com designates 45.249.212.187 as permitted sender) smtp.mailfrom=wangkefeng.wang@huawei.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1712712619; a=rsa-sha256; cv=none; b=EWMXkptY1Php+8KLWQxPawKb6CkN+rq4q0PY2b9NqPov9pVug2Oua7+wSwD9dlj9sAe1Kt Cko9dOG0rLYPfVize1yJzk7BiyHupnRwOYvENG4iNnD8TDoemzAFCSTP22gM4NpMh09K/l sNJ8rcIixq8Ng3C4O3FNmNkbBjbtmSk= Received: from mail.maildlp.com (unknown [172.19.88.194]) by szxga01-in.huawei.com (SkyGuard) with ESMTP id 4VDlYf2c6wzwRSj; Wed, 10 Apr 2024 09:27:18 +0800 (CST) Received: from dggpemm100001.china.huawei.com (unknown [7.185.36.93]) by mail.maildlp.com (Postfix) with ESMTPS id 6C6621403D3; Wed, 10 Apr 2024 09:30:13 +0800 (CST) Received: from [10.174.177.243] (10.174.177.243) by dggpemm100001.china.huawei.com (7.185.36.93) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.1.2507.35; Wed, 10 Apr 2024 09:30:13 +0800 Message-ID: Date: Wed, 10 Apr 2024 09:30:12 +0800 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH 1/2] arm64: mm: drop VM_FAULT_BADMAP/VM_FAULT_BADACCESS Content-Language: en-US To: Catalin Marinas CC: Andrew Morton , Russell King , Will Deacon , , References: <20240407081211.2292362-1-wangkefeng.wang@huawei.com> <20240407081211.2292362-2-wangkefeng.wang@huawei.com> From: Kefeng Wang In-Reply-To: Content-Type: text/plain; charset="UTF-8"; format=flowed Content-Transfer-Encoding: 7bit X-Originating-IP: [10.174.177.243] X-ClientProxiedBy: dggems702-chm.china.huawei.com (10.3.19.179) To dggpemm100001.china.huawei.com (7.185.36.93) X-Rspamd-Queue-Id: B4F7680010 X-Rspam-User: X-Rspamd-Server: rspam02 X-Stat-Signature: 6p6gnpbh9np7gkd5tzsgy6mj8t16f5ti X-HE-Tag: 1712712617-802683 X-HE-Meta: U2FsdGVkX18zAC3ZEiW+R14auzyS2eHZ2xOT3GZO6oKzCQeJR04+NomiAjpPV5hocWdqKWwWh+j7MR4uE855LIJMBpA1/tey5yctNswvXBLVurN05VUT5LzpwD3jlIoivpdkXmt8umWXGD7SEd4OKou0zI53G2nZJEKok+3ToWDcIPIx8ai4FHlZRDdw225oKdZotq26Y5qIXmBHIAlNptVKwGGo/r6uGIwgMevYqBDL9CSmWbDM0wthyPDHfRzpwTWOZuJKfo23ANUHXALKhC+jD9Zx57kliG28Q/Q9dI4b25FzPZzO7W4l2r+bfZYsOxjO3T6zB95iWhcbPwP07H46Rf8XKkftEd1hoUUfqqjmnr+/+Z00YKrP2FNQn04xbEUa94INaWcSFGhjx/7kfbC4XFyyl4CvOTyTiwLi33ihrG89TbH9hqrq9cZ0sl/0hYe3Qi6g3bDGE1VKC1l0sdIIymAIasu6/IMH+Da4Bb6jjEq9bbFg+4BED1ZMiRw5qdk3JEOmvoNJ36SEf7KdNBgYNlgUTn8OfEi2pWQ+vxzj6w2t6YSxhMoKV+97FKS0kM2TFt4POWxZ2xJW7BsqADSKAv6b1M9bfLLlJOd+djFArXzJrXYbJoaYnTMv8BdGGx0Zjj0hZ+Z/BKCB0gDgHJq92GAvmiypOCrApNMT4UPsXlN/OO9F4aLjzbOy6SVb+vE6SVLMpGqBt5qV4pU9z5haXAnPfWcTgl57HdUIZcCOarByP0NFxgxB/yWArcauV54ytrEWki+YVYcBK50Sm2JR9AtJQG8TJnKKu8VtdDWCIHzSarpy9I3Qs+0wrVVbRcLiqwZEfIgwAXfgX1YuTVOkYQPiJBVdxG7hkn8PS+n9Swgvn3/5gv1dsBD/jfkAaYyhmcZK6zOby724qL91ICPbtEBZo9KyesW1rflMZeHn+fWx2KdpkMzrZfIWUjOVQCbiXyEPvujIhDPYus4 a+6EvCMs nvB3DvhqcPKDnl6lXrTki6H5mBuo7XPRCYNRFFhmphCV8b2nFSk+vK0IGRr6j01MW5G0srVGleHtawpkPw7xSFWJIXz3m5RU7vuiC2eulIyM7KsFjz7FZ6OfAw9/iDjnwr1afeG1G9ftn0Z7cjnS+UFGQTh9WgClgZMajRkXb253fabevXnsS8ELwNCoHbYauOihwRhurx6trGI3Q6qDoSqrpSA== 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 2024/4/9 22:28, Catalin Marinas wrote: > Hi Kefeng, > > On Sun, Apr 07, 2024 at 04:12:10PM +0800, Kefeng Wang wrote: >> diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c >> index 405f9aa831bd..61a2acae0dca 100644 >> --- a/arch/arm64/mm/fault.c >> +++ b/arch/arm64/mm/fault.c >> @@ -500,9 +500,6 @@ static bool is_write_abort(unsigned long esr) >> return (esr & ESR_ELx_WNR) && !(esr & ESR_ELx_CM); >> } >> >> -#define VM_FAULT_BADMAP ((__force vm_fault_t)0x010000) >> -#define VM_FAULT_BADACCESS ((__force vm_fault_t)0x020000) >> - >> static int __kprobes do_page_fault(unsigned long far, unsigned long esr, >> struct pt_regs *regs) >> { >> @@ -513,6 +510,7 @@ static int __kprobes do_page_fault(unsigned long far, unsigned long esr, >> unsigned int mm_flags = FAULT_FLAG_DEFAULT; >> unsigned long addr = untagged_addr(far); >> struct vm_area_struct *vma; >> + int si_code; > > I think we should initialise this to 0. Currently all paths seem to set > si_code to something meaningful but I'm not sure the last 'else' close > in this patch is guaranteed to always cover exactly those earlier code > paths updating si_code. I'm not talking about the 'goto bad_area' paths > since they set 'fault' to 0 but the fall through after the second (under > the mm lock) handle_mm_fault(). Recheck it, without this patch, the second handle_mm_fault() never return VM_FAULT_BADACCESS, but could return VM_FAULT_SIGSEGV(maybe other), which not handled in the other error path, handle_mm_fault ret = sanitize_fault_flags(vma, &flags); if (!arch_vma_access_permitted()) ret = VM_FAULT_SIGSEGV; so the orignal logical will set si_code to SEGV_MAPERR fault == VM_FAULT_BADACCESS ? SEGV_ACCERR : SEGV_MAPERR, therefore, i think we should set the default si_code to SEGV_MAPERR. > >> if (kprobe_page_fault(regs, esr)) >> return 0; >> @@ -572,9 +570,10 @@ static int __kprobes do_page_fault(unsigned long far, unsigned long esr, >> >> if (!(vma->vm_flags & vm_flags)) { >> vma_end_read(vma); >> - fault = VM_FAULT_BADACCESS; >> + fault = 0; >> + si_code = SEGV_ACCERR; >> count_vm_vma_lock_event(VMA_LOCK_SUCCESS); >> - goto done; >> + goto bad_area; >> } >> fault = handle_mm_fault(vma, addr, mm_flags | FAULT_FLAG_VMA_LOCK, regs); >> if (!(fault & (VM_FAULT_RETRY | VM_FAULT_COMPLETED))) >> @@ -599,15 +598,18 @@ static int __kprobes do_page_fault(unsigned long far, unsigned long esr, >> retry: >> vma = lock_mm_and_find_vma(mm, addr, regs); >> if (unlikely(!vma)) { >> - fault = VM_FAULT_BADMAP; >> - goto done; >> + fault = 0; >> + si_code = SEGV_MAPERR; >> + goto bad_area; >> } >> >> - if (!(vma->vm_flags & vm_flags)) >> - fault = VM_FAULT_BADACCESS; >> - else >> - fault = handle_mm_fault(vma, addr, mm_flags, regs); >> + if (!(vma->vm_flags & vm_flags)) { >> + fault = 0; >> + si_code = SEGV_ACCERR; >> + goto bad_area; >> + } > > What's releasing the mm lock here? Prior to this change, it is falling > through to mmap_read_unlock() below or handle_mm_fault() was releasing > the lock (VM_FAULT_RETRY, VM_FAULT_COMPLETED). Indeed, will fix, > >> >> + fault = handle_mm_fault(vma, addr, mm_flags, regs); >> /* Quick path to respond to signals */ >> if (fault_signal_pending(fault, regs)) { >> if (!user_mode(regs)) >> @@ -626,13 +628,11 @@ static int __kprobes do_page_fault(unsigned long far, unsigned long esr, >> mmap_read_unlock(mm); >> >> done: >> - /* >> - * Handle the "normal" (no error) case first. >> - */ >> - if (likely(!(fault & (VM_FAULT_ERROR | VM_FAULT_BADMAP | >> - VM_FAULT_BADACCESS)))) >> + /* Handle the "normal" (no error) case first. */ >> + if (likely(!(fault & VM_FAULT_ERROR))) >> return 0; >> >> +bad_area: >> /* >> * If we are in kernel mode at this point, we have no context to >> * handle this fault with. >> @@ -667,13 +667,8 @@ static int __kprobes do_page_fault(unsigned long far, unsigned long esr, >> >> arm64_force_sig_mceerr(BUS_MCEERR_AR, far, lsb, inf->name); >> } else { >> - /* >> - * Something tried to access memory that isn't in our memory >> - * map. >> - */ >> - arm64_force_sig_fault(SIGSEGV, >> - fault == VM_FAULT_BADACCESS ? SEGV_ACCERR : SEGV_MAPERR, >> - far, inf->name); >> + /* Something tried to access memory that out of memory map */ >> + arm64_force_sig_fault(SIGSEGV, si_code, far, inf->name); >> } > > We can get to the 'else' close after the second handle_mm_fault(). Do we > guarantee that 'fault == 0' in this last block? If not, maybe a warning > and some safe initialisation for 'si_code' to avoid leaking stack data. As analyzed above, it is sufficient that make si_code to SEGV_MAPPER by default, right? Thanks. >