Page MenuHomeFreeBSD

PF and VIMAGE fixes
AbandonedPublic

Authored by kp on Feb 22 2015, 11:53 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Mar 2, 7:08 AM
Unknown Object (File)
Sat, Mar 2, 7:08 AM
Unknown Object (File)
Thu, Feb 29, 2:07 PM
Unknown Object (File)
Feb 18 2024, 1:40 PM
Unknown Object (File)
Feb 5 2024, 3:41 AM
Unknown Object (File)
Dec 23 2023, 8:25 AM
Unknown Object (File)
Dec 23 2023, 7:42 AM
Unknown Object (File)
Dec 20 2023, 8:06 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
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 reviewers: bz, zec, trociny.
rodrigc added subscribers: Unknown Object (MLST), Unknown Object (MLST).
rodrigc added a subscriber: Unknown Object (MLST).

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.

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!

sys/netpfil/pf/pf_ioctl.c
325 ↗(On Diff #3915)

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?

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 ↗(On Diff #3915)

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.

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.

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

Is there any update on those fixes?

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

Screen Shot 2015-07-19 at 17.48.26.png (832×1 px, 63 KB)

Screen Shot 2015-07-19 at 18.33.39.png (830×1 px, 69 KB)

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.

Screen Shot 2015-07-19 at 17.48.26.png (832×1 px, 63 KB)

Screen Shot 2015-07-19 at 18.33.39.png (830×1 px, 69 KB)

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?

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

@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

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

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.

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

Nikos, could you have a look into PR 205743?

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 #5290)

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

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.