[netebpfext] Add deleted filter contexts to zombie list for debugging purposes #4003
Conversation
matthewige
changed the title
matthewige
requested review from
dthaler,
poornagmsft,
Alan-Jowett,
saxena-anurag,
shankarseal,
dv-msft,
gtrevi,
shpalani,
mtfriesen and
rectified95
as code owners
netebpfext/net_ebpf_ext.c
Outdated
| // Global objects used to store filter contexts that are being cleaned up. This is currently only used in debug | ||
| // contexts. | ||
| EX_SPIN_LOCK _net_ebpf_filter_zombie_list_lock = {0}; | ||
| static bool _net_ebpf_filter_zombie_list_initialized = false; |
There was a problem hiding this comment.
nit: do we need this?
There was a problem hiding this comment.
Please correct me if I am wrong - my understanding is that each provider is serialized within calls to itself, but across providers, we have no serialization. Therefore, we could have two providers attaching/detaching at the same time, which could presumably cause issue here, right?
Regarding initialized - is it ever possible to hit the uninitialized codepath and then initialize again after? Now that I am thinking about it, I do not think this flag may be needed.
There was a problem hiding this comment.
yes we would need the lock; my comment was only about the initialized flag.
| net_ebpf_ext_remove_filter_context_from_zombie_list((filter_context)); \ | ||
| ExFreePool((filter_context)); \ |
There was a problem hiding this comment.
I don't think you need the double parens in these lines.
| net_ebpf_ext_remove_filter_context_from_zombie_list((filter_context)); \ | |
| ExFreePool((filter_context)); \ | |
| net_ebpf_ext_remove_filter_context_from_zombie_list(filter_context); \ | |
| ExFreePool(filter_context); \ |
There was a problem hiding this comment.
I have generally followed the practice of using parentheses around the macro parameter to ensure that there are no accidental issues with any sort of operator precedence when the macro is expanded. I don't think it's strictly necessary in this case, but I prefer to keep it in.
This is also staying consistent with the original author's usage in this macro.
matthewige
dismissed stale reviews from shpalani and saxena-anurag
via
8833dc5
Description
We are seeing failures in the cleanup logic, where netebpfext is unable to unload because of a reference to a WFP filter. To help debug this, a zombie list is added, where we should be able to see the list of filter objects that we have not received the WFP notify delete callback for when a dump is captured.
Testing
CICD
Documentation
n/a
Installation
n/a