Re: Are modunload and ifconfig create racy?
Hi,
On 2017/01/21 6:47, Paul Goyette wrote:
> (Replying _without_ looking at the code! I can look more deeply if you
> could re-send a pointer to the code...)
In the example of gre(4) interface, if CPU#0 do "modunload if_gre" and
CPU#1 do "ifconfig gre0 create", they seems to have below race as far as
if_gre.c.
+ CPU#1
- call gre_clone_create()
- before done gre_count increment, that is before line 369
https://nxr.netbsd.org/xref/src/sys/net/if_gre.c#327
+ CPU#0
- call gredetach()
- done gre_count check, that is, reached line 1464
https://nxr.netbsd.org/xref/src/sys/net/if_gre.c#1464
+ CPU#1
- continue gre_clone_create()
- there is no checking code if gredetach() is being called or not
in gre_clone_create(), so continue
At this point, CPU#0 continue to modunload processing while CPU#1 continue
to ifconfig create. What will happen? I think fault would arise and panic
in CPU#1 at some point.
> Generally, the module's unload code should detach the devsw entry for
> the driver first, to prevent any new accesses.
>
> Then, the config_fini_component() call will fail if existing devices
> cannot be detached. If this does fail, the module unload code should
> re-attach the devsw entry and return failure.
The pseudo interface modules' load/unload code is generated by IF_MODULE
macro
https://nxr.netbsd.org/xref/src/sys/net/if_module.h#54
The unload code seems to call each inteface's detach code(such as gredetach()),
and then, the code calls config_cfdriver_detach()(not config_fini_component()).
Hmm, isn't this general way?
> The only chance I see for any race condition is if the open/create logic
> can clone a new entry which is not seen by config_cfdata_detach() when
> it scans for device instances which need to be detached.
>
> If there is a race condition, it should be solved in each interface's
> driver module, or within the autoconfig framework. Each driver module
> is responsible for managing its own integration with autoconfig (ie,
> calling config_{init,fini}_component and devsw_{att},det}ach routines).
I think the race condition should solved in IF_MODULE macro. However,
I don't have an appropriate solution. It may be required to modify
if_clone_create()...
Thanks,
--
//
Internet Initiative Japan Inc.
Device Engineering Section,
IoT Platform Development Department,
Network Division,
Technology Unit
Kengo NAKAHARA
Re: Are modunload and ifconfig create racy?
(Replying _without_ looking at the code! I can look more deeply if you
could re-send a pointer to the code...)
Generally, the module's unload code should detach the devsw entry for
the driver first, to prevent any new accesses.
Then, the config_fini_component() call will fail if existing devices
cannot be detached. If this does fail, the module unload code should
re-attach the devsw entry and return failure.
The only chance I see for any race condition is if the open/create logic
can clone a new entry which is not seen by config_cfdata_detach() when
it scans for device instances which need to be detached.
If there is a race condition, it should be solved in each interface's
driver module, or within the autoconfig framework. Each driver module
is responsible for managing its own integration with autoconfig (ie,
calling config_{init,fini}_component and devsw_{att},det}ach routines).
On Fri, 20 Jan 2017, Kengo NAKAHARA wrote:
Hi,
As riastradh@n.o pointed out below mail, it seems there is race in
module unload processing about interfaces.
http://mail-index.netbsd.org/tech-net/2017/01/19/msg006250.html
== quote ==
+static int
+l2tpdetach(void)
+{
+ int error;
+
+ if (!LIST_EMPTY(_softc_list))
+ return EBUSY;
Need lock here? Need to first set flag preventing new creation?
mutex_enter(_softc.lock);
KASSERT(!l2tp_softc.dying);
l2tp_softc.detaching = true;
if (!LIST_EMPTY(_softc.list)) {
l2tp_softc.detaching = false;
mutex_exit(_softc.lock);
return EBUSY;
}
mutex_exit(_softc.lock);
Anyone trying to add to l2tp_softc.list must also check
l2tp_softc.detaching before proceeding.
== quote ==
I think this detaching flag exclusion is required to kernel module'd
interfaces such as pppoe(4), gre(4) and so on. Is this correct?
# I am not familiar with kernel module load/unload locking...
If correct, it seems some kind of exclusion is required in kernel module
framework to prevent race between modunload(8) kernel module of such
interface and ifconfig(8) create of such interface.
Could anyone have any ideas about this?
Thanks,
--
//
Internet Initiative Japan Inc.
Device Engineering Section,
IoT Platform Development Department,
Network Division,
Technology Unit
Kengo NAKAHARA
!DSPAM:58820233275462253564689!
+--+--++
| Paul Goyette | PGP Key fingerprint: | E-mail addresses: |
| (Retired)| FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com |
| Kernel Developer | 0786 F758 55DE 53BA 7731 | pgoyette at netbsd.org |
+--+--++
