PF and VIMAGE fixes

[kp]

Summary

Please review this. It is mostly the changes from project/pf plus
VNET_(SYSINIT,UNINIT) and pf_(load,unload).

Unloading the module is almost there, it works for GENERIC.
We need to fix the event handlers registration in order to
unload with a VIMAGE kernel.

Test Plan

loading pf after creating VNET jails should work
creating VNET jails and then loading pf should work
unloading the module is ok only for GENERIC at the moment, it needs a fix for VIMAGE

[• rodrigc]

Nikos has posted these patches to improve VIMAGE support in PF. If some of the folks who are experienced with PF and VIMAGE could take a look, that would be really great.

[glebius]

Nikos,

acking that I see the patches. Right now I'm waiting for pf to stablize after
recent patches to fragment handling. Kristof is working on the known problem.
Meanwhile you can finish your patch moving from "almost there" to "there" :)

If you got any questions about pf or FreeBSD kernel interfaces, feel free
to ask me via email.

[nvass-gmx.com]

In D1944#8, @glebius wrote:

Nikos,

acking that I see the patches. Right now I'm waiting for pf to stablize after
recent patches to fragment handling. Kristof is working on the known problem.
Meanwhile you can finish your patch moving from "almost there" to "there" :)

Yes, currently working on it.

If you got any questions about pf or FreeBSD kernel interfaces, feel free
to ask me via email.

Sure, thanks!

[kp]

sys/netpfil/pf/pf_ioctl.c

325

It's not clear to me why this is done here, rather than in pf_unload(). The initialisation is done in pf_load() after all.

3725 ↗

(On Diff #6288)

Don't we still need to do all of this somewhere?

[nvass-gmx.com]

In D1944#11, @kristof wrote:

Don't we still need to do all of this somewhere?

In D1944#11, @kristof wrote:

Don't we still need to do all of this somewhere?

comments inline

sys/netpfil/pf/pf_ioctl.c

325

pf_unload is called before pf_vnet_unit, this is why we do very little things in pf_unload. We need everything until the last vnet is destroyed.

3725 ↗

(On Diff #6288)

The patch includes per-VNET initialization, so this is not need anymore.
pf_vnet_init() handles all per-VNET initialization, including DEFAULT_VNET.

[gnn]

Any update on this?

[robak]

Is there any update on these fixes? I've just happened to bump my 10.1-RELEASE into 10-STABLE and created few VIMAGE based jails. As soon as I stop any of them, and I can reproduce it every time, the host OS crashes. That makes the entire VIMAGE completely unusable...

[glebius]

Recently Nikos has asked questions on kernel debugging. So, I guess, he is working.

[nvass-gmx.com]

Yes, I am trying to fix the issues. It needs more work. I will update when I have a new patch

[nvass-gmx.com]

Hi,

Please review this. It updates the previous patch, with another way to unload pf.

Eventhandlers are also touched, they should behave correctly now.

[julian]

in case I didn't point you at this before...
http://p4web.freebsd.org/@md=d&cd=//depot/projects/vimage/&cdf=//depot/projects/vimage/porting_to_vimage.txt&c=win@//depot/projects/vimage/porting_to_vimage.txt?ac=22

[nvass-gmx.com]

This guilde is definitely invaluable, I have seen it. It would be great if we could move it in the source tree.

Regarding PF it can be further simplified. I think the logic right now is OK but there are many *init* functions for sure and some of these
functions can go away.

Maybe we could do that a step two?

[• rodrigc]

In D1944#46127, @julian wrote:

in case I didn't point you at this before...
http://p4web.freebsd.org/@md=d&cd=//depot/projects/vimage/&cdf=//depot/projects/vimage/porting_to_vimage.txt&c=win@//depot/projects/vimage/porting_to_vimage.txt?ac=22

@julian , I have copied this to:
https://wiki.freebsd.org/VIMAGE/porting-to-vimage

so that this isn't buried in Perforce.

[• rodrigc]

I tested this patch.

# kldload pf
# kldunload pf
kldunload: can't unload file: Device busy

The fact that the pf module cannot be unloaded was one of the
reasons that @glebius used to back out the entire changeset last time
I committed your pf changes. Can you fix this?

I also saw this in dmesg:

CURVNET_SET() recursion in pfi_vnet_initialize() line 130, prev in vnet_register_sysinit()
    0xfffff800056e4100 -> 0xfffff800056e4100
KDB: stack backtrace:
db_trace_self_wrapper() at db_trace_self_wrapper+0x2b/frame 0xfffffe046389a550
pfi_vnet_initialize() at pfi_vnet_initialize+0x21b/frame 0xfffffe046389a590
pf_vnet_init() at pf_vnet_init+0x35/frame 0xfffffe046389a5c0
vnet_register_sysinit() at vnet_register_sysinit+0x13c/frame 0xfffffe046389a600
linker_load_module() at linker_load_module+0xc87/frame 0xfffffe046389a920
kern_kldload() at kern_kldload+0x10e/frame 0xfffffe046389a970
sys_kldload() at sys_kldload+0x5b/frame 0xfffffe046389a9a0
amd64_syscall() at amd64_syscall+0x27f/frame 0xfffffe046389aab0
Xfast_syscall() at Xfast_syscall+0xfb/frame 0xfffffe046389aab0

[nvass-gmx.com]

In D1944#47231, @rodrigc wrote:

I tested this patch.

# kldload pf
# kldunload pf
kldunload: can't unload file: Device busy

The fact that the pf module cannot be unloaded was one of the
reasons that @glebius used to back out the entire changeset last time
I committed your pf changes. Can you fix this?

This is intended behaviour, regadless of VIMAGE. You need to use kldunload -f
to unload it.

I also saw this in dmesg:

CURVNET_SET() recursion in pfi_vnet_initialize() line 130, prev in vnet_register_sysinit()
    0xfffff800056e4100 -> 0xfffff800056e4100
KDB: stack backtrace:
db_trace_self_wrapper() at db_trace_self_wrapper+0x2b/frame 0xfffffe046389a550
pfi_vnet_initialize() at pfi_vnet_initialize+0x21b/frame 0xfffffe046389a590
pf_vnet_init() at pf_vnet_init+0x35/frame 0xfffffe046389a5c0
vnet_register_sysinit() at vnet_register_sysinit+0x13c/frame 0xfffffe046389a600
linker_load_module() at linker_load_module+0xc87/frame 0xfffffe046389a920
kern_kldload() at kern_kldload+0x10e/frame 0xfffffe046389a970
sys_kldload() at sys_kldload+0x5b/frame 0xfffffe046389a9a0
amd64_syscall() at amd64_syscall+0x27f/frame 0xfffffe046389aab0
Xfast_syscall() at Xfast_syscall+0xfb/frame 0xfffffe046389aab0

This should be ok. It is just a warning that we are setting curvnet although
it is already set. Maybe it can be avoided.

[• rodrigc]

I can confirm that:

kldunload -f pf.ko

does work to unload the module. That is good.

I saw this warning message in dmesg:

lock order reversal: (sleepable after non-sleepable)
 1st 0xffffffff823b72e0 pf rulesets (pf rulesets) @ /opt2/branches/head/sys/modules/pf/../../netpfil/pf/pf_ioctl.c:321
 2nd 0xffffffff818f99a0 umadrain (umadrain) @ /opt2/branches/head/sys/vm/uma_core.c:2109
KDB: stack backtrace:
db_trace_self_wrapper() at db_trace_self_wrapper+0x2b/frame 0xfffffe04639fd310
witness_checkorder() at witness_checkorder+0xe26/frame 0xfffffe04639fd3a0
_sx_slock() at _sx_slock+0x76/frame 0xfffffe04639fd3e0
uma_zdestroy() at uma_zdestroy+0x23/frame 0xfffffe04639fd400
pf_normalize_cleanup() at pf_normalize_cleanup+0x26/frame 0xfffffe04639fd420
pf_vnet_uninit() at pf_vnet_uninit+0x6e7/frame 0xfffffe04639fd8c0
vnet_deregister_sysuninit() at vnet_deregister_sysuninit+0x9c/frame 0xfffffe04639fd900
linker_file_unload() at linker_file_unload+0x45e/frame 0xfffffe04639fd950
kern_kldunload() at kern_kldunload+0x12f/frame 0xfffffe04639fd9a0
amd64_syscall() at amd64_syscall+0x27f/frame 0xfffffe04639fdab0
Xfast_syscall() at Xfast_syscall+0xfb/frame 0xfffffe04639fdab0

If possible, it would be good to clean that up.

[• rodrigc]

Looks OK to me. We can hopefully fix some of the LOR's later.

@glebius : can you provide your feedback on this patch?

[glebius]

Thanks a lot, Nikos.

I've fixed the problem of sleeping in UMA on kldunload. It was out the scope of the patch. I also committed the first part of the patch - mutexes initialization.

Nikos, can you please svn update and then arc update, so that updated patch is posted here?

[nvass-gmx.com]

In D1944#47915, @glebius wrote:

Thanks a lot, Nikos.

I've fixed the problem of sleeping in UMA on kldunload. It was out the scope of the patch. I also committed the first part of the patch - mutexes initialization.

Nikos, can you please svn update and then arc update, so that updated patch is posted here?

Sure, I am currently on vacation. I think I will make some time soon to update it.

[nvass-gmx.com]

Updated to today's head branch. Please review

[robak]

Is there any chance to get these changes committed in time for 10.2-RELEASE? It would be great if we could have working VNET/PF before 11.0-R comes out...

[robak]

Is there any update on those fixes?

[robak]

I managed to crash freshly fetched and built 10-S today, with GENERIC+VIMAGE+RACCT while stopping a jail, attaching console screenshots.

[zec]

In D1944#62124, @robak wrote:

I managed to crash freshly fetched and built 10-S today, with GENERIC+VIMAGE+RACCT while stopping a jail, attaching console screenshots.

Those are just warning messages unrelated to the patchset discussed here, i.e. if your kernel really crashed, the attached screenshots do not reveal the culprit.

[robak]

Is there anything else I can do to provide some useful information? I've attached these screenshots only because the machine becomes unresponsive when this happens (and drops active ssh connections). I am happy to execute any test/information gathering suite you may need.

[nvass-gmx.com]

Hi,

could you write an email to me and [e-mail] with
the necessary steps to re-produce the problem?

[• rodrigc]

@glebius: Nikos updated the patch. Can you review it?

[mmoll]

what's the status here?

[• rodrigc]

@mmoll : It would be nice if @glebius could review this patch. He previously committed some patches I committed to FreeBSD which
attempted to fix this problem, so he has an interest in this area.

[• rodrigc]

@glebius : if you have time can you review this? you have expressed interest in PF + VIMAGE fixes in the past.

@bz : do you have time to review this? I understand you are going to be doing some VIMAGE work

[nvass-gmx.com]

Hi from me as well,

just want to say that I am here too and I am willing to work on this
even if i have to do it from scratch;)

Please review:) Nikos

[robak]

Just to add an end-user update, this stuff keeps leaking, even in 10.2-p7, every single time a VIMAGE jail is being stopped.

[robak]

Any news on that review?

[robak]

@bz: you've made some commits to VIMAGE code in past few days, how do they relate to this revision? Any chance you could review it and comment/commit on this?

[mmoll]

Nikos, could you have a look into PR 205743?

[nvass-gmx.com]

Sure, I will take a look

[kp]

sys/netpfil/pf/pf_if.c

130

I don't understand why this is required. Surely if an ifnet lives in V_ifnet (so, lives in the current vnet) ifp->if_vnet is always going to be curvnet?

141

Why add curvnet here?
pfi_attach_ifnet_event doesn't use its argument.

143

Same as above.

151

Same as above.

814 ↗

(On Diff #6288)

I believe this is correct, but should probably include adding an __unused annotation to arg, and removing the 'curvnet' argument from the EVENTHANDLER_REGISTER() call.

[nvass-gmx.com]

Thanks for taking a look Kristof,

I believe all your points are valid. Thing is that all eventhandlers need a more thorough look. They don't work as they should; be it virtualized or not, be it a VIMAGE or GENERIC kernel. I am trying to take a look but I haven't thrown enough time to it.

sys/netpfil/pf/pf_if.c

130

You're correct. It is plain wrong

[bz]

Just cross-referencing bug https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=205743 from here.

[bz]

Can I have you guys have a look at https://reviews.freebsd.org/D6924

Thanks

[kp]

I'm commandeering this so it can be closed, because the patch fro bz@ (D6924) has been included.