From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from out02.mta.xmission.com (out02.mta.xmission.com [166.70.13.232]) (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 9426317BA1 for ; Mon, 27 Jan 2025 17:53:15 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=166.70.13.232 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1738000398; cv=none; b=kzZP9ztt481NNegIJZWAhyZ0SSZODSG6KUFMh1cuTlUjGifiz4En1x8usnHOPG8SjXA/dIikR+0kPYboKN0MLs4C+QMwwbDbWqeaPilfwDzqLTtjvGwygkdf6e8GXnCRfa6Ci2x/1pOPPB8frM5p5O6498U4bE/egpKA6Yz7Mxg= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1738000398; c=relaxed/simple; bh=cZtNDaPRjR6MfonB8Cc6xBG+YGC7M4UBVJEqBebLxgU=; h=From:To:Cc:References:Date:In-Reply-To:Message-ID:MIME-Version: Content-Type:Subject; b=RGZX8wd3pEZTtnwWILXLkKyozSp5llPq7c3uU0UxUSICxXZ6UJnacYlraOgvaj+O7ODyyPSiYUP2ul8LAWcKowRtxiI5h22I/1n81BNtnJ4ve7bXOsq5CFHkOza8C6y1FamisodqMWMKZuw81+DUYBrsOS2mKUfBQQWdJCLOgO4= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=xmission.com; spf=pass smtp.mailfrom=xmission.com; arc=none smtp.client-ip=166.70.13.232 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=xmission.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=xmission.com Received: from in01.mta.xmission.com ([166.70.13.51]:55708) by out02.mta.xmission.com with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.93) (envelope-from ) id 1tcTIK-007127-9U; Mon, 27 Jan 2025 10:53:08 -0700 Received: from ip72-198-198-28.om.om.cox.net ([72.198.198.28]:56634 helo=email.froward.int.ebiederm.org.xmission.com) by in01.mta.xmission.com with esmtpsa (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.93) (envelope-from ) id 1tcTIJ-00G2d4-0P; Mon, 27 Jan 2025 10:53:07 -0700 From: "Eric W. Biederman" To: Joel Granados Cc: Wen Yang , 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> Date: Mon, 27 Jan 2025 11:51:51 -0600 In-Reply-To: (Joel Granados's message of "Mon, 27 Jan 2025 14:34:12 +0100") Message-ID: <875xm0gn60.fsf@email.froward.int.ebiederm.org> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/28.2 (gnu/linux) Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain X-XM-SPF: eid=1tcTIJ-00G2d4-0P;;;mid=<875xm0gn60.fsf@email.froward.int.ebiederm.org>;;;hst=in01.mta.xmission.com;;;ip=72.198.198.28;;;frm=ebiederm@xmission.com;;;spf=pass X-XM-AID: U2FsdGVkX197h5EFM1WcEa/c4QpC+xw/rDs8RUmVvwM= X-Spam-Level: X-Spam-Virus: No X-Spam-Report: * -1.0 ALL_TRUSTED Passed through trusted hosts only via SMTP * 0.8 BAYES_50 BODY: Bayes spam probability is 40 to 60% * [score: 0.4931] * 0.0 T_TM2_M_HEADER_IN_MSG BODY: No description available. * -0.0 DCC_CHECK_NEGATIVE Not listed in DCC * [sa02 1397; Body=1 Fuz1=1 Fuz2=1] X-Spam-DCC: XMission; sa02 1397; Body=1 Fuz1=1 Fuz2=1 X-Spam-Combo: ;Joel Granados X-Spam-Relay-Country: X-Spam-Timing: total 693 ms - load_scoreonly_sql: 0.03 (0.0%), signal_user_changed: 3.5 (0.5%), b_tie_ro: 2.4 (0.3%), parse: 1.31 (0.2%), extract_message_metadata: 16 (2.2%), get_uri_detail_list: 5 (0.7%), tests_pri_-2000: 15 (2.2%), tests_pri_-1000: 2.9 (0.4%), tests_pri_-950: 1.47 (0.2%), tests_pri_-900: 1.12 (0.2%), tests_pri_-90: 103 (14.9%), check_bayes: 101 (14.6%), b_tokenize: 10 (1.4%), b_tok_get_all: 14 (2.0%), b_comp_prob: 4.4 (0.6%), b_tok_touch_all: 70 (10.0%), b_finish: 0.75 (0.1%), tests_pri_0: 537 (77.5%), check_dkim_signature: 0.45 (0.1%), check_dkim_adsp: 3.5 (0.5%), poll_dns_idle: 2.2 (0.3%), tests_pri_10: 1.81 (0.3%), tests_pri_500: 6 (0.9%), rewrite_mail: 0.00 (0.0%) Subject: Re: [PATCH v5] sysctl: simplify the min/max boundary check X-SA-Exim-Connect-IP: 166.70.13.51 X-SA-Exim-Rcpt-To: linux-kernel@vger.kernel.org, dyoung@redhat.com, brauner@kernel.org, keescook@chromium.org, mcgrof@kernel.org, wen.yang@linux.dev, joel.granados@kernel.org X-SA-Exim-Mail-From: ebiederm@xmission.com X-SA-Exim-Scanned: No (on out02.mta.xmission.com); SAEximRunCond expanded to false Joel Granados writes: > On Thu, Jan 23, 2025 at 12:30:25PM -0600, Eric W. Biederman wrote: >> "Eric W. Biederman" writes: >> >> > Joel Granados writes: >> > >> >> On Sun, Jan 19, 2025 at 10:59:21PM +0800, Wen Yang wrote: >> >>> >> >>> >> >>> On 2025/1/16 17:37, Joel Granados wrote: >> >>> > On Sun, Jan 05, 2025 at 11:28:53PM +0800, Wen Yang wrote: >> >>> > > do_proc_dointvec_conv() used the default range of int type, while >> >>> > > do_proc_dointvec_minmax_conv() additionally used "int * {min, max}" of >> >>> > > struct do_proc_dointvec_minmax_conv_param, which are actually passed >> >>> > > in table->extra{1,2} pointers. > ... >> >> (if any). And this is why: >> >> 1. The long and the void* are most likely (depending on arch?) the same >> >> size. >> >> 2. In [1] it is mentioned that, we would still need an extra (void*) to >> >> address the sysctl tables that are *NOT* using extra{1,2} as min max. >> >> This means that we need a bigger ctl_table (long extra1, long extra2 >> >> and void* extra). We will need *more* memory? >> >> >> >> I would like to be proven wrong. So this is my proposal: Instead of >> >> trying to do an incremental change, I suggest you remove the sysctl_vals >> >> shared const array and measure how much memory you actually save. You >> >> can use the ./scripts/bloat-o-meter in the linux kernel source and >> >> follow something similar to what we did in [2] to measure how much >> >> memory we are actually talking about. >> >> >> >> Once you get a hard number, then we can move forward on the memory >> >> saving front. > > Hey Eric. > > Thx for the clarification. Much appreciated. >> > >> > When I originally suggested this my motivation had nothing to do with memory > That makes a *lot* of sense :). > >> > The sysctl_vals memory array is type unsafe and has actively > Here I understand that they are unsafe because of Integer promotion > issues exacerbated by the void* variables extra{1,2}. Please correct me > If I missed the point. Not precisely. It is because the (void *) pointers are silently cast to either (int *) or (long *) pointers. So for example passing SYSCTL_ZERO to proc_do_ulongvec_minmax results in reading sysctl_vals[0] and sysctl_vals[1] and to get the long value. Since sysctl_vals[1] is 1 a 0 is not accepted because 0 is below the minimum. The minimum value that is accepted depends on which architecture you are on. On x86_64 and other little endian architectures the minimum value accepted is 0x0000000100000000. On big endian architectures like mips64 the minimum value accepted winds up being 0x0000000000000001. Or do I have that backwards? It doesn't matter because neither case is what the programmer expected. Further it means that keeping the current proc_do_ulongvec_minmax and proc_do_int_minmax methods that it is impossible to define any of the SYSCTL_XXX macros except SYSCTL_ZERO that will work with both methods. > There is also the fact that you can just do a `extra1 = &sysctl_vals[15]` > and the compiler will not bark at you. At least It let me do that on my > side. All of which in the simplest for has me think the SYSCTL_XXX cleanups were a step in the wrong direction. >> > lead to real world bugs. AKA longs and int confusion. One example is >> > that SYSCTL_ZERO does not properly work as a minimum to >> > proc_do_ulongvec_minmax. > That is a great example. > >> > >> > Frankly those SYSCTL_XXX macros that use sysctl_vals are just plain >> > scary to work with. > I share your feeling :) > >> > >> > So I suggested please making everything simpler by putting unsigned long >> > min and max in to struct ctl_table and then getting rid of extra1 and >> > extra2. As extra1 and extra2 are almost exclusively used to implement >> > min and max. > Explicitly specifying the type will help reduce the "unsefeness" but > with all the ways that there are of using these pointers, I think we > need to think bigger and maybe try to find a more typesafe way to > represent all the interactions. > > It has always struck me as strange the arbitrariness of having 2 extra > pointers. Why not just one? Which would be the void *data pointer. > At the end it is a pointer and can point to > a struct that holds min, max... I do not have the answer yet, but I > think what you propose here is part of a bigger refactoring needed in > ctl_table structure. Would like to hear your thought on it if you have > any. One of the things that happens and that is worth acknowledging is there is code that wraps proc_doulongvec_minmax and proc_dointvec_minmax. Having the minmax information separate from the data pointer makes that wrapping easier. Further the min/max information is typically separate from other kinds of data. So even when not wrapped it is nice just to take a quick glance and see what the minimums and maximums are. My original suggest was that we change struct ctl_table from: > /* A sysctl table is an array of struct ctl_table: */ > struct ctl_table { > const char *procname; /* Text ID for /proc/sys */ > void *data; > int maxlen; > umode_t mode; > proc_handler *proc_handler; /* Callback for text formatting */ > struct ctl_table_poll *poll; > void *extra1; > void *extra2; > } __randomize_layout; to: > /* A sysctl table is an array of struct ctl_table: */ > struct ctl_table { > const char *procname; /* Text ID for /proc/sys */ > void *data; > int maxlen; > umode_t mode; > proc_handler *proc_handler; /* Callback for text formatting */ > struct ctl_table_poll *poll; > unsigned long min; > unsigned long max; > } __randomize_layout; That is just replace extra1 and extra2 with min and max members. The members don't have any reason to be pointers. Without being pointers the min/max functions can just use long values to cap either ints or longs, and there is no room for error. The integer promotion rules will ensure that even negative values can be stored in unsigned long min and max values successfully. Plus it is all bog standard C so there is nothing special to learn. There are a bunch of fiddly little details to transition from where we are today. The most straightforward way I can see of making the transition is to add the min and max members. Come up with replacements for proc_doulongvec_minmax and proc_dointvec_minmax that read the new min and max members. Update all of the users. Update the few users 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. Eric