Alexander Viro wrote:
>
> On Sun, 25 Jun 2000, Andrew Morton wrote:
>
> > > I'm less than happy about ->do_ioctl(), though - are you sure that it can
> > > happen with refcount==0? AFAICS it does not touch the reference counter
> > > and it means that we need to be extra cautious inside...
> >
> > Yes, these methods are called with refcount == 0. For example, the
> > 'mii-diag' tool may be used to manipulate the device's MII when the
> > interface is downed (via netdevice.do_ioctl())
>
> And what happens if it blocks?
Bad things, potentially. It would be most extraordinary if these little
methods _were_ to block, but there's nothing which says they
can't/don't.
> > netdevice.open() is a misnomer. The device is always "open". It should
> > have been called 'start' or 'up'.
>
> Let me get it straight: currently
> a) we have a table of registered devices.
> b) refcount on module is 1 iff interface is up.
> c) methods may be called regardless the refcount value.
> d) modules are loaded by dev_load() and nothing prevents them from
> immediate removal.
> e) normally, module can be unloaded if the interface is down. It
> may destroy settings made by ->do_ioctl().
5 out of 5.
> f) in some cases we do __dev_get_by_name(), in some dev_load().
> Deliberate difference or inconsistence?
And dev_get_by_name().
Note also that dev_change_flags() is a quite low-level function which is
called from various places. The netdevice layer just ain't set up to
handle "self-modifying code".
The call graph by which all the netdevice methods can be called is
pretty straggly. It's a couple of hour's work to write it down, but
once that's done, I suspect any coding which utilised this knowledge
would be fragile in the face of future changes.
http://www.wcug.wwu.edu/lists/netdev/200006/msg00232.html contains a
partial analysis.
> g) in several places we do dev_load() followed by blocking
> operation and __dev_get_by_name(). Seems like we are asking for races
> here, no?
>
> IMO it means that refcounting for these beasts is bogus. I would rather
> see dev_something(name) doing dev_load(), incrementing the reference
> counter and returning the resulting struct net_device * along with
> dev_somethingelse(dev) dropping that counter. That, and additional
> increment/decrement upon up/down. Then we could be sure that whenever we
> pick the structure by name it will stay around until we explicitly say
> that we don't care and we would know that pointers to such structure
> are stored only if the counter is positive (aside of the pointers in
> device table, that is).
>
> Comments?
It'll work. What's your take on the 'grab all the other CPUs' idea?
That pushes all the module uglies into one place.
<brokenrecord> But I would rather be spending time looking at timer
deletion races, which are more important than module unload races - they
have no workaround </brokenrecord>
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.rutgers.edu
Please read the FAQ at http://www.tux.org/lkml/
This archive was generated by hypermail 2b29 : Mon Jun 26 2000 - 21:00:06 EST