Re: [PATCH] sysctl: Add a group of macro functions to initcall the sysctl table of each feature

[Eric W. Biederman]

Luis Chamberlain <mcgrof@xxxxxxxxxx> writes:

> On Mon, Dec 06, 2021 at 05:38:42PM -0800, Andrew Morton wrote:
>> On Tue, 7 Dec 2021 09:13:20 +0800 Xiaoming Ni <nixiaoming@xxxxxxxxxx> wrote:
>> > --- a/fs/inode.c
>> > +++ b/fs/inode.c
>> > @@ -132,12 +132,7 @@ static struct ctl_table inodes_sysctls[] = {
>> >  	{ }
>> >  };
>> >  
>> > -static int __init init_fs_inode_sysctls(void)
>> > -{
>> > -	register_sysctl_init("fs", inodes_sysctls);
>> > -	return 0;
>> > -}
>> > -early_initcall(init_fs_inode_sysctls);
>> > +fs_sysctl_initcall(inodes_sysctls);
>> >  #endif
>> 
>> Here's another, of many.
>> 
>> Someone made the decision to use early_initcall() here (why?) and this
>> patch switches it to late_initcall()!  Worrisome.  Each such stealth
>> conversion should be explained and justified, shouldn't it?
>
> I made the decisions for quite a bit of the ordering and yes I agree
> this need *very careful* explanation, specially if we are going to
> generalize this.
>
> First and foremost. git grep for sysctl_init_bases and you will see
> that the bases for now are initialized on proc_sys_init() and that
> gets called on proc_root_init() and that in turn on init/main.c's
> start_kernel(). And so this happens *before* the init levels.
>
> The proper care for what goes on top of this needs to take into
> consideration the different init levels and that the if a sysctl
> is using a directory *on top* of a base, then that sysctl registration
> must be registered *after* that directory. The *base* directory for
> "fs" is now registered through fs/sysctls.c() on init_fs_sysctls()
> using register_sysctl_base(). I made these changes with these names
> and requiring the DECLARE_SYSCTL_BASE() so it would be easy for us
> to look at where these are declared.
>
> So the next step in order to consider is *link* ordering and that
> order is maintained by the Makefile. That is why I put this at the
> top of the fs Makfile:
>
> obj-$(CONFIG_SYSCTL)            += sysctls.o 
>
> So any file after this can use early_initcall(), because the base
> for "fs" was declared first in link order, and it used early_initcall().
> It is fine to have the other stuff that goes on top of the "fs" base
> use late_initcall() but that assumes that vetting has been done so that
> if a directory on "fs" was created, let's call it "foo", vetting was done
> to ensure that things on top of "foo" are registered *after* the "foo"
> directory.
>
> We now have done the cleanup for "fs", and we can do what we see fine
> for "fs", but we may run into surprises later with the other bases, so
> I'd be wary of making assumptions at this point if we can use
> late_initcall().
>
> So, as a rule of thumb I'd like to see bases use early_initcall(). The
> rest requires manual work and vetting.
>
> So, how about this, we define fs_sysctl_initcall() to use also
> early_initcall(), and ask susbsystems to do their vetting so that
> the base also gets linked first.
>
> After this, if a directory on top of a base is created we should likely create
> a new init level and just bump that to use the next init level. So
> something like fs_sysctl_base_initcall_subdir_1() map to core_initcall()
> and so on.
>
> That would allow us to easily grep for directory structures easily and
> puts some implicit onus of ordering on those folks doing these conversions.
> We'd document well the link order stuff for those using the base stuff
> too as that is likely only where this will matter most.

I am a bit confused at this explanation of things.

Last I looked the implementation of sysctls allocated the directories
independently of the sysctls entries that populated them.

Which should be that as long as there are not conflicts in name of
directory entries, any order should work.

Am I misremembering how the code works?


Eric