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 X-Spam-Level: X-Spam-Status: No, score=-5.2 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,USER_AGENT_SANE_1 autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id B5513C2D0E5 for ; Thu, 26 Mar 2020 22:13:12 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 643722073E for ; Thu, 26 Mar 2020 22:13:12 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 643722073E Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=suse.cz Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id F30D76B000A; Thu, 26 Mar 2020 18:13:11 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id EBABA6B000C; Thu, 26 Mar 2020 18:13:11 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id D83CA6B000E; Thu, 26 Mar 2020 18:13:11 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0151.hostedemail.com [216.40.44.151]) by kanga.kvack.org (Postfix) with ESMTP id B9D256B000A for ; Thu, 26 Mar 2020 18:13:11 -0400 (EDT) Received: from smtpin04.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay01.hostedemail.com (Postfix) with ESMTP id 83EFD180ACF6C for ; Thu, 26 Mar 2020 22:13:11 +0000 (UTC) X-FDA: 76638914982.04.night82_73173006e5357 X-HE-Tag: night82_73173006e5357 X-Filterd-Recvd-Size: 12027 Received: from mx2.suse.de (mx2.suse.de [195.135.220.15]) by imf15.hostedemail.com (Postfix) with ESMTP for ; Thu, 26 Mar 2020 22:13:10 +0000 (UTC) X-Virus-Scanned: by amavisd-new at test-mx.suse.de Received: from relay2.suse.de (unknown [195.135.220.254]) by mx2.suse.de (Postfix) with ESMTP id 9B0E5ABBD; Thu, 26 Mar 2020 22:13:08 +0000 (UTC) Subject: Re: [RFC v3 1/2] kernel/sysctl: support setting sysctl parameters from kernel command line To: Kees Cook Cc: Luis Chamberlain , Iurii Zaikin , linux-kernel@vger.kernel.org, linux-api@vger.kernel.org, linux-mm@kvack.org, Ivan Teterevkov , Michal Hocko , David Rientjes , Matthew Wilcox , "Eric W . Biederman" , "Guilherme G . Piccoli" References: <20200326181606.7027-1-vbabka@suse.cz> <202003261256.950F1E5@keescook> From: Vlastimil Babka Autocrypt: addr=vbabka@suse.cz; prefer-encrypt=mutual; keydata= mQINBFZdmxYBEADsw/SiUSjB0dM+vSh95UkgcHjzEVBlby/Fg+g42O7LAEkCYXi/vvq31JTB KxRWDHX0R2tgpFDXHnzZcQywawu8eSq0LxzxFNYMvtB7sV1pxYwej2qx9B75qW2plBs+7+YB 87tMFA+u+L4Z5xAzIimfLD5EKC56kJ1CsXlM8S/LHcmdD9Ctkn3trYDNnat0eoAcfPIP2OZ+ 9oe9IF/R28zmh0ifLXyJQQz5ofdj4bPf8ecEW0rhcqHfTD8k4yK0xxt3xW+6Exqp9n9bydiy tcSAw/TahjW6yrA+6JhSBv1v2tIm+itQc073zjSX8OFL51qQVzRFr7H2UQG33lw2QrvHRXqD Ot7ViKam7v0Ho9wEWiQOOZlHItOOXFphWb2yq3nzrKe45oWoSgkxKb97MVsQ+q2SYjJRBBH4 8qKhphADYxkIP6yut/eaj9ImvRUZZRi0DTc8xfnvHGTjKbJzC2xpFcY0DQbZzuwsIZ8OPJCc LM4S7mT25NE5kUTG/TKQCk922vRdGVMoLA7dIQrgXnRXtyT61sg8PG4wcfOnuWf8577aXP1x 6mzw3/jh3F+oSBHb/GcLC7mvWreJifUL2gEdssGfXhGWBo6zLS3qhgtwjay0Jl+kza1lo+Cv BB2T79D4WGdDuVa4eOrQ02TxqGN7G0Biz5ZLRSFzQSQwLn8fbwARAQABtCBWbGFzdGltaWwg QmFia2EgPHZiYWJrYUBzdXNlLmN6PokCVAQTAQoAPgIbAwULCQgHAwUVCgkICwUWAgMBAAIe AQIXgBYhBKlA1DSZLC6OmRA9UCJPp+fMgqZkBQJeWsBDBQkLv4wmAAoJECJPp+fMgqZkgXgQ ALtf9fOTVgBzszJu+9swQ9PYMCUHUDhp2Iz3ZqiQPk911qoA+imeqlFMFFc3fxehMiv45/QM MD1t/qND8NIl/+ldjR8AMebCLf5v6g16D/8/RbvJV651cPxANiOwSkmuAJqfshxkijZ9aM2r iUeyoic4FHNSwgEvbkx8mrIRksbKwubDWUVsnayh4X5Xw+OxxNCXuWl0WfrVm16Izj0tuQ+2 0JkYzDWw1CX3oGgqgwboeOk8UcAVVbFLklCYn87+PoiX81ZcLFeRKjd8yz+Lc8uCjfHRSlaF nSt0dwijfPxRp8VsHTO3M0DfUaXmTSPZE+0JR57v0b2Ydl8YibHUzNJ1d42jZR1R3GDu6Knl +myBsEQ8AQ9dcjWO/JJLHfGLAZiJ2PFqJvnBLXsrpDChMTTorUsbv2cfBZgyjW62VOJEH9zj S+KaRop+INcBDdvoLCX7AbatAnuS41vIiFz9eVmJN/aYeWdXsHjihgtHySKx6eg52htXQixI 9e41hqfGvq+zblJi39NxIvVg2tw0v4VV5UpqD0zB2IFOYkzWjZRuhwfIeNku0I843lsuVd6M AAyxJtILK/K8VDOp72cU5vhxbIzFUk6yCnuuIMBCJB/OL2GRUclkhPz+28J8LMraq3WBHdy1 BJt8HMfyb9FIORT8jYG8MqKpT+XkVUSpqbHQuQENBFsZNTUBCACfQfpSsWJZyi+SHoRdVyX5 J6rI7okc4+b571a7RXD5UhS9dlVRVVAtrU9ANSLqPTQKGVxHrqD39XSw8hxK61pw8p90pg4G /N3iuWEvyt+t0SxDDkClnGsDyRhlUyEWYFEoBrrCizbmahOUwqkJbNMfzj5Y7n7OIJOxNRkB IBOjPdF26dMP69BwePQao1M8Acrrex9sAHYjQGyVmReRjVEtv9iG4DoTsnIR3amKVk6si4Ea X/mrapJqSCcBUVYUFH8M7bsm4CSxier5ofy8jTEa/CfvkqpKThTMCQPNZKY7hke5qEq1CBk2 wxhX48ZrJEFf1v3NuV3OimgsF2odzieNABEBAAGJAjwEGAEKACYCGwwWIQSpQNQ0mSwujpkQ PVAiT6fnzIKmZAUCXlrGNQUJBwP4AAAKCRAiT6fnzIKmZBS8D/9RfpA5gqj68RNpQiI8Bf82 KNVyG2S5DCL7UblqjnVZRLB7gZiXs484IZ628E20iBjx7yMFcH9hxjPJ+xPw1yRqubaqGCYm yUaHRauwGL4GKCgcdfIonn888cbEdNDslhp3yCEv4350h8ARD03e3ySRmXr9Onm8aL/+mzug Rd2UVDrQN9VYyAyJYiSn0Dt4JTNfXpPDrReBRld11X9A+aEajHYmowP3iJFji3msbNXPpsp4 sRIvNoa0JmGy+/Wl/uJDF3NNUYCFGL3famC7/mDGZX4p9b40Qbg31KLbQqCQ5h7uR0yabYE5 lQRV4r0SOcBX0mNVo/JtDKusfKndVS7o6KbtBCuKtBXQVTOI1DAIJn1FpPndgsyDHuqnNoed U1jqctKR97PLwPwT3kWjg4gt12YQkvvaj6e9itzg5I/9SgeuYo6AtHc/X7ReNZFL04YxpNL6 Sj9A3NrvSdmTxgtgXr7tnwXQRS8/DyHd+g+Bjcbl92xTZygJl/gxBy2N+5sqyl6V4oqvF2g3 aA8X5VBZv48X0lPLLf6C0q0YrzDsWBQeHNE26EA8Eaz1VfGla71qGMn7NekJzwlMb7C+TYKw UmyyDtMVmhPY57PCMtFcosy8HBZDAJ6mKR1WwpOdpVbmwW/BcfvMt2sj2ceINTSEpbHiJdBA 84qEcUTqS3rfKrkBDQReWsIzAQgAwX4mVSPXh8Cvkqg7faiv9qhpyMulBhVM1PXi+zOptSLI LU7dpTSaOXIY+kG5MXuc1X6uigv0+6DxIzuffvrR8K+//tMa1RWTItlLe6bd6wG60J1Q6tj2 7RTDjo3K1nDHFpmcR9hS3VQpeFFTtGk6RnESnlzpF3/FY7d9/6dEsochH0QGHBJUXXnMibPS zYxUJQNZzJg0HZKItczKfCo3jnhkDkdyqlqDEWLeu2B/24FBEK3bk30xRkxfLaCEHULhfOtt USmml989EHA6IXtk5rgUYeE6tTmp2XVNCQ0KjgV0eCsK70T0ZHIgiYyytOS+TaZBif1R9JaZ KmFqeTk1zQARAQABiQNyBBgBCgAmAhsCFiEEqUDUNJksLo6ZED1QIk+n58yCpmQFAl5axhMF CQPCauABQMB0IAQZAQoAHRYhBI1LkwGpNeMYvkhezOAhynPxiakQBQJeWsIzAAoJEOAhynPx iakQleUH/AnO1u/JkytOIKii1ZHH3H92Ru19Bu99cD8U2mVdjo4R9AOK+tZphkWcd3RBvbyv EmrxXkfIKUk2wOPGXZ0vKnw6EpYOVz4Nzpqi2tcKtMH0y8gqnoT1HDiat/ROhNKM+WuvR6JH Pl1LXjBSaPB+UV6DlGUbQrYK6CtrwyMrK59u2V+JIRnM98oG+7nOlfVBAGlKqXVHcRpbgrRY Nuh54h52n2mxqwN7dLPLeIw3RX/x+vxjs+P4uJYDcH216kmq9GoDPaHS0kKbirJXLDcXKEog 3toKuqjhw1oOdx3RfYFgxnNbUfinx+PLBYSU9/9GRlplKV/CbFz3ALEUQiPQV9UJECJPp+fM gqZkzbIQALRoRjiQmyDTkZ/7/tOc0RXEC1zdgeKs3JzegkeoFDvJSZV6TyhkyHmzKjxbGEsx K+srScb9suGCKK//y++8vxTbuzji1910AS+8BiS5S/k5QMKxThKgAsmSpt0rCkYW5hhLoR67 n1pn42dGGS+DlX4+AJMZ/0/sWOC98UWzN3Q6dxcwdPzLd4H3zLpWL9gMR/E2A96v49NgXt/H phqe1EQzA1t1s4dolGvesm5KiET3xhcFAoYDX2CZQ5uCN1s5e4EFVAfTzf58AYXtRaKk5Obn 0Y3E6YBLAT30n7br4QT2nrCmt8pdSN+fPA1idEs00Y/4mEnnl9WJgmym77EmsT/N2T7tmwcJ hUitw7VdTB9wnKKynRM4YuAqtSrq/SzQJeI6is6MivBJYEhlBziXR390iYEboN44RAGulw/y 2ExlIPaQ7OpRzyzQXLUMfxTDyrUuxd/SczEZcwhzNkV4HC0g9WO+aLJq6HdYaHOoxgOFd1jt f4jrpwnHHx0YtOpmzltxOmBip0YRz84KJr686B+/bFpryUZ2eUp8xeFfeBS8/KCvLICBYbRJ 7VnsUkMd6SnGk1hs4av+BKWIFzN68T5ZfUlNZ/BhRFPwIW7IRuUBJLg6ynyOp1QSKvGhSvqA NgbXVD458F5EzAtwcvIOarCGfag4JEdG2Ea/Bhgadge+ Message-ID: <8afebb97-db51-5744-dca9-840dc60cd396@suse.cz> Date: Thu, 26 Mar 2020 23:08:40 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.4.2 MIME-Version: 1.0 In-Reply-To: <202003261256.950F1E5@keescook> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: quoted-printable 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 3/26/20 9:24 PM, Kees Cook wrote: > On Thu, Mar 26, 2020 at 07:16:05PM +0100, Vlastimil Babka wrote: >> Since the major change, I'm sending another RFC. If this approach is o= k, then >> it probably needs just some tweaks to the various error prints, and th= en >> converting the rest of existing on-off aliases (if I come up with an i= dea how >> to find them all). Thanks for all the feedback so far. >=20 > Yeah, I think you can drop "RFC" from this in the next version -- you'r= e > well into getting this finalized IMO. Thanks! >> >> .../admin-guide/kernel-parameters.txt | 9 ++ >> fs/proc/proc_sysctl.c | 90 ++++++++++++++++++= + >> include/linux/sysctl.h | 4 + >> init/main.c | 2 + >> 4 files changed, 105 insertions(+) >> >> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documen= tation/admin-guide/kernel-parameters.txt >> index c07815d230bc..0c7e032e7c2e 100644 >> --- a/Documentation/admin-guide/kernel-parameters.txt >> +++ b/Documentation/admin-guide/kernel-parameters.txt >> @@ -4793,6 +4793,15 @@ >> =20 >> switches=3D [HW,M68k] >> =20 >> + sysctl.*=3D [KNL] >> + Set a sysctl parameter, right before loading the init >> + process, as if the value was written to the respective >> + /proc/sys/... file. Unrecognized parameters and invalid >> + values are reported in the kernel log. Sysctls >> + registered later by a loaded module cannot be set this >> + way. >=20 > Maybe add: "Both '.' and '/' are recognized as separators." OK >> + >> +/* Set sysctl value passed on kernel command line. */ >> +static int process_sysctl_arg(char *param, char *val, >> + const char *unused, void *arg) >> +{ >> + char *path; >> + struct file_system_type *proc_fs_type; >> + struct file *file; >> + int len; >> + int err; >> + loff_t pos =3D 0; >> + ssize_t wret; >> + >> + if (strncmp(param, "sysctl", sizeof("sysctl") - 1)) >> + return 0; >> + >> + param +=3D sizeof("sysctl") - 1; >> + >> + if (param[0] !=3D '/' && param[0] !=3D '.') >> + return 0; >> + >> + param++; >> + >> + if (!proc_mnt) { >> + proc_fs_type =3D get_fs_type("proc"); >> + if (!proc_fs_type) { >> + pr_err("Failed to mount procfs to set sysctl from command line"); >> + return 0; >> + } >> + proc_mnt =3D kern_mount(proc_fs_type); >> + put_filesystem(proc_fs_type); >> + if (IS_ERR(proc_mnt)) { >> + pr_err("Failed to mount procfs to set sysctl from command line"); >> + proc_mnt =3D NULL; >> + return 0; >> + } >> + } >> + >> + len =3D 4 + strlen(param) + 1; >> + path =3D kmalloc(len, GFP_KERNEL); >> + if (!path) >> + panic("%s: Failed to allocate %d bytes t\n", __func__, len); >> + >> + strcpy(path, "sys/"); >> + strcat(path, param); >> + strreplace(path, '.', '/'); >=20 > You can do the replacement against the param directly, and also avoid > all the open-coded string manipulations: >=20 > strreplace(param, '.', '/'); I didn't want to modify param for the sake of error prints, but perhaps the replacements won't confuse system admin too much? > path =3D kasprintf(GFP_KERNEL, "sys/%s", param); Ah yea that's nicer. >> + >> + file =3D file_open_root(proc_mnt->mnt_root, proc_mnt, path, O_WRONLY= , 0); >> + if (IS_ERR(file)) { >> + err =3D PTR_ERR(file); >> + pr_err("Error %d opening proc file %s to set sysctl parameter '%s=3D= %s'", >> + err, path, param, val); >> + goto out; >> + } >> + len =3D strlen(val); >> + wret =3D kernel_write(file, val, len, &pos); >> + if (wret < 0) { >> + err =3D wret; >> + pr_err("Error %d writing to proc file %s to set sysctl parameter '%= s=3D%s'", >> + err, path, param, val); >> + } else if (wret !=3D len) { >> + pr_err("Wrote only %ld bytes of %d writing to proc file %s to set = sysctl parameter '%s=3D%s'", >> + wret, len, path, param, val); >> + } >> + >> + filp_close(file, NULL); >=20 > Please check the return value of filp_close() and treat that as an erro= r > for this function too. Well I could print it, but not much else? The unmount will probably fail in that case? >> +out: >> + kfree(path); >> + return 0; >> +} >> + >> +void do_sysctl_args(void) >> +{ >> + char *command_line; >> + >> + command_line =3D kstrdup(saved_command_line, GFP_KERNEL); >> + if (!command_line) >> + panic("%s: Failed to allocate copy of command line\n", __func__); >> + >> + parse_args("Setting sysctl args", command_line, >> + NULL, 0, -1, -1, NULL, process_sysctl_arg); >> + >> + if (proc_mnt) >> + kern_unmount(proc_mnt); >=20 > I don't recommend sharing allocation lifetimes between two functions > (process_sysctl_arg() allocs proc_mnt, and do_sysctl_args() frees it). > And since you have a scoped lifetime, why allocate it or have it as a > global at all? It can be stack-allocated and passed to the handler: So the point was that the mount is only done when an applicable sysctl parameter is found. On majority of systems there won't be any, at least for initial X years :) > void do_sysctl_args(void) > { > struct file_system_type *proc_fs_type; > struct vfsmount *proc_mnt; > char *command_line; >=20 > proc_fs_type =3D get_fs_type("proc"); > if (!proc_fs_type) { > pr_err("Failed to mount procfs to set sysctl from command line"); > return; > } > proc_mnt =3D kern_mount(proc_fs_type); > put_filesystem(proc_fs_type); > if (IS_ERR(proc_mnt)) { > pr_err("Failed to mount procfs to set sysctl from command line"); > return; > } >=20 > command_line =3D kstrdup(saved_command_line, GFP_KERNEL); > if (!command_line) > panic("%s: Failed to allocate copy of command line\n", > __func__); >=20 > parse_args("Setting sysctl args", command_line, > NULL, 0, -1, -1, proc_mnt, process_sysctl_arg); >=20 > kfree(command_line); > kern_unmount(proc_mnt); > } >=20 > And then pull the mount from (the hilariously overloaded name) "arg": But I guess the "mount on first applicable argument" approach would work with this scheme as well: struct vfsmount *proc_mnt =3D NULL; parse_args(..., &proc_mnt, ...) Thanks!