Re: sysctl's in modules - specifically compat_netbsd32

[Paul Goyette <paul%whooppee.com@localhost>]

On Sun, 14 Dec 2008, Antti Kantee wrote:

> On Sat Dec 13 2008 at 17:00:20 -0800, Paul Goyette wrote:
> > While tracking down some strange-looking entries in dmesg (as mentioned
> > on current-users) it occurs to me that the current sysctl architecture
> > doesn't lend itself well to having loadable modules declaring new sysctl
> > entries with CTLFLAG_PERMANENT set.  Of particular interest is module
> > compat_netbsd32 which has its own netbsd32_sysctl_init() routine.
> >
> > This routine is called long after the main sysctl_init() has finished
> > and marked the tree as having been set-up (set CTLFLAG_PERMANENT on the
> > root node).  Yet this routine attempts to create several PERMANENT
> > nodes.  These attempts all fail, generating the strange-looking dmesg,
> > but the nodes they attempt to create are apparently created elsewhere,
> > so there's no real damage.
> >
> > For the specific case, it would seem that netbsd32_sysctl_init() is
> > superfluous and unnecessary.  But there might be other modules that want
> > to create entries, and those will fail if the entries being created are
> > marked PERMANENT.  Since modules can be unloaded, it seems to me that
> > modules' sysctl()s should not be marked PERMANENT.
>
> This is a problem with sysctl.  Basically there should be a way to say
> "this is a system-provided node, but it can be unloaded if the code
> providing it is unloaded".  Now you can work around the problem simply
> by providing a storage parameter for the unwind log (which internally
> removes the permanent flag).
>
> Hmm, looking at the code netbsd32_sysctl_init() already provides
> the log parameter.  It must be tripping on something else besides
> CTLFLAG_PERMANENT.

No, it really is tripping over the PERMANENT flag!

In kern/kern_sysctl.c line 736 the check for "init_done" is made by 
looking at the flags of the system's default sysctl tree, regardless of 
which root is being updated.  So even though netbsd32_sysctl_init() is 
adding its shadow nodes to a non-default tree, it fails because of the 
init_done check.

I think the following patch is appropriate.  It changes the init_done 
check to examine the PERMANENT flag in the affected tree's root node, 
rather than the default system tree's node.  (Note I have not yet done 
even a compile test of this patch yet!)

Index: kern/kern_sysctl.c
===================================================================
RCS file: /cvsroot/src/sys/kern/kern_sysctl.c,v
retrieving revision 1.219
diff -u -p -r1.219 kern_sysctl.c
--- kern/kern_sysctl.c  12 Nov 2008 12:36:16 -0000      1.219
+++ kern/kern_sysctl.c  14 Dec 2008 02:18:17 -0000
@@ -733,7 +733,7 @@ sysctl_create(SYSCTLFN_ARGS)
         * the tree itself is not writeable or
         * the entire sysctl system is not writeable
         */
-       if ((sysctl_root.sysctl_flags & CTLFLAG_PERMANENT) &&
+       if ((sysctl_rootof(rnode)->sysctl_flags & CTLFLAG_PERMANENT) &&
            (!(sysctl_rootof(rnode)->sysctl_flags & CTLFLAG_READWRITE) 
||
             !(sysctl_root.sysctl_flags & CTLFLAG_READWRITE)))
                return (EPERM);


> Incidentally, sysctl node registrations in general should move away from
> link sets to provide for modules.  I did a little bit of it recently,
> but there's still a lot left.

There's always "a lot left" when you start something like that!  :)


----------------------------------------------------------------------
|   Paul Goyette   | PGP DSS Key fingerprint: |  E-mail addresses:   |
| Customer Service | FA29 0E3B 35AF E8AE 6651 |  paul%whooppee.com@localhost   |
| Network Engineer | 0786 F758 55DE 53BA 7731 | pgoyette%juniper.net@localhost |
----------------------------------------------------------------------