From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751162AbeCHUpV (ORCPT ); Thu, 8 Mar 2018 15:45:21 -0500 Received: from mx2.suse.de ([195.135.220.15]:55333 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751083AbeCHUpU (ORCPT ); Thu, 8 Mar 2018 15:45:20 -0500 Date: Thu, 8 Mar 2018 20:45:18 +0000 From: "Luis R. Rodriguez" To: Waiman Long Cc: "Luis R. Rodriguez" , Andrew Morton , Kees Cook , linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org, Al Viro , Matthew Wilcox Subject: Re: [PATCH v3 3/6] sysctl: Add flags to support min/max range clamping Message-ID: <20180308204518.GL4449@wotan.suse.de> References: <1519926220-7453-1-git-send-email-longman@redhat.com> <1519926220-7453-4-git-send-email-longman@redhat.com> <20180301133117.1d4b27e327f8e51275c82489@linux-foundation.org> <20180308175109.GA4449@wotan.suse.de> <20180308175747.GD4449@wotan.suse.de> <2feea08e-7772-e0aa-af69-05cd7b281725@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <2feea08e-7772-e0aa-af69-05cd7b281725@redhat.com> User-Agent: Mutt/1.6.0 (2016-04-01) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, Mar 08, 2018 at 02:35:32PM -0500, Waiman Long wrote: > On 03/08/2018 12:57 PM, Luis R. Rodriguez wrote: > > On Thu, Mar 08, 2018 at 05:51:09PM +0000, Luis R. Rodriguez wrote: > >> On Thu, Mar 01, 2018 at 01:31:17PM -0800, Andrew Morton wrote: > >>> On Thu, 1 Mar 2018 12:43:37 -0500 Waiman Long wrote: > >>> > >>>> When minimum/maximum values are specified for a sysctl parameter in > >>>> the ctl_table structure with proc_dointvec_minmax() handler, update > >>>> to that parameter will fail with error if the given value is outside > >>>> of the required range. > >>>> > >>>> There are use cases where it may be better to clamp the value of > >>>> the sysctl parameter to the given range without failing the update, > >>>> especially if the users are not aware of the actual range limits. > >>>> Reading the value back after the update will now be a good practice > >>>> to see if the provided value exceeds the range limits. > >>>> > >>>> To provide this less restrictive form of range checking, a new flags > >>>> field is added to the ctl_table structure. The new field is a 16-bit > >>>> value that just fits into the hole left by the 16-bit umode_t field > >>>> without increasing the size of the structure. > >>>> > >>>> When the CTL_FLAGS_CLAMP_RANGE flag is set in the ctl_table entry, > >>>> any update from the userspace will be clamped to the given range > >>>> without error. > >>>> > >>>> ... > >>>> > >>>> --- a/include/linux/sysctl.h > >>>> +++ b/include/linux/sysctl.h > >>>> @@ -116,6 +116,7 @@ struct ctl_table > >>>> void *data; > >>>> int maxlen; > >>>> umode_t mode; > >>>> + uint16_t flags; > >>> It would be nice to make this have type `enum ctl_table_flags', but I > >>> guess there's then no reliable way of forcing it to be 16-bit. > >>> > >>> I guess this is the best we can do... > >>> > >> We can add this to the enum: > >> > >> enum ctl_table_flags { > >> CTL_FLAGS_CLAMP_RANGE = BIT(0), > >> + __CTL_FLAGS_CLAMP_MAX = BIT(16), > >> }; > >> > >> > >> Then also: > >> > >> #define CTL_TABLE_FLAGS_ALL ((BIT(__CTL_FLAGS_CLAMP_MAX + 1))-1) > >> > >> at the end of the definition, then a helper which can be used during > >> parsing: > >> > >> static int check_ctl_table_flags(u16 flags) > >> { > >> if (flags & ~(CTL_TABLE_FLAGS_ALL)) > >> return -ERANGE; > >> return 0; > >> } > >> > >> Waiman please evaluate and add. > > Also, I guess we have ... max bit used and max allowed (16) really, where one is the > > max allowed bit field given current definitions, the other is the max flag possible > > setting in the future. We might as well go with the smaller one, which is the current > > max, so it can just be > > > > enum ctl_table_flags { > > CTL_FLAGS_CLAMP_RANGE = BIT(0), > > __CTL_FLAGS_CLAMP_MAX = BIT(1), > > }; > > > > > > #define CTL_TABLE_FLAGS_ALL ((BIT(__CTL_FLAGS_CLAMP_MAX))-1) > > > > That way we just check against the actual max defined, now the max allowed on > > the entire flag setting. > > > > Luis > > Yes, I can certainly add check to see if the flags are out of range. > However, I would like to know your opinion of what to do when an invalid > flag bit is set. Do we just print a warning in the ring buffer or fail > the registration of the ctl table? We should fail setting. See sysctl_check_table_array(), that should just reject the entry. Luis