VIMAGE crashes on 9.x with hotplug net80211 devices

[Adrian Chadd]

Hi all,

I have some crashes in the VIMAGE code on releng_9. Specifically, when
I enable VIMAGE and then hotplug some cardbus ath(4) NICs.

The panics are dereferencing the V_ ifindex and related fields.

If I start adding CURVNET_SET(vnet0) and CURVNET_RESTORE() around the
ifnet calls (attach, detach) then things stop panicing - however,
things are slightly more complicated than that.

Since it's possible that the cloned interfaces (and maybe the parent
interface?) are placed into other VNETs, I have to make sure that the
right vnet context is switched to before I free interfaces.

So, may I please have some help by some VIMAGE-cluey people to sort
out how to _properly_ get VIMAGE up on net80211? I'd like to fix this
in -HEAD and -9 so people are able to use VIMAGEs for hostapd
interfaces (and so I can abuse it for lots of local testing on a
single laptop.)

Thanks!



Adrian
_______________________________________________
freebsd...@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to "freebsd-hacke...@freebsd.org"

[Marko Zec]

On Sunday 21 October 2012 21:04:41 Adrian Chadd wrote:
> Hi all,
>
> I have some crashes in the VIMAGE code on releng_9. Specifically, when
> I enable VIMAGE and then hotplug some cardbus ath(4) NICs.
>
> The panics are dereferencing the V_ ifindex and related fields.
>
> If I start adding CURVNET_SET(vnet0) and CURVNET_RESTORE() around the
> ifnet calls (attach, detach) then things stop panicing - however,
> things are slightly more complicated than that.
>
> Since it's possible that the cloned interfaces (and maybe the parent
> interface?) are placed into other VNETs, I have to make sure that the
> right vnet context is switched to before I free interfaces.
>
> So, may I please have some help by some VIMAGE-cluey people to sort
> out how to _properly_ get VIMAGE up on net80211? I'd like to fix this
> in -HEAD and -9 so people are able to use VIMAGEs for hostapd
> interfaces (and so I can abuse it for lots of local testing on a
> single laptop.)

The right approach would be to do a single CURVNET_SET(vnet0) /
CURVNET_RESTORE() somewhere near the root of the call graph being triggered
by the hotplug attach event. Not having any hotpluggable hardware at hand
I cannot be more specific where that place could be...

But most certainly doing CURVNET_SET(vnet0) on detach events would be wrong:
since ifnets may be assignet to non-default vnets,
CURVNET_SET(ifp->if_vnet) should be more appropriate there.

Another thing that may help could be turning on options VNET_DEBUG when, as
that should reveal excessive (and probably redundant) CURVNET_SET()
recursions.

Hope this helps,

Marko

[Adrian Chadd]

On 21 October 2012 12:36, Marko Zec <z...@fer.hr> wrote:

> The right approach would be to do a single CURVNET_SET(vnet0) /
> CURVNET_RESTORE() somewhere near the root of the call graph being triggered
> by the hotplug attach event. Not having any hotpluggable hardware at hand
> I cannot be more specific where that place could be...

Right; would that be at the net80211 side, or something higher up (eg
at device_attach, which gets called from the cardbus/pci bridge
enumeration code.)

> But most certainly doing CURVNET_SET(vnet0) on detach events would be wrong:
> since ifnets may be assignet to non-default vnets,
> CURVNET_SET(ifp->if_vnet) should be more appropriate there.

Thanks for that. I'll look at adding that in my next debug pass.

> Another thing that may help could be turning on options VNET_DEBUG when, as
> that should reveal excessive (and probably redundant) CURVNET_SET()
> recursions.

I've spotted a couple, however the crashing here is the important bit. :-)

So - why is it that the V_* variables are NULL pointers at this stage?
I thought the kernel would've been running with a default vnet context
of vnet0? Why doesn't this impact other network device hotplugging? Or
does it, and noone noticed?



Adrian

[Marko Zec]

On Sunday 21 October 2012 21:50:21 Adrian Chadd wrote:
> On 21 October 2012 12:36, Marko Zec <z...@fer.hr> wrote:
> > The right approach would be to do a single CURVNET_SET(vnet0) /
> > CURVNET_RESTORE() somewhere near the root of the call graph being
> > triggered by the hotplug attach event. Not having any hotpluggable
> > hardware at hand I cannot be more specific where that place could be...
>
> Right; would that be at the net80211 side, or something higher up (eg
> at device_attach, which gets called from the cardbus/pci bridge
> enumeration code.)

As high as it gets - if you get lucky, as a side effect you might even fix
similar issues with USB hotplugging.

> > But most certainly doing CURVNET_SET(vnet0) on detach events would be
> > wrong: since ifnets may be assignet to non-default vnets,
> > CURVNET_SET(ifp->if_vnet) should be more appropriate there.
>
> Thanks for that. I'll look at adding that in my next debug pass.
>
> > Another thing that may help could be turning on options VNET_DEBUG
> > when, as that should reveal excessive (and probably redundant)
> > CURVNET_SET() recursions.
>
> I've spotted a couple, however the crashing here is the important bit.
> :-)
>
> So - why is it that the V_* variables are NULL pointers at this stage?
> I thought the kernel would've been running with a default vnet context
> of vnet0? Why doesn't this impact other network device hotplugging? Or
> does it, and noone noticed?

By design, the kernel is never running "by default" in any of the vnets
(vnet0 included). If it were, it would be extremely difficult to spot and
catch many cases where a subsystem would be (implicitly) working with
vnet0, while in fact it should be working in a different vnet context.

Obviously, handling device attach events is an exception from this rule, and
up to this date this was never properly addressed...

Marko

[Adrian Chadd]

On 21 October 2012 14:22, Marko Zec <z...@fer.hr> wrote:

>> Right; would that be at the net80211 side, or something higher up (eg
>> at device_attach, which gets called from the cardbus/pci bridge
>> enumeration code.)
>
> As high as it gets - if you get lucky, as a side effect you might even fix
> similar issues with USB hotplugging.

Right. There's three problems:

* the device attach (ath_attach, calls if_* calls)
* the net80211 device attach;
* the net80211 vap attach.

There's also detaching all of the above too.

>> So - why is it that the V_* variables are NULL pointers at this stage?
>> I thought the kernel would've been running with a default vnet context
>> of vnet0? Why doesn't this impact other network device hotplugging? Or
>> does it, and noone noticed?
>
> By design, the kernel is never running "by default" in any of the vnets
> (vnet0 included). If it were, it would be extremely difficult to spot and
> catch many cases where a subsystem would be (implicitly) working with
> vnet0, while in fact it should be working in a different vnet context.

Right. Well, that's why it's panicing then.

> Obviously, handling device attach events is an exception from this rule, and
> up to this date this was never properly addressed...

*laugh*.

The problem now is figuring out how to do it without modifying all the drivers.

The attach is easy - I can likely set it up during the device_attach()
pass. I can do that, but it's enforcing "networking-ness" with the
device attach, which will be called for networking and non-networking
devices alike.

However detach isn't easy - because I'm required to call
CURVNET_SET(ifp->if_vnet) and CURVNET_RESTORE() around if_free(), and
if_free() is called in the device specific detach() routine, I can't
easily set the current VNET context from outside the driver.

I _guess_ I could device_attach() to use CURVNET_SET(vnet0) but
device_detach() can't do the same - it doesn't "know" about the
networking-ness of the device.

I'm open to other suggestions.

(how the hell does this work for devices attached at probe time? What
vnet context do they have, and why doesn't the kernel panic there?)



Adrian

[Marko Zec]

On Monday 22 October 2012 01:03:19 Adrian Chadd wrote:
..

> > Obviously, handling device attach events is an exception from this
> > rule, and up to this date this was never properly addressed...
>
> *laugh*.
>
> The problem now is figuring out how to do it without modifying all the
> drivers.
>
> The attach is easy - I can likely set it up during the device_attach()
> pass. I can do that, but it's enforcing "networking-ness" with the
> device attach, which will be called for networking and non-networking
> devices alike.
>
> However detach isn't easy - because I'm required to call
> CURVNET_SET(ifp->if_vnet) and CURVNET_RESTORE() around if_free(), and
> if_free() is called in the device specific detach() routine, I can't
> easily set the current VNET context from outside the driver.
>
> I _guess_ I could device_attach() to use CURVNET_SET(vnet0) but
> device_detach() can't do the same - it doesn't "know" about the
> networking-ness of the device.
>
> I'm open to other suggestions.

The only option I can think of now is to update all of the hotunpluggable
device_detach() handlers to do CURVNET_SET(ifp->if_vnet) before calling
further down into the networking stack, because as you already observed,
whatever triggers a device_detach() handler is not aware of the nature of
the driver.

> (how the hell does this work for devices attached at probe time? What
> vnet context do they have, and why doesn't the kernel panic there?)

Because at boot / autoconfiguration time curvnet is implicitly set to vnet0
between SI_SUB_VNET and SI_SUB_VNET_DONE (i.e. before going SMP).

Similarly, curvnet is set to vnet0 during kldload events.

Marko

[Adrian Chadd]

On 22 October 2012 03:08, Marko Zec <z...@fer.hr> wrote:

> The only option I can think of now is to update all of the hotunpluggable
> device_detach() handlers to do CURVNET_SET(ifp->if_vnet) before calling
> further down into the networking stack, because as you already observed,
> whatever triggers a device_detach() handler is not aware of the nature of
> the driver.

Right. Well, since most things are in theory hotpluggable these days
(or soon will be, with pcie hotplug), I think we need a slightly more
generic solution.

>> (how the hell does this work for devices attached at probe time? What
>> vnet context do they have, and why doesn't the kernel panic there?)
>
> Because at boot / autoconfiguration time curvnet is implicitly set to vnet0
> between SI_SUB_VNET and SI_SUB_VNET_DONE (i.e. before going SMP).
>
> Similarly, curvnet is set to vnet0 during kldload events.

. like this.

The trouble is going to be handling unplug and kldunload events too.
Does curvnet -> vnet0 during kldunload events?

Thanks,



Adrian

[Julian Elischer]

On 10/22/12 7:12 AM, Adrian Chadd wrote:
> On 22 October 2012 03:08, Marko Zec <z...@fer.hr> wrote:
>
>> The only option I can think of now is to update all of the hotunpluggable
>> device_detach() handlers to do CURVNET_SET(ifp->if_vnet) before calling
>> further down into the networking stack, because as you already observed,
>> whatever triggers a device_detach() handler is not aware of the nature of
>> the driver.
> Right. Well, since most things are in theory hotpluggable these days
> (or soon will be, with pcie hotplug), I think we need a slightly more
> generic solution.
>
>>> (how the hell does this work for devices attached at probe time? What
>>> vnet context do they have, and why doesn't the kernel panic there?)
>> Because at boot / autoconfiguration time curvnet is implicitly set to vnet0
>> between SI_SUB_VNET and SI_SUB_VNET_DONE (i.e. before going SMP).
>>
>> Similarly, curvnet is set to vnet0 during kldload events.

> .. like this.

>
> The trouble is going to be handling unplug and kldunload events too.
> Does curvnet -> vnet0 during kldunload events?

I think in unload events we probably need to cycle through all vnets and
do individual shutdowns of anything that is set up on that vnet..
(but I'm not reading the code to say that, it's possible to ignore me
safely)

[Adrian Chadd]

On 22 October 2012 10:29, Julian Elischer <jul...@freebsd.org> wrote:

>> The trouble is going to be handling unplug and kldunload events too.
>> Does curvnet -> vnet0 during kldunload events?
>
> I think in unload events we probably need to cycle through all vnets and
> do individual shutdowns of anything that is set up on that vnet..
> (but I'm not reading the code to say that, it's possible to ignore me
> safely)

Well, in an unload event you know the device you're unloading.
However, there may be clones and such involved. It's not like a
kldunload will kill a specific VAP on an ath(4) interface, it'll kill
the whole interface with all vaps.

So in net80211 I need to teach the VAP setup/destroy path to use
CURVNET_*() correctly. That's a given.

I still however need to ensure that
CURVNET_SET(vnet0)/CURVNET_RESTORE() is used around the device
attach/detach, as right now the hotplug code doesn't do this.

So Marko:

* Given that you've "fixed" the kldload path and bootup path to set
CURVNET_SET(vnet0) as a special case, how about we teach the
device_attach() path to just do this in general?

* How does kldunload work right now if any devices are in a vnet? If I
kldunload if_bridge with vnets everywhere, what happens? if_bridge
doesn't at all know anything about VIMAGE. How do the cloned
interfaces get correctly destroyed?

I don't want to have to teach _every network device_ that they need to
be vnet aware on attach or detach.

* the device probe/attach path should just use vnet0; and
* the device detach/destroy path, to things like if_free(), should
have those functions just use ifp->if_vnet, rather than assuming
CURVNET_SET() was called.

I know you wanted to be warned if parts of the stack weren't correctly
using CURVNET_SET()/CURVNET_RESTORE(), but I think this battle is
already lost. :/

[Marko Zec]

On Monday 22 October 2012 19:41:19 Adrian Chadd wrote:
> On 22 October 2012 10:29, Julian Elischer <jul...@freebsd.org> wrote:
> >> The trouble is going to be handling unplug and kldunload events too.
> >> Does curvnet -> vnet0 during kldunload events?
> >
> > I think in unload events we probably need to cycle through all vnets
> > and do individual shutdowns of anything that is set up on that vnet..
> > (but I'm not reading the code to say that, it's possible to ignore me
> > safely)
>
> Well, in an unload event you know the device you're unloading.
> However, there may be clones and such involved. It's not like a
> kldunload will kill a specific VAP on an ath(4) interface, it'll kill
> the whole interface with all vaps.
>
> So in net80211 I need to teach the VAP setup/destroy path to use
> CURVNET_*() correctly. That's a given.
>
> I still however need to ensure that
> CURVNET_SET(vnet0)/CURVNET_RESTORE() is used around the device
> attach/detach, as right now the hotplug code doesn't do this.
>
> So Marko:
>
> * Given that you've "fixed" the kldload path and bootup path to set
> CURVNET_SET(vnet0) as a special case, how about we teach the
> device_attach() path to just do this in general?

While it's true that the kldunload path (most probably) does
CURVNET_SET(vnet0), this is obviously just a kludge which works on pure
luck, i.e. only when ifnets to be detached live inside vnet0.

> * How does kldunload work right now if any devices are in a vnet?

It (most probably) doesn't.

> If I
> kldunload if_bridge with vnets everywhere, what happens? if_bridge
> doesn't at all know anything about VIMAGE. How do the cloned
> interfaces get correctly destroyed?

Haven't tried this out recently, really, though bz@ maintained a patch for a
while which specifically targetted VNET issues with cloner ifnets, but I
don't know the current status of that work...

> I don't want to have to teach _every network device_ that they need to
> be vnet aware on attach or detach.
>
> * the device probe/attach path should just use vnet0; and

Right.

> * the device detach/destroy path, to things like if_free(), should
> have those functions just use ifp->if_vnet, rather than assuming
> CURVNET_SET() was called.

How many functions like if_free() are we talking about here? If only a few
would need to be extended to do a CURVNET_SET(ifp->if_vnet), that doesn't
sound like too big an issue, though I'm not completely convinced that such
an approach could guarantee that every driver would survive hotunplugging
with vnets. Still, that would be an improvement over what we have right
now.

> I know you wanted to be warned if parts of the stack weren't correctly
> using CURVNET_SET()/CURVNET_RESTORE(), but I think this battle is
> already lost. :/

It is absolutely critical that, at minimum, we always completely unwind the
VNET stack when exiting the networking code, otherwise we risk to continue
running with a fully random implicit curvnet context. As many of the
networking subsystems or code paths are still not VNET-friendly, entering
any of those on a VIMAGE kernel should lead to panics, not to obscure and
silent inter-vnet leakages which may become a nightmare to nail down.

OTOH, avoiding excessive recursions on curvnet remains an effort similar to
our style(9) - if you don't stick to it to the letter, things will still
work, but some code paths may become more difficult to debug when things go
wrong... Plus, keep in mind that every CURVNET_SET() consumes a few CPU
cycles here and there, and requires a few extra bytes on the stack...

Marko

[Adrian Chadd]

Hi,

I don't mind tackling the net80211 clone detach path.

I do mind how the default for hotplug is "argh, it doesn't work." :-)

So I'd like to come up with something to fix the basic device detach,
rather than having to actually add CURVNET_*() calls around each
if_free() in each device detach method.



Adrian

[Marko Zec]

On Monday 22 October 2012 23:43:11 Adrian Chadd wrote:
> Hi,
>
> I don't mind tackling the net80211 clone detach path.
>
> I do mind how the default for hotplug is "argh, it doesn't work." :-)
>
> So I'd like to come up with something to fix the basic device detach,
> rather than having to actually add CURVNET_*() calls around each
> if_free() in each device detach method.

As already mentioned earlier, I don't terribly object if you'd place
CURVNET_SET(ifp->if_vnet) inside if_free() and a limited number of similar
functions, but I don't quite believe this is will enough to solve the
device_detach() issue without having to touch any of the drivers...

Marko

[Adrian Chadd]

On 23 October 2012 00:16, Marko Zec <z...@fer.hr> wrote:

> As already mentioned earlier, I don't terribly object if you'd place
> CURVNET_SET(ifp->if_vnet) inside if_free() and a limited number of similar
> functions, but I don't quite believe this is will enough to solve the
> device_detach() issue without having to touch any of the drivers...

That's why I'm asking for more/better ideas.

So far my ideas are:

* for hotplug insert - do the same as what you're doing during the
kldload and boot device enumeration pass - call CURVNET_SET(vnet0)
* for device unload (hotplug or otherwise) - if vnet isn't set,
implicitly set it to vnet0
* for the net80211 vaps, they get destroyed in a few places (ioctl
path, device detach path, I'm sure I saw one more) so I have to use
CURVNET_SET(ifp->if_vnet) on those.

Now, that _should_ fix it for ath(4) and net80211, and it should fix
it for all the other non-USB wireless devices out there.

Now as for USB - Hans, what do you think? Should we do something
similar? How does VIMAGE work right now with USB wireless and USB
ethernet devices?

Marko - thanks for persisting with this. I'd like to try and make this
work for 10.0.



Adrian

[Adrian Chadd]

ping?

Marko - would you be willing to add the if_free() vnet context setup into -HEAD?

Hans, what do you think about USB device attach? detach will be
covered by the above (I hope!) but we still need to do a
CURVNET_SET(vnet0); during hotplug attach.

Thanks,


Adrian

[Hans Petter Selasky]

Hi,

I currently have not tested VIMAGE with USB devices.

Detach is the final exit for a USB device.

There is also shutdown, but softc still is around.

--HPS

[Marko Zec]

On Sunday 28 October 2012 19:47:20 Adrian Chadd wrote:

> ping?
>
> Marko - would you be willing to add the if_free() vnet context setup into
> -HEAD?

Feel free to do it - though I'd suggest to use the CURVNET_SET_QUIET()
variant there, to reduce the console spam with VNET_DEBUG.

Marko


Index: if.c
===================================================================
--- if.c (revision 242304)
+++ if.c (working copy)
@@ -513,7 +513,9 @@

if (!refcount_release(&ifp->if_refcount))
return;
+ CURVNET_SET_QUIET(ifp->if_vnet);
if_free_internal(ifp);
+ CURVNET_RESTORE();
}

/*

[Marko Zec]

On Thursday 15 November 2012 07:18:31 Adrian Chadd wrote:
> Hi,
>
> Here's what I have thus far. Please ignore the device_printf() change.
>
> This works for me, both for hotplug cardbus wireless devices as well
> as (inadvertently!) a USB bluetooth device.
>
> What do you think?

It looks that you've hit the right spot to set curvnet context in
device_probe_and_attach().

Could you try out a slightly revised verstion (attached) - this one also
removes now redundant curvnet setting from linker routines (kldload /
kldunload), and adds a few extra bits which might be necessary for a
broader range of drivers to work.

Note that I haven't tested this myself as I don't have a -CURRENT machine
ATM, but a similar patch for 8.3 apparently works fine, though I don't have
hotplugabble network cards to play with (neither cardbus nor USB)...

Cheers,

Marko

[Adrian Chadd]

Hans brings up a very good point for USB - they split if_alloc and
if_attach across two different threads.

So this works for non-USB devices, but not for USB devices.

Hans, does each device implement its own workqueue for this kind of
delayed action, or is there some generic work queue that is doing this
work?



Adrian

[Hans Petter Selasky]

On Thursday 15 November 2012 20:16:12 Adrian Chadd wrote:
> Hans brings up a very good point for USB - they split if_alloc and
> if_attach across two different threads.
>
> So this works for non-USB devices, but not for USB devices.
>
> Hans, does each device implement its own workqueue for this kind of
> delayed action, or is there some generic work queue that is doing this
> work?
>
>

Hi,

I think a new thread is created for this stuff. It is inside the USB
subsystem, but would consider this a big *hack* to add VNET specific stuff in
there.

Isn't it possible to have curvnet return "vnet0" when nothing else is set?

--HPS

[Marko Zec]

On Thursday 15 November 2012 20:32:06 Hans Petter Selasky wrote:
> On Thursday 15 November 2012 20:16:12 Adrian Chadd wrote:
> > Hans brings up a very good point for USB - they split if_alloc and
> > if_attach across two different threads.

Fine, so maybe one of the following options could work:

1) pass the vnet context embedded in some other already available struct
when forwarding request from 1st to 2nd thread; or

2) if we can safely assume that device attach events can only occur in
context of vnet0 (and I think we can), place a few CURVNET_SET(vnet0)
macros wherever necessary in 2nd USB "attach" thread.

> > So this works for non-USB devices, but not for USB devices.

Could you post a sample backtrace for me to look at?

> > Hans, does each device implement its own workqueue for this kind of
> > delayed action, or is there some generic work queue that is doing this
> > work?
>
> Hi,
>
> I think a new thread is created for this stuff. It is inside the USB
> subsystem, but would consider this a big *hack* to add VNET specific
> stuff in there.
>
> Isn't it possible to have curvnet return "vnet0" when nothing else is
> set?

No! This was discussed already at several ocassions, including earlier in
this thread: with curvnet pointing by default to vnet0, it would be
essentially impossible to detect, trace and debug leakages between vnets.

Marko