Linux-api Archive on lore.kernel.org
 help / color / Atom feed
From: ebiederm@xmission.com (Eric W. Biederman)
To: Michal Hocko <mhocko@kernel.org>
Cc: Vlastimil Babka <vbabka@suse.cz>,
	Luis Chamberlain <mcgrof@kernel.org>,
	Kees Cook <keescook@chromium.org>,
	Iurii Zaikin <yzaikin@google.com>,
	linux-kernel@vger.kernel.org, linux-api@vger.kernel.org,
	linux-mm@kvack.org, Ivan Teterevkov <ivan.teterevkov@nutanix.com>,
	David Rientjes <rientjes@google.com>,
	Matthew Wilcox <willy@infradead.org>,
	"Guilherme G . Piccoli" <gpiccoli@canonical.com>
Subject: Re: [RFC v2 1/2] kernel/sysctl: support setting sysctl parameters from kernel command line
Date: Thu, 26 Mar 2020 07:45:13 -0500
Message-ID: <87bloj2skm.fsf@x220.int.ebiederm.org> (raw)
In-Reply-To: <20200326065829.GC27965@dhcp22.suse.cz> (Michal Hocko's message of "Thu, 26 Mar 2020 07:58:29 +0100")

Michal Hocko <mhocko@kernel.org> writes:

> On Wed 25-03-20 17:20:40, Eric W. Biederman wrote:
>> Vlastimil Babka <vbabka@suse.cz> writes:
> [...]
>> > +	if (strncmp(param, "sysctl.", sizeof("sysctl.") - 1))
>> > +		return 0;
>> 
>> Is there any way we can use a slash separated path.  I know
>> in practice there are not any sysctl names that don't have
>> a '.' in them but why should we artifically limit ourselves?
>
> Because this is the normal userspace interface? Why should it be any
> different from calling sysctl?
> [...]

Why should the kernel command line implement userspace whims?
I was thinking something like: "sysctl/kernel/max_lock_depth=2048"
doesn't look too bad and it makes things like reusing our
kernel internal helpers much easier.

Plus it suggest that we could do the same for sysfs files:
	"sysfs/kernel/fscaps=1"

And the code could be same for both cases except for the filesystem
prefix.

>> Further it will be faster to lookup the sysctls using the code from
>> proc_sysctl.c as it constructs an rbtree of all of the entries in
>> a directory.  The code might as well take advantage of that for large
>> directories.
>
> Sounds like a good fit for a follow up patch to me. Let's make this
> as simple as possible for the initial version. But up to Vlastimil of course.

I would argue that reusing proc_sysctl.c:lookup_entry() should make the
code simpler, and easier to reason about.

Especially given the bugs in the first version with a sysctl path.
A clean separation between separating the path from into pieces and
looking up those pieces should make the code more robust.

That plus I want to get very far away from the incorrect idea that you
can have sysctls without compiling in proc support.  That is not how
the code works, that is not how the code is tested.

It is also worth pointing out that:

	proc_mnt = kern_mount(proc_fs_type);
        for_each_sysctl_cmdline() {
        	...
		file = file_open_root(proc_mnt->mnt_root, proc_mnt, sysctl_path, O_WRONLY, 0);
		kernel_write(file, value, value_len);
        }
        kern_umount(proc_mnt);

Is not an unreasonable implementation.

There are problems with a persistent mount of proc in that it forces
userspace not to use any proc mount options.  But a temporary mount of
proc to deal with command line options is not at all unreasonable.
Plus it looks like we can have kern_write do all of the kernel/user
buffer silliness.

> [...]
>
>> Hmm.  There is a big gotcha in here and I think it should be mentioned.
>> This code only works because no one has done set_fs(KERNEL_DS).  Which
>> means this only works with strings that are kernel addresses essentially
>> by mistake.  A big fat comment documenting why it is safe to pass in
>> kernel addresses to a function that takes a "char __user*" pointer
>> would be very good.
>
> Agreed

Eric

  parent reply index

Thread overview: 18+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-03-25 12:03 Vlastimil Babka
2020-03-25 12:03 ` [RFC v2 2/2] kernel/sysctl: support handling command line aliases Vlastimil Babka
2020-03-25 14:29   ` Michal Hocko
2020-03-25 14:36     ` Vlastimil Babka
2020-03-25 14:44       ` Michal Hocko
2020-03-25 22:42   ` Kees Cook
2020-03-29 15:00   ` Arvind Sankar
2020-03-25 21:21 ` [RFC v2 1/2] kernel/sysctl: support setting sysctl parameters from kernel command line Kees Cook
2020-03-26  9:30   ` Vlastimil Babka
2020-03-25 22:20 ` Eric W. Biederman
2020-03-25 22:54   ` Kees Cook
2020-03-26  6:58   ` Michal Hocko
2020-03-26  7:21     ` Kees Cook
2020-03-26 12:45     ` Eric W. Biederman [this message]
2020-03-30 22:09       ` Luis Chamberlain
2020-03-26 13:30     ` Christian Brauner
2020-03-26 13:39       ` Michal Hocko
2020-03-26 13:29   ` Vlastimil Babka

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=87bloj2skm.fsf@x220.int.ebiederm.org \
    --to=ebiederm@xmission.com \
    --cc=gpiccoli@canonical.com \
    --cc=ivan.teterevkov@nutanix.com \
    --cc=keescook@chromium.org \
    --cc=linux-api@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=mcgrof@kernel.org \
    --cc=mhocko@kernel.org \
    --cc=rientjes@google.com \
    --cc=vbabka@suse.cz \
    --cc=willy@infradead.org \
    --cc=yzaikin@google.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-api Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-api/0 linux-api/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-api linux-api/ https://lore.kernel.org/linux-api \
		linux-api@vger.kernel.org
	public-inbox-index linux-api

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-api


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git