modules registering as sysctl handlers

modules registering as sysctl handlers

[Muli Ben-Yehuda]

[-- Attachment #1: Type: text/plain, Size: 488 bytes --]

Hi 

Looking at 2.6.3-bk, it appears that the sysctl code does not raise a
module's reference count before calling a sysctl handler registered by
that module. 

- are modules allowed to register sysctl handlers?
register_sysctl_table is exported, so I imagine so. 

- am I missing something and modules are in fact protected against
concurrent unloading and invocation of sysctl?

Cheers, 
Muli 
-- 
Muli Ben-Yehuda
http://www.mulix.org | http://mulix.livejournal.com/


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: modules registering as sysctl handlers

[viro]

On Tue, Mar 02, 2004 at 02:29:09PM +0200, Muli Ben-Yehuda wrote:
> Hi 
> 
> Looking at 2.6.3-bk, it appears that the sysctl code does not raise a
> module's reference count before calling a sysctl handler registered by
> that module. 
> 
> - are modules allowed to register sysctl handlers?
> register_sysctl_table is exported, so I imagine so. 
> 
> - am I missing something and modules are in fact protected against
> concurrent unloading and invocation of sysctl?

They are not and no, bumping refcount would not be anywhere near enough.
It's not just modules - no module refcount will help e.g. per-interface
sysctls.  All dynamic sysctls are b0rken and the best course of action
is to avoid using that crap.

We'll need to fix that mess, but it won't be easy.

Re: modules registering as sysctl handlers

[Rusty Russell]

On Tue, 2004-03-02 at 23:41, viro@parcelfarce.linux.theplanet.co.uk
wrote:
> On Tue, Mar 02, 2004 at 02:29:09PM +0200, Muli Ben-Yehuda wrote:
> > Hi 
> > 
> > Looking at 2.6.3-bk, it appears that the sysctl code does not raise a
> > module's reference count before calling a sysctl handler registered by
> > that module. 
> > 
> > - are modules allowed to register sysctl handlers?
> > register_sysctl_table is exported, so I imagine so. 
> > 
> > - am I missing something and modules are in fact protected against
> > concurrent unloading and invocation of sysctl?
> 
> They are not and no, bumping refcount would not be anywhere near enough.

Al is referring to the fact that there's no protection for a dynamically
allocated ctl_table.

However, an owner field and standard module_get() would solve the case
of statically declared ctl_table.

I don't know that there's a current user who requires it though?

Rusty.
-- 
Anyone who quotes me in their signature is an idiot -- Rusty Russell

Re: modules registering as sysctl handlers

[Muli Ben-Yehuda]

[-- Attachment #1: Type: text/plain, Size: 1359 bytes --]

On Wed, Mar 03, 2004 at 11:05:39AM +1100, Rusty Russell wrote:

> Al is referring to the fact that there's no protection for a dynamically
> allocated ctl_table.
> 
> However, an owner field and standard module_get() would solve the case
> of statically declared ctl_table.

That's what I had in mind. 
 
> I don't know that there's a current user who requires it though?

Yes, there are. Using the attached scriptlet on a 2.6.1 tree I had
lying around: 

[root@hydra kernel]# /tmp/find-register-sysctl 
/lib/modules/2.6.1/kernel/drivers/cdrom/cdrom.ko uses register_sysctl
/lib/modules/2.6.1/kernel/drivers/parport/parport.ko uses register_sysctl
/lib/modules/2.6.1/kernel/net/appletalk/appletalk.ko uses register_sysctl
/lib/modules/2.6.1/kernel/net/ipx/ipx.ko uses register_sysctl
/lib/modules/2.6.1/kernel/net/irda/irda.ko uses register_sysctl
/lib/modules/2.6.1/kernel/net/sctp/sctp.ko uses register_sysctl

I'm building 2.6.3-bk with allmodconfig now. Once it's done, I'll post
the resulting list. 

#!/bin/sh

KERNEL_VER=2.6.1
for f in `find /lib/modules/${KERNEL_VER} -name "*.ko"`; do 
    nm $f | grep register_sysctl 2>&1 > /dev/null
    if [ "x$?" == "x0" ]; then
        echo "$f uses register_sysctl"
    fi
done

Cheers, 
Muli 
-- 
Muli Ben-Yehuda
http://www.mulix.org | http://mulix.livejournal.com/


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: modules registering as sysctl handlers

[viro]

On Wed, Mar 03, 2004 at 11:22:39AM +0200, Muli Ben-Yehuda wrote:
> > However, an owner field and standard module_get() would solve the case
> > of statically declared ctl_table.
> 
> That's what I had in mind. 
>  
> > I don't know that there's a current user who requires it though?
> 
> Yes, there are. Using the attached scriptlet on a 2.6.1 tree I had
> lying around: 
> 
> [root@hydra kernel]# /tmp/find-register-sysctl 
> /lib/modules/2.6.1/kernel/drivers/cdrom/cdrom.ko uses register_sysctl
> /lib/modules/2.6.1/kernel/drivers/parport/parport.ko uses register_sysctl
> /lib/modules/2.6.1/kernel/net/appletalk/appletalk.ko uses register_sysctl
> /lib/modules/2.6.1/kernel/net/ipx/ipx.ko uses register_sysctl
> /lib/modules/2.6.1/kernel/net/irda/irda.ko uses register_sysctl
> /lib/modules/2.6.1/kernel/net/sctp/sctp.ko uses register_sysctl

	At least parport has both module-wide and per-port sysctls.  The
latter are dynamic even if module support is turned off.  I seriously
suspect that other examples are similar.

Re: modules registering as sysctl handlers

[Muli Ben-Yehuda]

[-- Attachment #1: Type: text/plain, Size: 2240 bytes --]

On Wed, Mar 03, 2004 at 10:43:32AM +0000, viro@parcelfarce.linux.theplanet.co.uk wrote:

> At least parport has both module-wide and per-port sysctls.  The
> latter are dynamic even if module support is turned off.  I seriously
> suspect that other examples are similar.

For reference, here's the list of register_sysctl_table() users from
2.6.3-bk with an "almost allmodconfig" configuration. Those that I've
randomly looked at all used a statically allocated ctl_table
structure. Will adding a .owner field to it with the relevant
module_get/module_put in the registration functions hinder future
efforts to fix it properly? It won't be perfect, but it will be better
than what we currently have. 

./drivers/cdrom/cdrom.ko uses register_sysctl
./drivers/char/rtc.ko uses register_sysctl
./drivers/cpufreq/cpufreq_userspace.ko uses register_sysctl
./drivers/md/md.ko uses register_sysctl
./drivers/net/wireless/arlan.ko uses register_sysctl
./drivers/parport/parport.ko uses register_sysctl
./drivers/scsi/scsi_mod.ko uses register_sysctl
./fs/coda/coda.ko uses register_sysctl
./fs/lockd/lockd.ko uses register_sysctl
./fs/ntfs/ntfs.ko uses register_sysctl
./fs/xfs/xfs.ko uses register_sysctl
./net/appletalk/appletalk.ko uses register_sysctl
./net/ax25/ax25.ko uses register_sysctl
./net/bridge/bridge.ko uses register_sysctl
./net/decnet/decnet.ko uses register_sysctl
./net/ipv4/ipvs/ip_vs_lblc.ko uses register_sysctl
./net/ipv4/ipvs/ip_vs_lblcr.ko uses register_sysctl
./net/ipv4/ipvs/ip_vs.ko uses register_sysctl
./net/ipv4/netfilter/ip_conntrack.ko uses register_sysctl
./net/ipv4/netfilter/ip_queue.ko uses register_sysctl
./net/ipv6/netfilter/ip6_queue.ko uses register_sysctl
./net/ipv6/ipv6.ko uses register_sysctl
./net/ipx/ipx.ko uses register_sysctl
./net/irda/irda.ko uses register_sysctl
./net/netrom/netrom.ko uses register_sysctl
./net/rose/rose.ko uses register_sysctl
./net/rxrpc/rxrpc.ko uses register_sysctl
./net/sctp/sctp.ko uses register_sysctl
./net/sunrpc/sunrpc.ko uses register_sysctl
./net/unix/unix.ko uses register_sysctl
./net/x25/x25.ko uses register_sysctl

Cheers, 
Muli 
-- 
Muli Ben-Yehuda
http://www.mulix.org | http://mulix.livejournal.com/


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: modules registering as sysctl handlers

[Dave Jones]

On Thu, Mar 04, 2004 at 12:38:25PM +0200, Muli Ben-Yehuda wrote:

 > ./drivers/cpufreq/cpufreq_userspace.ko uses register_sysctl

Obsolete, and will be going away in 2.7
There's a perfectly functional sysfs interface for this stuff now.
 

		Dave