tech-kern archive

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]

Re: sysctl's in modules - specifically compat_netbsd32

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

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 |   |
| Network Engineer | 0786 F758 55DE 53BA 7731 | |

Home | Main Index | Thread Index | Old Index