Re: [PATCH bpf-next v5 0/6] Add a snprintf eBPF helper
On Mon, Apr 19, 2021 at 9:34 PM Andrii Nakryiko wrote: > > On Mon, Apr 19, 2021 at 8:52 AM Florent Revest wrote: > > > > We have a usecase where we want to audit symbol names (if available) in > > callback registration hooks. (ex: fentry/nf_register_net_hook) > > > > A few months back, I proposed a bpf_kallsyms_lookup series but it was > > decided in the reviews that a more generic helper, bpf_snprintf, would > > be more useful. > > > > This series implements the helper according to the feedback received in > > https://lore.kernel.org/bpf/20201126165748.1748417-1-rev...@google.com/T/#u > > > > - A new arg type guarantees the NULL-termination of string arguments and > > lets us pass format strings in only one arg > > - A new helper is implemented using that guarantee. Because the format > > string is known at verification time, the format string validation is > > done by the verifier > > - To implement a series of tests for bpf_snprintf, the logic for > > marshalling variadic args in a fixed-size array is reworked as per: > > https://lore.kernel.org/bpf/20210310015455.1095207-1-rev...@chromium.org/T/#u > > > > --- > > Changes in v5: > > - Fixed the bpf_printf_buf_used counter logic in try_get_fmt_tmp_buf > > - Added a couple of extra incorrect specifiers tests > > - Call test_snprintf_single__destroy unconditionally > > - Fixed a C++-style comment > > > > --- > > Changes in v4: > > - Moved bpf_snprintf, bpf_printf_prepare and bpf_printf_cleanup to > > kernel/bpf/helpers.c so that they get built without CONFIG_BPF_EVENTS > > - Added negative test cases (various invalid format strings) > > - Renamed put_fmt_tmp_buf() as bpf_printf_cleanup() > > - Fixed a mistake that caused temporary buffers to be unconditionally > > freed in bpf_printf_prepare > > - Fixed a mistake that caused missing 0 character to be ignored > > - Fixed a warning about integer to pointer conversion > > - Misc cleanups > > > > --- > > Changes in v3: > > - Simplified temporary buffer acquisition with try_get_fmt_tmp_buf() > > - Made zero-termination check more consistent > > - Allowed NULL output_buffer > > - Simplified the BPF_CAST_FMT_ARG macro > > - Three new test cases: number padding, simple string with no arg and > > string length extraction only with a NULL output buffer > > - Clarified helper's description for edge cases (eg: str_size == 0) > > - Lots of cosmetic changes > > > > --- > > Changes in v2: > > - Extracted the format validation/argument sanitization in a generic way > > for all printf-like helpers. > > - bpf_snprintf's str_size can now be 0 > > - bpf_snprintf is now exposed to all BPF program types > > - We now preempt_disable when using a per-cpu temporary buffer > > - Addressed a few cosmetic changes > > > > Florent Revest (6): > > bpf: Factorize bpf_trace_printk and bpf_seq_printf > > bpf: Add a ARG_PTR_TO_CONST_STR argument type > > bpf: Add a bpf_snprintf helper > > libbpf: Initialize the bpf_seq_printf parameters array field by field > > libbpf: Introduce a BPF_SNPRINTF helper macro > > selftests/bpf: Add a series of tests for bpf_snprintf > > > > include/linux/bpf.h | 22 ++ > > include/uapi/linux/bpf.h | 28 ++ > > kernel/bpf/helpers.c | 306 ++ > > kernel/bpf/verifier.c | 82 > > kernel/trace/bpf_trace.c | 373 ++ > > tools/include/uapi/linux/bpf.h| 28 ++ > > tools/lib/bpf/bpf_tracing.h | 58 ++- > > .../selftests/bpf/prog_tests/snprintf.c | 125 ++ > > .../selftests/bpf/progs/test_snprintf.c | 73 > > .../bpf/progs/test_snprintf_single.c | 20 + > > 10 files changed, 770 insertions(+), 345 deletions(-) > > create mode 100644 tools/testing/selftests/bpf/prog_tests/snprintf.c > > create mode 100644 tools/testing/selftests/bpf/progs/test_snprintf.c > > create mode 100644 tools/testing/selftests/bpf/progs/test_snprintf_single.c > > > > -- > > 2.31.1.368.gbe11c130af-goog > > > > Looks great, thank you! > > For the series: > > Acked-by: Andrii Nakryiko Thank you for the all the fast and in-depth reviews Andrii! :)
Re: [PATCH bpf-next v5 0/6] Add a snprintf eBPF helper
On Mon, Apr 19, 2021 at 8:52 AM Florent Revest wrote: > > We have a usecase where we want to audit symbol names (if available) in > callback registration hooks. (ex: fentry/nf_register_net_hook) > > A few months back, I proposed a bpf_kallsyms_lookup series but it was > decided in the reviews that a more generic helper, bpf_snprintf, would > be more useful. > > This series implements the helper according to the feedback received in > https://lore.kernel.org/bpf/20201126165748.1748417-1-rev...@google.com/T/#u > > - A new arg type guarantees the NULL-termination of string arguments and > lets us pass format strings in only one arg > - A new helper is implemented using that guarantee. Because the format > string is known at verification time, the format string validation is > done by the verifier > - To implement a series of tests for bpf_snprintf, the logic for > marshalling variadic args in a fixed-size array is reworked as per: > https://lore.kernel.org/bpf/20210310015455.1095207-1-rev...@chromium.org/T/#u > > --- > Changes in v5: > - Fixed the bpf_printf_buf_used counter logic in try_get_fmt_tmp_buf > - Added a couple of extra incorrect specifiers tests > - Call test_snprintf_single__destroy unconditionally > - Fixed a C++-style comment > > --- > Changes in v4: > - Moved bpf_snprintf, bpf_printf_prepare and bpf_printf_cleanup to > kernel/bpf/helpers.c so that they get built without CONFIG_BPF_EVENTS > - Added negative test cases (various invalid format strings) > - Renamed put_fmt_tmp_buf() as bpf_printf_cleanup() > - Fixed a mistake that caused temporary buffers to be unconditionally > freed in bpf_printf_prepare > - Fixed a mistake that caused missing 0 character to be ignored > - Fixed a warning about integer to pointer conversion > - Misc cleanups > > --- > Changes in v3: > - Simplified temporary buffer acquisition with try_get_fmt_tmp_buf() > - Made zero-termination check more consistent > - Allowed NULL output_buffer > - Simplified the BPF_CAST_FMT_ARG macro > - Three new test cases: number padding, simple string with no arg and > string length extraction only with a NULL output buffer > - Clarified helper's description for edge cases (eg: str_size == 0) > - Lots of cosmetic changes > > --- > Changes in v2: > - Extracted the format validation/argument sanitization in a generic way > for all printf-like helpers. > - bpf_snprintf's str_size can now be 0 > - bpf_snprintf is now exposed to all BPF program types > - We now preempt_disable when using a per-cpu temporary buffer > - Addressed a few cosmetic changes > > Florent Revest (6): > bpf: Factorize bpf_trace_printk and bpf_seq_printf > bpf: Add a ARG_PTR_TO_CONST_STR argument type > bpf: Add a bpf_snprintf helper > libbpf: Initialize the bpf_seq_printf parameters array field by field > libbpf: Introduce a BPF_SNPRINTF helper macro > selftests/bpf: Add a series of tests for bpf_snprintf > > include/linux/bpf.h | 22 ++ > include/uapi/linux/bpf.h | 28 ++ > kernel/bpf/helpers.c | 306 ++ > kernel/bpf/verifier.c | 82 > kernel/trace/bpf_trace.c | 373 ++ > tools/include/uapi/linux/bpf.h| 28 ++ > tools/lib/bpf/bpf_tracing.h | 58 ++- > .../selftests/bpf/prog_tests/snprintf.c | 125 ++ > .../selftests/bpf/progs/test_snprintf.c | 73 > .../bpf/progs/test_snprintf_single.c | 20 + > 10 files changed, 770 insertions(+), 345 deletions(-) > create mode 100644 tools/testing/selftests/bpf/prog_tests/snprintf.c > create mode 100644 tools/testing/selftests/bpf/progs/test_snprintf.c > create mode 100644 tools/testing/selftests/bpf/progs/test_snprintf_single.c > > -- > 2.31.1.368.gbe11c130af-goog > Looks great, thank you! For the series: Acked-by: Andrii Nakryiko
[PATCH bpf-next v5 0/6] Add a snprintf eBPF helper
We have a usecase where we want to audit symbol names (if available) in callback registration hooks. (ex: fentry/nf_register_net_hook) A few months back, I proposed a bpf_kallsyms_lookup series but it was decided in the reviews that a more generic helper, bpf_snprintf, would be more useful. This series implements the helper according to the feedback received in https://lore.kernel.org/bpf/20201126165748.1748417-1-rev...@google.com/T/#u - A new arg type guarantees the NULL-termination of string arguments and lets us pass format strings in only one arg - A new helper is implemented using that guarantee. Because the format string is known at verification time, the format string validation is done by the verifier - To implement a series of tests for bpf_snprintf, the logic for marshalling variadic args in a fixed-size array is reworked as per: https://lore.kernel.org/bpf/20210310015455.1095207-1-rev...@chromium.org/T/#u --- Changes in v5: - Fixed the bpf_printf_buf_used counter logic in try_get_fmt_tmp_buf - Added a couple of extra incorrect specifiers tests - Call test_snprintf_single__destroy unconditionally - Fixed a C++-style comment --- Changes in v4: - Moved bpf_snprintf, bpf_printf_prepare and bpf_printf_cleanup to kernel/bpf/helpers.c so that they get built without CONFIG_BPF_EVENTS - Added negative test cases (various invalid format strings) - Renamed put_fmt_tmp_buf() as bpf_printf_cleanup() - Fixed a mistake that caused temporary buffers to be unconditionally freed in bpf_printf_prepare - Fixed a mistake that caused missing 0 character to be ignored - Fixed a warning about integer to pointer conversion - Misc cleanups --- Changes in v3: - Simplified temporary buffer acquisition with try_get_fmt_tmp_buf() - Made zero-termination check more consistent - Allowed NULL output_buffer - Simplified the BPF_CAST_FMT_ARG macro - Three new test cases: number padding, simple string with no arg and string length extraction only with a NULL output buffer - Clarified helper's description for edge cases (eg: str_size == 0) - Lots of cosmetic changes --- Changes in v2: - Extracted the format validation/argument sanitization in a generic way for all printf-like helpers. - bpf_snprintf's str_size can now be 0 - bpf_snprintf is now exposed to all BPF program types - We now preempt_disable when using a per-cpu temporary buffer - Addressed a few cosmetic changes Florent Revest (6): bpf: Factorize bpf_trace_printk and bpf_seq_printf bpf: Add a ARG_PTR_TO_CONST_STR argument type bpf: Add a bpf_snprintf helper libbpf: Initialize the bpf_seq_printf parameters array field by field libbpf: Introduce a BPF_SNPRINTF helper macro selftests/bpf: Add a series of tests for bpf_snprintf include/linux/bpf.h | 22 ++ include/uapi/linux/bpf.h | 28 ++ kernel/bpf/helpers.c | 306 ++ kernel/bpf/verifier.c | 82 kernel/trace/bpf_trace.c | 373 ++ tools/include/uapi/linux/bpf.h| 28 ++ tools/lib/bpf/bpf_tracing.h | 58 ++- .../selftests/bpf/prog_tests/snprintf.c | 125 ++ .../selftests/bpf/progs/test_snprintf.c | 73 .../bpf/progs/test_snprintf_single.c | 20 + 10 files changed, 770 insertions(+), 345 deletions(-) create mode 100644 tools/testing/selftests/bpf/prog_tests/snprintf.c create mode 100644 tools/testing/selftests/bpf/progs/test_snprintf.c create mode 100644 tools/testing/selftests/bpf/progs/test_snprintf_single.c -- 2.31.1.368.gbe11c130af-goog
