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 B9A1DD3ABF3 for ; Mon, 11 Nov 2024 19:10:46 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id E725C6B0099; Mon, 11 Nov 2024 14:10:45 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id E22B16B009A; Mon, 11 Nov 2024 14:10:45 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id C9C976B009B; Mon, 11 Nov 2024 14:10:45 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0012.hostedemail.com [216.40.44.12]) by kanga.kvack.org (Postfix) with ESMTP id A746C6B0099 for ; Mon, 11 Nov 2024 14:10:45 -0500 (EST) Received: from smtpin24.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay07.hostedemail.com (Postfix) with ESMTP id 62B0C160B69 for ; Mon, 11 Nov 2024 19:10:45 +0000 (UTC) X-FDA: 82774754832.24.61B7FC8 Received: from mail-oo1-f53.google.com (mail-oo1-f53.google.com [209.85.161.53]) by imf03.hostedemail.com (Postfix) with ESMTP id C173720004 for ; Mon, 11 Nov 2024 19:10:24 +0000 (UTC) Authentication-Results: imf03.hostedemail.com; dkim=pass header.d=chromium.org header.s=google header.b="kJU/rN5L"; spf=pass (imf03.hostedemail.com: domain of jeffxu@chromium.org designates 209.85.161.53 as permitted sender) smtp.mailfrom=jeffxu@chromium.org; dmarc=pass (policy=none) header.from=chromium.org ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1731352189; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to: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=2oVunbPX+hQzjw8N4pwS8cQ26lQYQ2E2JKsydudMgZ0=; b=V2E3rfJ1PIIGoRY7QVfZVtjAZ2YXZAie8+uljIStHd94A3rm0OU/XkibQK01+DkGQxpzz1 Nb6c9kHEaIE5r7ollPZmAsteoog4Z7P4kLyGu7Hq3aYn91u4HzPE9mND+2HrOxoQgLNCkl E1mV+zK13Wk4Zkp/E6n37hRNmDgCXgo= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1731352189; a=rsa-sha256; cv=none; b=fuEdIa0RKT/81wX7n0QLDKiY6w1KN2UeFqr355tcgcJjOsa7cmJFMUoQQPSzFsvK/9SaLw tgnum3/2LNRQOOLkiheTDDW6MA44c1ubKQuN9oJHIEySk9jT0cBYfGi9hDHwjUyVeYh73O GFnIdPQO5PqHCc0yDPnl4fsQckLb4Ac= ARC-Authentication-Results: i=1; imf03.hostedemail.com; dkim=pass header.d=chromium.org header.s=google header.b="kJU/rN5L"; spf=pass (imf03.hostedemail.com: domain of jeffxu@chromium.org designates 209.85.161.53 as permitted sender) smtp.mailfrom=jeffxu@chromium.org; dmarc=pass (policy=none) header.from=chromium.org Received: by mail-oo1-f53.google.com with SMTP id 006d021491bc7-5ebeca94c87so85416eaf.2 for ; Mon, 11 Nov 2024 11:10:43 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; t=1731352242; x=1731957042; darn=kvack.org; h=content-transfer-encoding:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=2oVunbPX+hQzjw8N4pwS8cQ26lQYQ2E2JKsydudMgZ0=; b=kJU/rN5L0eNUuOKXonBhsIYpuJCdv7u9uWl8AroeOzhDOJcAPU2jkOULG26SFJNonF wp07/XfoTH5E8b718IYj4zmRaEFrILMYerU+OjBfPP5qKhltoSbNtLsg+1F3nH/Ly2Fy 421K1B+mP5xSfwCiZX5zG/fCPxruK1Dc2zUC4= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1731352242; x=1731957042; h=content-transfer-encoding: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=2oVunbPX+hQzjw8N4pwS8cQ26lQYQ2E2JKsydudMgZ0=; b=MKeSrfaGzfgskEtLPknBlEcJLp176h0XU4ylB/oaZmoSvX4nOibiogQrEaRtgibWyR HEBEoEiHt51dfsK09+Lq4QFapxLtO8qV2KdOPrVwUe1GBEcAEa+a13Yq1k6o43PTQB6F aB+nlNzwXwAMY02ezMjegXA5Di+M23WsVeEW2Q43S0Gozy9kfJAf5zAXiqROLLpsNEK7 7MYCnP8gsht2MPJCEDG8iv4SE8oMwyj0G+QAcDAkuwV6XMGm6CnBU3+cc353SWkeqzEo WX3DK7/aM/6H+JvDHRWhMi1fxrQCdNjIaO1uDTn5xrWwJcGlZCg+ighNg8b1J2qxOnqt tX/w== X-Forwarded-Encrypted: i=1; AJvYcCUPnjdfgUiBSDGl9t+cGSxZiROnHArKDDMi2KzVo4XPMpSkDmwPMJwrL0my0kc3p4pLN95oZWHU6g==@kvack.org X-Gm-Message-State: AOJu0YwqU8SpJ9lFZsm7caPL+k1aJbfUgSDD3atle1IDJfmHSqVpDdSa Vn3Gx6lmJDjRaqlNnwptDkKJ/J1C6aFYItJM5tK8pzOeh1OA5JBfbKQoNXbdDgOteauA61nWpm/ a8yatgcdIEtK5zKupjR9YuhqB0MCZTgkVLvfI X-Google-Smtp-Source: AGHT+IEpFmf11bck6bv6giVGNlwr01CEOxwij9HmVNKR3ArfGvUXwv1JnxM37AA0Rduivx4vGLTAuA8GztTXRwykqX0= X-Received: by 2002:a05:6870:fe87:b0:27b:73dd:2a85 with SMTP id 586e51a60fabf-295600cb480mr2544177fac.1.1731352242328; Mon, 11 Nov 2024 11:10:42 -0800 (PST) MIME-Version: 1.0 References: <20241014215022.68530-1-jeffxu@google.com> <20241014215022.68530-2-jeffxu@google.com> <6r5sxlhfujr2expiscsfpdjtraqlvy6k3cznmv25lo6usmyw7x@igmuywngc5xi> In-Reply-To: From: Jeff Xu Date: Mon, 11 Nov 2024 11:10:30 -0800 Message-ID: Subject: Re: [RFC PATCH v2 1/1] exec: seal system mappings To: "Liam R. Howlett" , Jeff Xu , akpm@linux-foundation.org, keescook@chromium.org, jannh@google.com, torvalds@linux-foundation.org, adhemerval.zanella@linaro.org, oleg@redhat.com, linux-kernel@vger.kernel.org, linux-hardening@vger.kernel.org, linux-mm@kvack.org, jorgelo@chromium.org, sroettger@google.com, ojeda@kernel.org, adobriyan@gmail.com, anna-maria@linutronix.de, mark.rutland@arm.com, linus.walleij@linaro.org, Jason@zx2c4.com, deller@gmx.de, rdunlap@infradead.org, davem@davemloft.net, hch@lst.de, peterx@redhat.com, hca@linux.ibm.com, f.fainelli@gmail.com, gerg@kernel.org, dave.hansen@linux.intel.com, mingo@kernel.org, ardb@kernel.org, mhocko@suse.com, 42.hyeyoo@gmail.com, peterz@infradead.org, ardb@google.com, enh@google.com, rientjes@google.com, groeck@chromium.org, lorenzo.stoakes@oracle.com Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Stat-Signature: gpwcycsjaefreduwda4c3kunpsueeman X-Rspam-User: X-Rspamd-Queue-Id: C173720004 X-Rspamd-Server: rspam02 X-HE-Tag: 1731352224-484827 X-HE-Meta: U2FsdGVkX19UQw2r0jyDO1Bpegi0grO4OKYCN0hs5y5KLk7foE0fNczSmtQrqOBRbTTZ93Et1j3I5lsEoC2dmjI998JnhuhJ7XL5GVo4rUB670UfG1KLzif7az+lkMS8mQnDTgirPiQaxigkRRnTAw6wRfL5jtDA8UWalbpkfUU7PU/TLwBQeJ7+/v5zyYjkC0IG2wFZRDrfp3qy8oSRT0PHCugSy5GFUKq5X1fag3+Gy8j/5oISSojmPWM9pZwdTxHywl3xL7zv7DS2HMg0S0GYebUOpKFX9uvsj3GbCwov+AboncIUujvYEj/HhudE8IuBU9s60b1Cl57h6zL5RM5wjvL9ACeGbQDuDkwS0OZ+ufjJMoofo0HSsLNv5B2fS8xxfGE83I0BGV6TC6GQOshZ5IsbEoRa7wsHW2tt2IYOyLHZMJ0kye+tJjKnU9zDtz/sBMQhZnh4Jc6jQ15Ab9BQSzDXF1vWlHpLitzan5CJTJlCT9NC1Diw6ohYKYQwwDnkmSGVdXf6kYcQfKxKoVm8/UKktDsikoztTtWYbrqzCt3ZsnZHV7ztVdVNFE4zU/1KgTSNUY5JbgEoTVLORKEiKlag8qxlrMdSj4E8dg0OZCh9YVP36qbdBlS6p/pnk7bs1SViF7nWW0IXRAE/SQamHdjd3klqp817gO8t/S/mglsxFwzO4E+q6lwWzzgtaoniiQPagsTz5JBmHoJbyPGYmfI9w1VmQffalfcoRa5qkQBFyxiuC8P7g7vvUp1SsT5nf9/k5aknym2VOghhfUTo9WB13/y3YTZpfqZTBNYudZuILE2mUpVPKvRlEUpN/Qi42uPhNKtLXqhG0adKWm5mifx+P1idy+/keGd5HLQ9gL561rskprw6Z00AftKd3XlUmLqLKqfD+DdoeUGgrL/wVutte0ZBAkx+mOLdgapv1ka/A9yTnmtM7JFI+bt3IvykyN396asoOMgG9BH HoIK4UKR eaHDfVw55vPjSkNPz8HA0wTTZC3gxr27X4xVE4SLm6dFIqzJENvDm20abd5TO/dOQZXdAgU2Z3GQy9fWkcnMgChsG6/esUzZsJUcrBRBZE60oKFQTspQkYWvVq20BekAjsOXFXFTD8gNe+TvJrsOkP0GvxIdLZDyYLkCf/ZBlec3cbVAmNK/zzleabNcwwaafhRVNm031IJzbkQrxrAlgM2e0KLCznC4+e01aUi5VOoDJreg+VTJqLrr3Ty1e7NP7uDAq6t4wgU3TAmgzSUwuLP5p4e0AFn71KMQgBqeHR5cyS/LTF/i5iqtJDwJZpow4lRqr20rn1/kbZYGwWKSV364gPi6KZWkn/SO+ 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: Hi Liam On Thu, Oct 17, 2024 at 9:01=E2=80=AFAM Liam R. Howlett wrote: > > > Does it make sense for this to live in exec? Couldn't you put it in = the > > > mm/mseal.c file? It's vma flags for mappings and you've put it in > > > fs/exec? > > > > > If you are referring to utilities related to kernel cmdline, they > > should be in this file. > > You created a wrapper for the command line, but then included the user > in this file as well. > > hugetlbfs reads the command line as well, in cmdline_parse_hugetlb_cma. > That parser lives with the rest of the hugetlb code in hugetlb.c > > I think this has to do with your view as this is an exec thing, where I > think it's an mm thing. My arguments are that you are directly adding > flags to vmas and it's dealing with mseal which has memory in the name > with the file living in the mm/ directory. If I wanted to know what's > using mseal, I'd start there and totally miss what you are adding here. > > Besides applying a vma flag to exec mappings, why do you feel like it > belongs in fs/ ? > The vdso/vvar/stack/heap alike are type of mappings belonging to processes, and are created during execve() syscall which is in fs/exec.c. mm/mseal.c provides core memory sealing functionality and exec.c uses it. IMO, it is better to keep the provider (mm/mseal.c) and consumer (executable) separate. To make modulization better, I can do below adjustment: if (seal_system_mapping_enabled()) <-- implemented by fs/exec.c add_vm_sealed() <- keep in include/linux/mm.h However, if you have a strong opinion on this, I could move the parsing logic to mm/mseal. > > > > +void update_seal_exec_system_mappings(unsigned long * > > > The name is also very long and a bit odd, it could be used for other > > > reasons, but you have _system_mappings on the end, and you use seal b= ut > > > it's mseal (or vm_seal)? Would mseal_flag() work? > > > > > It could be longer :-) > > it means update_sealing_flag_for_executable_system_mappings. > > mseal_flag is too short and not descriptive. > > mseal_exec_flags() ? > It needs to be more descriptive because there are also stacks and heaps to be sealed. I suggest to use below name to make it shorter: if (seal_system_mapping_enabled()) add_vm_sealed() > > > > diff --git a/mm/mmap.c b/mm/mmap.c > > > > index 57fd5ab2abe7..d4717e34a60d 100644 > > > > --- a/mm/mmap.c > > > > +++ b/mm/mmap.c > > > > @@ -2133,6 +2133,7 @@ struct vm_area_struct *_install_special_mappi= ng( > > > > unsigned long addr, unsigned long len, > > > > unsigned long vm_flags, const struct vm_special_mapping *spec= ) > > > > { > > > > + update_seal_exec_system_mappings(&vm_flags); > > > > return __install_special_mapping(mm, addr, len, vm_flags, (vo= id *)spec, > > > > &special_mapping_vmops); > > > > > > If you were to return a flag, you could change the vm_flags argument = to > > > vm_flags | mseal_flag() > > > > > passing pointer seems to be the most efficient way. > > I disagree. Here is the godbolt.org output for gcc x86-64 14.2 of your > code (with some added #defines to make it compile) > > seal_system_mappings: > .long 1 > seal_system_mappings_enabled: > push rbp > mov rbp, rsp > mov eax, DWORD PTR seal_system_mappings[rip] > cmp eax, 1 > jne .L2 > mov eax, 1 > jmp .L3 > .L2: > mov eax, 0 > .L3: > pop rbp > ret > update_seal_exec_system_mappings: > push rbp > mov rbp, rsp > sub rsp, 8 > mov QWORD PTR [rbp-8], rdi > mov rax, QWORD PTR [rbp-8] > mov rax, QWORD PTR [rax] > and eax, 2 > test rax, rax > jne .L6 > call seal_system_mappings_enabled > test al, al > je .L6 > mov rax, QWORD PTR [rbp-8] > mov rax, QWORD PTR [rax] > or rax, 2 > mov rdx, rax > mov rax, QWORD PTR [rbp-8] > mov QWORD PTR [rax], rdx > .L6: > nop > leave > ret > main: > push rbp > mov rbp, rsp > sub rsp, 16 > mov QWORD PTR [rbp-8], 0 > lea rax, [rbp-8] > mov rdi, rax > call update_seal_exec_system_mappings > mov rax, QWORD PTR [rbp-8] > leave > ret > > ----- 48 lines ----- > Here is what I am suggesting to do with replacing the passing of a > pointer with a concise "vm_flags | mseal_exec_flags()" (with the same > added #defines to make it compile) > > seal_system_mappings: > .long 1 > mseal_exec_flags: > push rbp > mov rbp, rsp > mov eax, DWORD PTR seal_system_mappings[rip] > cmp eax, 1 > jne .L2 > mov eax, 2 > jmp .L3 > .L2: > mov eax, 0 > .L3: > pop rbp > ret > main: > push rbp > mov rbp, rsp > sub rsp, 16 > mov QWORD PTR [rbp-8], 0 > call mseal_exec_flags > mov edx, eax > mov rax, QWORD PTR [rbp-8] > or eax, edx > leave > ret > > ----- 26 lines ----- > > So as you can see, there are less instructions in my version; there are > 47.92% less lines of assembly. > vm_flags already run out of space in 32 bit, sooner or later we will need to change that to *** a struct ***, passing address will be becoming necessary with struct. Since this is not a performance sensitive code path, 3 or 4 times during execve(), I think it would be good to start here. -Jeff