Re: sysctl locking

On Monday 13 December 2004 09:13, I wrote:
> On Monday 13 December 2004 08:14, Suleiman Souhlal wrote:
> > Hello,
> >
> > The patch at http://people.freebsd.org/~ssouhlal/sysctl-locking.diff
> > removes Giant from the sysctl subsystem.
> > I tested it on i386 and powerpc, where it appears to work perfectly.
> > However, I have not been able to test it on an SMP box, as I don't have
> > access to any.
> > So I would appreciate it if someone would test it, and report any
> > problems.
> > I will commit it in about week, unless, of course, there are
> > objections/problems.
>
> Wait a minute ... you can't just assert that all sysctl handler are MPSAFE.
> It's a good idea to introduce "real" locking for the sysctl-tree handling
> in order to be able to lose Giant at a later point, but I *strongly*
> suggest that you keep on grabing Giant before calling oid_handler() in
> sysctl_root(). It doesn't seem like you do so, right now.
>
> You have identified two places where Giant is explicitly asserted, but I am
> afraid that there are much more handlers that don't assert Giant but need
> it. Moreover, the "simple" handler might also write to memory that is
> implicitly protected by Giant and should not be modified without it.
>
> As a transition step I suggest that we extend the API in the way the
> callout API works. So that you can ask for a Giant-free handler call by
> setting an MPSAFE flag.

This got me wondering a bit how well we protect sysctls at the moment or if we 
have code that just assumes atomicy for sysctls. As an example of a very well 
protected sysctl there is kern.securelevel, which has it's own handler that 
uses it's own mutex to protect the change and a defined API to read the value 
(useing the same mutex to protect the access). Other values are not so well 
protected.

It is safe for some sysctls - with just on/off semantic (net.inet.forwarding 
e.g.) - to read them unprotected. As the user can decide to flip the switch 
in a loop, the code should cope with a (short) unstable value anyway.

Sysctls that describe maximum buffer sizes, portranges or maximal number of 
threads per process are more dangerous and might need attention. With bad 
timing we might have very undesired effects.

The "complex" sysctls that implement their own handlers are not a concern, as 
they are usually implemented within the .c file that has the appropriate 
mutex and can use proper locking if required.

If we come to the conclusion that it is required to protect these values 
better, I suggest the following:

1) Extend sysctl_add_oid() to accept an additional mutex argument.
2) Extend the simple sysctl handler to use this mutex to protect the actual 
   write(?read?). We must not hold the mutex during the useland copy in/out so 
   we must move to temporary storage.
3) To maintain the current API and behavior we use &Giant as the default 
   fallback argument. This might need some extension for complex handler (i.e. 
   no mutex given -> acquire Giant before calling the complex handler).

What do people think of this? Does it make any sense? Should we be concerned 
at all? Does the extension make sense? Comments?

[CCing -arch]

-- 
/"\  Best regards,                      | mlaier_at_freebsd.org
\ /  Max Laier                          | ICQ #67774661
 X   http://pf4freebsd.love2party.net/  | mlaier_at_EFnet
/ \  ASCII Ribbon Campaign              | Against HTML Mail and News