libbpf-tools: Fix misaligned pointer accesses in exitsnoop #4760
Conversation
The perf buffer in exit snoop doesn't maintain 8 byte alignment for start_time and exit_time in struct event. When building with "-fsanitize=alignment -fsanitize-undefined-trap-on-error" failures happen in handle_event. Fix these by copying the event from the perf buffer before accessing. Signed-off-by: Ian Rogers <[email protected]>
|
Thanks for the patch.
Consider changing the deprecated |
Please include the error message here. IIRC, raw perf events are 64-bit aligned, though it could contain trailing garbage. |
There's no particular error message except SIGILL: I see the BPF ring buffer 8 byte aligning: I see perf events manually aligning themselves to 8 bytes: I don't see alignment in the perf code: |
|
The original code fixed by this patch has UB. FWIW C11 6.3.2.3p7 says
|
|
I can reproduce the issue with gcc or clang. After some investigation, I guess the reason is below. For each RAW record, first is a perf_event_header and then followed by perf record type specific fields. So for sampling type, we have total 12 bytes (8 for 'perf_event_header' and 4 byte for above 'size') before the real data. This is aligning with above core stack as well (esp. perf_buffer__process_record() and handle_event()). So it looks like we not only have this issue for exitsnoop, but for other tools as well (e.g., biosnoop, drsnoop, etc.). |
|
I found that the simplest way to fix the issue is: diff --git a/libbpf-tools/exitsnoop.h b/libbpf-tools/exitsnoop.h
index e42d156f..08b5f595 100644
--- a/libbpf-tools/exitsnoop.h
+++ b/libbpf-tools/exitsnoop.h
@@ -4,7 +4,7 @@
#define TASK_COMM_LEN 16
-struct event {
+struct __attribute__((__packed__)) event {
__u64 start_time;
__u64 exit_time;
__u32 pid; |
|
Thanks @chenhengqi for the suggestion. Indeed, adding 'attribute((packed))' will prevent sanitizer to check alignment so we won't have issue any more. arm64 and x86 should handle misaligned load/store efficiently (almost the same as aligned). Some architecture does not support efficient misalign access (e.g., MIPS, SPARC?) but there should be a hardware interrupt to do the work automatically. @captain5050 could you change the code based on @chenhengqi 's suggestion? |
|
Let us do ensure that each struct member at least 4 byte aligned though. |
|
Thanks for the feedback and particularly the perf sample details! Using attribute packed is less than ideal because:
For these reasons it is better to memcpy, the compiler able to reason the src of the memcpy is unaligned but afterward things will be aligned without a risk of introducing UB.
The change is simple, I'm happy to help. I haven't observed other failures so I may have a challenge to reproduce and check a fix. Given that I'd need to learn each tool to produce a fix, would it be possible for the maintainer of the tool to fix it? Fixing other tools doesn't need to gate this patch. Thanks again! |
|
|
The memcpy is fixed size and treated by compilers as a builtin. It is possible for it to be inefficient but in practice it isn't. Given the problem packed creates it is the opinion of myself and people working on sanitizers that memcpy is preferable. |
|
I did some further checking. Looks bcc does memcpy as well. The below is libbpf sample_fn callback function: The |
|
Sync'ed with @anakryiko Since the data pointer (as below) has type 'void *' which implies alignment of 1 although advanced users might be dig out that actual alignment is 4, application doing memcpy() sounds a good idea to Will merge this patch and other similar cases (biosnoop, drsnoop, etc.) can be handled later. |
|
Oh, I'd handle it in a code a bit more efficiently and nicely. Basically just add this on top and don't change anything else: |
|
@captain5050 could you have a followup with the above code suggested by @anakryiko? This code pattern, which has minimum diff's can be used for other tools in the future. Thanks! |
|
So if I try this in godbolt I find the code is worse with both gcc and clang. When there is an unconditional memcpy both gcc and clang can remove it, whereas the conditional one leads to larger code: https://godbolt.org/z/1sj4o85aa I think I favor the memcpy and changing '->' to '.' , I find the code easier to read with less cognitive load, the compiler does a better job with it. Unfortunately the '->' to '.' makes the change a little large and more invasive. |
|
How can compiler remove memcpy() in foo2() if there is no guarantee that pointer is aligned? It just opts to do unaligned 8-byte reads from memory? I mean, I don't particularly care, I was just trying to help minimize changes. |
The following is an explanation. Note it has 'align 1'. A single load. A lot of loads with bytes. In summary, for x86, since it supports unaligned load with "almost" no performance penalty, it uses one load even for misaligned data. But for BPF, it does not support unaligned load at all so generated code is much more complexity, |
|
Interesting! Thanks, @yonghong-song! |
The perf buffer in biosnoop doesn't maintain 8 byte alignment for delta, qdelta, ts and sector in struct event. When building with "-fsanitize=alignment -fsanitize-trap=undefined" failures happen in handle_event. Fix these by copying the event from the perf buffer before accessing. This is similar to a fix in exitsnoop where different ways to handle misaligned pointers were discussed: iovisor#4760 Signed-off-by: Ian Rogers <[email protected]>
The perf buffer in filelife doesn't maintain 8 byte alignment for delta_ns in struct event. When building with "-fsanitize=alignment -fsanitize-trap=undefined" failures happen in handle_event. Fix these by copying the event from the perf buffer before accessing. This is similar to a fix in exitsnoop where different ways to handle misaligned pointers were discussed: iovisor#4760 Signed-off-by: Ian Rogers <[email protected]>
The perf buffer in fsslower doesn't maintain 8 byte alignment for delta_us, end_ns, offset and size in struct event. When building with "-fsanitize=alignment -fsanitize-trap=undefined" failures happen in handle_event. Fix these by copying the event from the perf buffer before accessing. This is similar to a fix in exitsnoop where different ways to handle misaligned pointers were discussed: iovisor#4760 Signed-off-by: Ian Rogers <[email protected]>
The perf buffer in gethostlatency doesn't maintain 8 byte alignment for time in struct event. When building with "-fsanitize=alignment -fsanitize-trap=undefined" failures happen in handle_event. Fix these by copying the event from the perf buffer before accessing. This is similar to a fix in exitsnoop where different ways to handle misaligned pointers were discussed: iovisor#4760 Signed-off-by: Ian Rogers <[email protected]>
The perf buffer in opensnoop doesn't maintain 8 byte alignment for ts and callers in struct event. When building with " -fsanitize=alignment -fsanitize-trap=undefined" failures happen in handle_event. Fix these by copying the event from the perf buffer before accessing. This is similar to a fix in exitsnoop where different ways to handle misaligned pointers were discussed: iovisor#4760 Signed-off-by: Ian Rogers <[email protected]>
The perf buffer in runqslower doesn't maintain 8 byte alignment for delta_us in struct event. When building with "-fsanitize=alignment -fsanitize-trap=undefined" failures happen in handle_event. Fix these by copying the event from the perf buffer before accessing. This is similar to a fix in exitsnoop where different ways to handle misaligned pointers were discussed: iovisor#4760 Signed-off-by: Ian Rogers <[email protected]>
The perf buffer in statsnoop doesn't maintain 8 byte alignment for ts_ns in struct event. When building with "-fsanitize=alignment -fsanitize-trap=undefined" failures happen in handle_event. Fix these by copying the event from the perf buffer before accessing. This is similar to a fix in exitsnoop where different ways to handle misaligned pointers were discussed: iovisor#4760 Signed-off-by: Ian Rogers <[email protected]>
The perf buffer in tcpconnect doesn't maintain 8 byte alignment for ts_us in struct event. When building with "-fsanitize=alignment -fsanitize-trap=undefined" failures happen in handle_event. Fix these by copying the event from the perf buffer before accessing. This is similar to a fix in exitsnoop where different ways to handle misaligned pointers were discussed: iovisor#4760 Signed-off-by: Ian Rogers <[email protected]>
The perf buffer in tcplife doesn't maintain 8 byte alignment for saddr, daddr, ts_us, span_us, rx_b and tx_b in struct event. When building with "-fsanitize=alignment -fsanitize-trap=undefined" failures happen in handle_event. Fix these by copying the event from the perf buffer before accessing. This is similar to a fix in exitsnoop where different ways to handle misaligned pointers were discussed: iovisor#4760 Signed-off-by: Ian Rogers <[email protected]>
The perf buffer in tcpstates doesn't maintain 8 byte alignment for saddr, daddr, skaddr, ts_us and delta_us in struct event. When building with "-fsanitize=alignment -fsanitize-trap=undefined" failures happen in handle_event. Fix these by copying the event from the perf buffer before accessing. This is similar to a fix in exitsnoop where different ways to handle misaligned pointers were discussed: iovisor#4760 Signed-off-by: Ian Rogers <[email protected]>
The perf buffer in tcptracer doesn't maintain 8 byte alignment for saddr_v4, daddr_v4 and ts_us in struct event. When building with "-fsanitize=alignment -fsanitize-trap=undefined" failures happen in handle_event. Fix these by copying the event from the perf buffer before accessing. This is similar to a fix in exitsnoop where different ways to handle misaligned pointers were discussed: iovisor#4760 Signed-off-by: Ian Rogers <[email protected]>
The perf buffer in drsnoop doesn't maintain 8 byte alignment for delta_ns, nr_reclaimed and nr_free_pages in struct event. When building with "-fsanitize=alignment -fsanitize-trap=undefined" failures happen in handle_event. Fix these by copying the event from the perf buffer before accessing. This is similar to a fix in exitsnoop where different ways to handle misaligned pointers were discussed: iovisor#4760 Signed-off-by: Ian Rogers <[email protected]>
The perf buffer in biosnoop doesn't maintain 8 byte alignment for delta, qdelta, ts and sector in struct event. When building with "-fsanitize=alignment -fsanitize-trap=undefined" failures happen in handle_event. Fix these by copying the event from the perf buffer before accessing. This is similar to a fix in exitsnoop where different ways to handle misaligned pointers were discussed: #4760 Signed-off-by: Ian Rogers <[email protected]>
The perf buffer in filelife doesn't maintain 8 byte alignment for delta_ns in struct event. When building with "-fsanitize=alignment -fsanitize-trap=undefined" failures happen in handle_event. Fix these by copying the event from the perf buffer before accessing. This is similar to a fix in exitsnoop where different ways to handle misaligned pointers were discussed: #4760 Signed-off-by: Ian Rogers <[email protected]>
The perf buffer in fsslower doesn't maintain 8 byte alignment for delta_us, end_ns, offset and size in struct event. When building with "-fsanitize=alignment -fsanitize-trap=undefined" failures happen in handle_event. Fix these by copying the event from the perf buffer before accessing. This is similar to a fix in exitsnoop where different ways to handle misaligned pointers were discussed: #4760 Signed-off-by: Ian Rogers <[email protected]>
The perf buffer in gethostlatency doesn't maintain 8 byte alignment for time in struct event. When building with "-fsanitize=alignment -fsanitize-trap=undefined" failures happen in handle_event. Fix these by copying the event from the perf buffer before accessing. This is similar to a fix in exitsnoop where different ways to handle misaligned pointers were discussed: #4760 Signed-off-by: Ian Rogers <[email protected]>
The perf buffer in opensnoop doesn't maintain 8 byte alignment for ts and callers in struct event. When building with " -fsanitize=alignment -fsanitize-trap=undefined" failures happen in handle_event. Fix these by copying the event from the perf buffer before accessing. This is similar to a fix in exitsnoop where different ways to handle misaligned pointers were discussed: #4760 Signed-off-by: Ian Rogers <[email protected]>
The perf buffer in runqslower doesn't maintain 8 byte alignment for delta_us in struct event. When building with "-fsanitize=alignment -fsanitize-trap=undefined" failures happen in handle_event. Fix these by copying the event from the perf buffer before accessing. This is similar to a fix in exitsnoop where different ways to handle misaligned pointers were discussed: #4760 Signed-off-by: Ian Rogers <[email protected]>
The perf buffer in statsnoop doesn't maintain 8 byte alignment for ts_ns in struct event. When building with "-fsanitize=alignment -fsanitize-trap=undefined" failures happen in handle_event. Fix these by copying the event from the perf buffer before accessing. This is similar to a fix in exitsnoop where different ways to handle misaligned pointers were discussed: #4760 Signed-off-by: Ian Rogers <[email protected]>
The perf buffer in tcpconnect doesn't maintain 8 byte alignment for ts_us in struct event. When building with "-fsanitize=alignment -fsanitize-trap=undefined" failures happen in handle_event. Fix these by copying the event from the perf buffer before accessing. This is similar to a fix in exitsnoop where different ways to handle misaligned pointers were discussed: #4760 Signed-off-by: Ian Rogers <[email protected]>
The perf buffer in tcplife doesn't maintain 8 byte alignment for saddr, daddr, ts_us, span_us, rx_b and tx_b in struct event. When building with "-fsanitize=alignment -fsanitize-trap=undefined" failures happen in handle_event. Fix these by copying the event from the perf buffer before accessing. This is similar to a fix in exitsnoop where different ways to handle misaligned pointers were discussed: #4760 Signed-off-by: Ian Rogers <[email protected]>
The perf buffer in tcpstates doesn't maintain 8 byte alignment for saddr, daddr, skaddr, ts_us and delta_us in struct event. When building with "-fsanitize=alignment -fsanitize-trap=undefined" failures happen in handle_event. Fix these by copying the event from the perf buffer before accessing. This is similar to a fix in exitsnoop where different ways to handle misaligned pointers were discussed: #4760 Signed-off-by: Ian Rogers <[email protected]>
The perf buffer in tcptracer doesn't maintain 8 byte alignment for saddr_v4, daddr_v4 and ts_us in struct event. When building with "-fsanitize=alignment -fsanitize-trap=undefined" failures happen in handle_event. Fix these by copying the event from the perf buffer before accessing. This is similar to a fix in exitsnoop where different ways to handle misaligned pointers were discussed: #4760 Signed-off-by: Ian Rogers <[email protected]>
The perf buffer in drsnoop doesn't maintain 8 byte alignment for delta_ns, nr_reclaimed and nr_free_pages in struct event. When building with "-fsanitize=alignment -fsanitize-trap=undefined" failures happen in handle_event. Fix these by copying the event from the perf buffer before accessing. This is similar to a fix in exitsnoop where different ways to handle misaligned pointers were discussed: #4760 Signed-off-by: Ian Rogers <[email protected]>
The perf buffer in exit snoop doesn't maintain 8 byte alignment for start_time and exit_time in struct event. When building with "-fsanitize=alignment -fsanitize-undefined-trap-on-error" failures happen in handle_event. Fix these by copying the event from the perf buffer before accessing.