From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from out-186.mta0.migadu.com (out-186.mta0.migadu.com [91.218.175.186]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 8413726C384 for ; Fri, 19 Dec 2025 18:10:31 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=91.218.175.186 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1766167834; cv=none; b=oXtEa6OWky2HzwviLVBEXqVUyakfkIF7uHB9kqWFo0XqfClh5QlQ238N/NBhRqtNHoZaf9gZwOyJR/wI/o8zv6z4d9wQvvdJsb47XsAAkT0TQNukU2tl7NeYbpE5HfQuNtzlEDfdOmyLlxv9Cl+tXHNGvg8pPpqtXDwYnIKdywA= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1766167834; c=relaxed/simple; bh=k7G5n5sS38Nyz+axp2fneo7HwLERWaDJf7eY36cszj0=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type; b=pKH5fjYQbbvOUmZ43Z2Lek+/6UyEbkU+PCO6tCLQ31d9qEWAMomNCg9+uV9OBCz8+G7uHrFZZZM4hIK/Q6EU6GGfqsLeXd9N3Q/YN2c44QU9E6CiWXp/l8bR94UdlckNtCiADZguS5dk4QBkpTUbTe1U4Kj2nMHz4dS6DFqBL9I= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linux.dev; spf=pass smtp.mailfrom=linux.dev; dkim=pass (1024-bit key) header.d=linux.dev header.i=@linux.dev header.b=aYrLIxcM; arc=none smtp.client-ip=91.218.175.186 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linux.dev Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=linux.dev Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=linux.dev header.i=@linux.dev header.b="aYrLIxcM" Message-ID: <07477d83-2044-44c2-b334-b08d73d4da2b@linux.dev> DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.dev; s=key1; t=1766167824; h=from:from: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=6lWSl6k3GaYgurMvO+w9QyF9/VQw4zXh8Tpz4TlroCA=; b=aYrLIxcMBu2xCGfXnyhRr7PFVBFS/AXd5Z2lnP424YnWlkSSFWfEQcWlmH5yBaXPriMsaB wpnONBVXgdZpFvZTVsnOQD//2IWDxGqe0mUKYcFx58UXE2xDN7FHSLh2gTSoHrxacAAMFt xCs19lxTcu8crk7tH3fA5pWocLBiG8k= Date: Sat, 20 Dec 2025 02:10:17 +0800 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Subject: Re: [PATCH v5] sysctl: simplify the min/max boundary check To: Joel Granados Cc: "Eric W. Biederman" , Luis Chamberlain , Kees Cook , Christian Brauner , Dave Young , linux-kernel@vger.kernel.org References: <20250105152853.211037-1-wen.yang@linux.dev> <58da9dcb-a4ea-4d23-a7e5-b7f92293831a@linux.dev> <875xm5o0tx.fsf@email.froward.int.ebiederm.org> <87o6zxmlha.fsf@email.froward.int.ebiederm.org> <875xm0gn60.fsf@email.froward.int.ebiederm.org> Content-Language: en-US X-Report-Abuse: Please report any abuse attempt to abuse@migadu.com and include these headers. From: Wen Yang In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Migadu-Flow: FLOW_OUT On 12/19/25 22:44, Joel Granados wrote: > Hey Wen > > I know it has been a very long time, but I think I have a plan to make > this happen. And by "this" I mean changing the extra{1,2} to another > type. I'm writing to you to see if you are still up for pushing it > forward. > > My proposal would go in two big steps. Each one might take more than one > release. > > First step > ========== > Consolidate the way ctl_table entries are created into a SYSCTL_ENTRY > macro. There would be no functional change on this first step. It is > just a preparation step for the second stage. The main objective is to > put all the logic of creating the ctl_table entry in one place (mainly > into sysctl.h). An example of what I mean for step one in [1] > > Second step > =========== > Actually change the extern{1,2} from *void to ulong. Having the logic > in one place makes this easier. Here I'm guessing that we would still > have to handle outliers (like net), but they would be much less. > > In my opinion, if we are going to go through the pain of doing a > treewide change, we might as well make it easier for ourselves to make > systemic changes to sysctl in the future; we do this by having the > creation macro within sysctl.{c,h} > > Tell me if you are still interested and have cycles for interested > Thank you very much. It is my honor to participate in this program, and I am willing to join as soon as possible. -- Best wishes, Wen > Best > > [1] > > diff --git i/include/linux/sysctl.h w/include/linux/sysctl.h > index 39cf1bf9703f..716ad7e41a01 100644 > --- i/include/linux/sysctl.h > +++ w/include/linux/sysctl.h > @@ -58,6 +58,7 @@ extern const int sysctl_vals[]; > #define SYSCTL_LONG_ZERO ((void *)&sysctl_long_vals[0]) > #define SYSCTL_LONG_ONE ((void *)&sysctl_long_vals[1]) > #define SYSCTL_LONG_MAX ((void *)&sysctl_long_vals[2]) > +extern const void *SYSCTL_NULL; > > /** > * > @@ -230,6 +231,23 @@ struct ctl_table { > void *extra2; > } __randomize_layout; > > +#define __SYSCTL_ENTRY(NAME, DATA, TYPE, MODE, HANDLER, SMIN, SMAX)\ > + { \ > + .procname = NAME, \ > + .data = (void*) &DATA, \ > + .maxlen = sizeof(TYPE), \ > + .mode = MODE, \ > + .proc_handler = HANDLER, \ > + .extra1 = SMIN, \ > + .extra2 = SMAX, \ > + } > + > +#define SYSCTL_ENTRY(NAME, DATA, TYPE, MODE) \ > + __SYSCTL_ENTRY(NAME, DATA, TYPE, MODE, proc_do##TYPE##vec, NULL, NULL) > + > +#define SYSCTL_RANGE_ENTRY(NAME, DATA, TYPE, MODE, SMIN, SMAX) \ > + __SYSCTL_ENTRY(NAME, DATA, TYPE, MODE, proc_do##TYPE##vec_minmax, SMIN, SMAX) > + > struct ctl_node { > struct rb_node node; > struct ctl_table_header *header; > diff --git i/kernel/exit.c w/kernel/exit.c > index 8a87021211ae..04e531e45912 100644 > --- i/kernel/exit.c > +++ w/kernel/exit.c > @@ -88,13 +88,7 @@ static unsigned int oops_limit = 10000; > > #ifdef CONFIG_SYSCTL > static const struct ctl_table kern_exit_table[] = { > - { > - .procname = "oops_limit", > - .data = &oops_limit, > - .maxlen = sizeof(oops_limit), > - .mode = 0644, > - .proc_handler = proc_douintvec, > - }, > + SYSCTL_ENTRY("oops_limit", oops_limit, uint, 0644), > }; > > static __init int kernel_exit_sysctls_init(void) > diff --git i/kernel/sysctl.c w/kernel/sysctl.c > index 0965ea1212b4..fa228b89862a 100644 > --- i/kernel/sysctl.c > +++ w/kernel/sysctl.c > @@ -22,6 +22,7 @@ > #include > #include > > +const void *SYSCTL_NULL = NULL; > /* shared constants to be used in various sysctls */ > const int sysctl_vals[] = { 0, 1, 2, 3, 4, 100, 200, 1000, 3000, INT_MAX, 65535, -1 }; > EXPORT_SYMBOL(sysctl_vals); > @@ -1328,47 +1329,16 @@ int proc_do_static_key(const struct ctl_table *table, int dir, > > static const struct ctl_table sysctl_subsys_table[] = { > #ifdef CONFIG_PROC_SYSCTL > - { > - .procname = "sysctl_writes_strict", > - .data = &sysctl_writes_strict, > - .maxlen = sizeof(int), > - .mode = 0644, > - .proc_handler = proc_dointvec_minmax, > - .extra1 = SYSCTL_NEG_ONE, > - .extra2 = SYSCTL_ONE, > - }, > + SYSCTL_RANGE_ENTRY("sysctl_writes_strict", sysctl_writes_strict, int, \ > + 0644, SYSCTL_NEG_ONE, SYSCTL_ONE), > #endif > - { > - .procname = "ngroups_max", > - .data = (void *)&ngroups_max, > - .maxlen = sizeof (int), > - .mode = 0444, > - .proc_handler = proc_dointvec, > - }, > - { > - .procname = "cap_last_cap", > - .data = (void *)&cap_last_cap, > - .maxlen = sizeof(int), > - .mode = 0444, > - .proc_handler = proc_dointvec, > - }, > + SYSCTL_ENTRY("ngroups_max", ngroups_max, int, 0444), > + SYSCTL_ENTRY("cap_last_cap", cap_last_cap, int, 0444), > #ifdef CONFIG_SYSCTL_ARCH_UNALIGN_ALLOW > - { > - .procname = "unaligned-trap", > - .data = &unaligned_enabled, > - .maxlen = sizeof(int), > - .mode = 0644, > - .proc_handler = proc_dointvec, > - }, > + SYSCTL_ENTRY("unaligned-trap", unaligned_enabled, int, 0644), > #endif > #ifdef CONFIG_SYSCTL_ARCH_UNALIGN_NO_WARN > - { > - .procname = "ignore-unaligned-usertrap", > - .data = &no_unaligned_warning, > - .maxlen = sizeof (int), > - .mode = 0644, > - .proc_handler = proc_dointvec, > - }, > + SYSCTL_ENTRY("ignore-unaligned-usertrap", no_unaligned_warning, int, 0644), > #endif > }; > > diff --git i/kernel/ucount.c w/kernel/ucount.c > index 586af49fc03e..b9f4fec7638a 100644 > --- i/kernel/ucount.c > +++ w/kernel/ucount.c > @@ -63,15 +63,9 @@ static struct ctl_table_root set_root = { > static long ue_zero = 0; > static long ue_int_max = INT_MAX; > > -#define UCOUNT_ENTRY(name) \ > - { \ > - .procname = name, \ > - .maxlen = sizeof(long), \ > - .mode = 0644, \ > - .proc_handler = proc_doulongvec_minmax, \ > - .extra1 = &ue_zero, \ > - .extra2 = &ue_int_max, \ > - } > +#define UCOUNT_ENTRY(name) \ > + SYSCTL_RANGE_ENTRY(name, SYSCTL_NULL, ulong, 0644, &ue_zero, &ue_int_max) > + > static const struct ctl_table user_table[] = { > UCOUNT_ENTRY("max_user_namespaces"), > UCOUNT_ENTRY("max_pid_namespaces"), > > On Thu, Feb 27, 2025 at 03:09:12PM +0100, Joel Granados wrote: >> On Thu, Jan 30, 2025 at 10:32:14PM +0800, Wen Yang wrote: >>> >>> >>> On 2025/1/28 01:51, Eric W. Biederman wrote: >>>> Joel Granados writes: >>>> >> ... >>>> that use extra1 or extra2 for something besides min and max. Then >>>> remove extra1 and extra2. At the end it is simpler and requires the >>>> same or a little less space. >>>> >>>> That was and remains my suggestion. >>>> >>> >>> Thanks for your valuable suggestions. We will continue to move forward along >>> it and need your more guidance. >>> >>> But there are also a few codes that do take the extra{1, 2} as pointers, for >>> example: >>> >>> int neigh_sysctl_register(struct net_device *dev, struct neigh_parms *p, >>> proc_handler *handler) >>> { >>> ... >>> for (i = 0; i < NEIGH_VAR_GC_INTERVAL; i++) { >>> t->neigh_vars[i].data += (long) p; >>> t->neigh_vars[i].extra1 = dev; >>> t->neigh_vars[i].extra2 = p; >>> } >>> ... >>> } >>> >>> static void neigh_proc_update(const struct ctl_table *ctl, int write) >>> { >>> struct net_device *dev = ctl->extra1; >>> struct neigh_parms *p = ctl->extra2; >>> struct net *net = neigh_parms_net(p); >>> int index = (int *) ctl->data - p->data; >>> ... >>> } >>> >>> >>> So could we modify it in this way to make it compatible with these two >>> situations: >>> >>> @@ -137,8 +137,16 @@ struct ctl_table { >>> umode_t mode; >>> proc_handler *proc_handler; /* Callback for text formatting */ >>> struct ctl_table_poll *poll; >>> - void *extra1; >>> - void *extra2; >>> + union { >>> + struct { >>> + void *extra1; >>> + void *extra2; >>> + }; >>> + struct { >>> + unsigned long min; >>> + unsigned long max; >>> + }; >>> + }; >>> } __randomize_layout; >> >> I'm still not convinced that a union is the best way out of this. I have >> postponed reviewing this for several weeks, but I'm slowly coming back >> to it. >> >> Thx for your suggestion >> >> Best >> >> >> -- >> >> Joel Granados >