Don't cleanup prematurely #619
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #619 +/- ##
===========================================
- Coverage 79.41% 44.57% -34.84%
===========================================
Files 70 68 -2
Lines 5989 5817 -172
===========================================
- Hits 4756 2593 -2163
- Misses 1005 3071 +2066
+ Partials 228 153 -75
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| @@ -13,15 +13,15 @@ char __license[] SEC("license") = "Dual MIT/GPL"; | |||
|
|
|||
| // Temporary tracking of accept arguments | |||
| struct { | |||
| __uint(type, BPF_MAP_TYPE_HASH); | |||
| __uint(type, BPF_MAP_TYPE_LRU_HASH); | |||
There was a problem hiding this comment.
I changed some of these because kretprobes may not fire, so we can't be sure we'll clean-up these maps.
| @@ -550,6 +553,7 @@ int uprobe_hpack_Encoder_WriteField(struct pt_regs *ctx) { | |||
|
|
|||
| if (val_len <= 0) { | |||
| bpf_dbg_printk("Encoded traceparent value too large or empty"); | |||
| bpf_map_delete_elem(&ongoing_grpc_header_writes, &goroutine_addr); | |||
There was a problem hiding this comment.
This was simply missed cleanup
| @@ -203,7 +203,7 @@ int uprobe_WriteHeader(struct pt_regs *ctx) { | |||
|
|
|||
| #ifndef NO_HEADER_PROPAGATION | |||
| struct { | |||
| __uint(type, BPF_MAP_TYPE_HASH); | |||
| __uint(type, BPF_MAP_TYPE_LRU_HASH); | |||
There was a problem hiding this comment.
No guaratee this will be cleaned up, making an LRU.
| @@ -379,6 +380,8 @@ int uprobe_writeSubset(struct pt_regs *ctx) { | |||
| bpf_probe_write_user((void *)(io_writer_addr + io_writer_n_pos), &len, sizeof(len)); | |||
| } | |||
|
|
|||
| done: | |||
| bpf_map_delete_elem(&header_req_map, &header_addr); | |||
There was a problem hiding this comment.
Another missed cleanup
Yeah, it's sort of common in eBPF. Each program is always individual single function and when we use functions we'd have to put __always_inline so that the verifier likes it. The only way to call in terms of a function call is to use bpf_tail_call. So I think you are right, we could've made the cleanup a function and "call it", but then we'd have to make individual cleanup function for each probe, I thought this was simpler, but maybe I'm not considering some approach. If you have a cleaner code in mind, please shout :). |
I hope it will. I'll ask on the issue if they can try with our main branch now once this is built. |
|
Hi Nikola,
Is this a limitation of the way |
|
Oh maybe it's not needed anymore, I just keep using it by habbit. |
Fixing number of places where we used the unsafe delete after use pattern.