Page MenuHomeFreeBSD

PF and VIMAGE fixes
AbandonedPublic

Authored by kp on Feb 22 2015, 11:53 PM.

Details

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

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

nvass-gmx.com retitled this revision from to PF and VIMAGE fixes.
nvass-gmx.com updated this object.
nvass-gmx.com edited the test plan for this revision. (Show Details)
nvass-gmx.com added reviewers: glebius, rodrigc.
rodrigc added a reviewer: gnn.Feb 23 2015, 6:54 AM
rodrigc added reviewers: bz, zec, trociny.
rodrigc added subscribers: Unknown Object (MLST), Unknown Object (MLST).
rodrigc added a subscriber: Unknown Object (MLST).
rodrigc edited edge metadata.Feb 23 2015, 6:56 AM

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 edited edge metadata.Feb 27 2015, 7:27 PM

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.

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!

rodrigc added a reviewer: kp.Mar 26 2015, 3:33 PM
kp added inline comments.Mar 26 2015, 9:24 PM
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

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

nvass-gmx.com added a comment.EditedApr 1 2015, 1:24 AM
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

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

gnn edited edge metadata.Apr 14 2015, 2:30 PM

Any update on this?

robak added a subscriber: robak.Apr 24 2015, 9:05 PM

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...

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

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

nvass-gmx.com edited edge metadata.

Hi,

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

Eventhandlers are also touched, they should behave correctly now.

nvass-gmx.com added a comment.EditedMay 11 2015, 10:48 AM

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?

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

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.

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 accepted this revision.May 18 2015, 4:35 PM
rodrigc edited edge metadata.

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

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

This revision is now accepted and ready to land.May 18 2015, 4:35 PM
glebius requested changes to this revision.May 19 2015, 2:33 PM
glebius edited edge metadata.

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?

This revision now requires changes to proceed.May 19 2015, 2:33 PM

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 edited edge metadata.

Updated to today's head branch. Please review

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 added a comment.Jul 16 2015, 6:35 PM

Is there any update on those fixes?

robak added a comment.Jul 19 2015, 5:41 PM

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

zec edited edge metadata.Jul 19 2015, 11:06 PM
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.

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.

Hi,

could you write an email to me and freebsd-virtualization@freebsd.org with
the necessary steps to re-produce the problem?

eri added a reviewer: eri.Jul 21 2015, 3:52 PM

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

mmoll added a subscriber: mmoll.Nov 1 2015, 9:22 PM

what's the status here?

@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.

@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 added a comment.EditedDec 3 2015, 3:14 PM

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 added a comment.Dec 3 2015, 3:16 PM

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 added a comment.Dec 23 2015, 9:47 AM

Any news on that review?

robak added a comment.Dec 25 2015, 1:46 PM

@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 added a comment.Dec 31 2015, 9:01 PM

Nikos, could you have a look into PR 205743?

Sure, I will take a look

kp added inline comments.Apr 24 2016, 11:33 AM
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

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.

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 added a comment.Jun 22 2016, 1:00 PM

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

Thanks

kp commandeered this revision.Aug 16 2016, 9:26 AM
kp edited reviewers, added: nvass-gmx.com; removed: kp.

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

kp abandoned this revision.Aug 16 2016, 9:26 AM