| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAEf4BzaRip7ghZ2Z_1yF_OXkno6Na-wcrM5522fDV1c1mLGLpQ@mail.gmail.com>
Date: Wed, 20 Aug 2025 16:18:47 -0700
From: Andrii Nakryiko <andrii.nakryiko@...il.com>
To: Jiawei Zhao <phoenix500526@....com>
Cc: ast@...nel.org, daniel@...earbox.net, andrii@...nel.org,
yonghong.song@...ux.dev, bpf@...r.kernel.org, linux-kselftest@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH bpf-next v11 1/3] libbpf: fix USDT SIB argument handling
causing unrecognized register error
On Mon, Aug 18, 2025 at 12:47 AM Jiawei Zhao <phoenix500526@....com> wrote:
>
> On x86-64, USDT arguments can be specified using Scale-Index-Base (SIB)
> addressing, e.g. "1@-96(%rbp,%rax,8)". The current USDT implementation
> in libbpf cannot parse this format, causing `bpf_program__attach_usdt()`
> to fail with -ENOENT (unrecognized register).
>
> This patch fixes this by implementing the necessary changes:
> - add correct handling for SIB-addressed arguments in `bpf_usdt_arg`.
> - add adaptive support to `__bpf_usdt_arg_type` and
> `__bpf_usdt_arg_spec` to represent SIB addressing parameters.
>
> Signed-off-by: Jiawei Zhao <phoenix500526@....com>
> ---
> tools/lib/bpf/usdt.bpf.h | 54 +++++++++++++++++++++++++++++++++--
> tools/lib/bpf/usdt.c | 61 ++++++++++++++++++++++++++++++++++++----
> 2 files changed, 108 insertions(+), 7 deletions(-)
>
> diff --git a/tools/lib/bpf/usdt.bpf.h b/tools/lib/bpf/usdt.bpf.h
> index 2a7865c8e3fe..1e6de0afe65d 100644
> --- a/tools/lib/bpf/usdt.bpf.h
> +++ b/tools/lib/bpf/usdt.bpf.h
> @@ -34,13 +34,31 @@ enum __bpf_usdt_arg_type {
> BPF_USDT_ARG_CONST,
> BPF_USDT_ARG_REG,
> BPF_USDT_ARG_REG_DEREF,
> + BPF_USDT_ARG_SIB,
> };
>
> +/*
> + * To preserve overall layout and avoid growing this struct while adding SIB
> + * extras, we keep 4 bytes worth of space after val_off:
> + *
> + * - arg_type is stored as a single byte (values from enum below)
this is cleaner expressed with
enum __bpf_usdt_arg_type arg_type: 8;
in code
> + * - idx_packed is a 16-bit field packing idx_reg_off (high 12 bits)
again, using bitfields make this obvious from code without verbose comment
> + * and scale shift (low 4 bits, i.e., scale = 1 << shift)
> + * - reserved is one spare byte for future use
this is obvious and doesn't require a comment
> + *
> + * This keeps the offset of reg_off identical to the historical layout
Not just ref_off, but arg_type as well. I'd keep it a bit more generic:
"This struct layout is designed specifically to be backwards/forward
compatible between libbpf versions for ARG_CONST, ARG_REG, and
ARG_REG_DEREF modes. ARG_SIB requires libbpf v1.7+."
> + * (val_off:8 + 4 bytes here), ensuring backwards/forwards compatibility for
> + * non-SIB modes that only rely on val_off/arg_type/reg_off/... offsets.
> + */
> struct __bpf_usdt_arg_spec {
> /* u64 scalar interpreted depending on arg_type, see below */
> __u64 val_off;
> /* arg location case, see bpf_usdt_arg() for details */
> - enum __bpf_usdt_arg_type arg_type;
> + __u8 arg_type;
> + /* packed: [15:4] idx_reg_off, [3:0] scale_shift */
> + __u16 idx_packed;
> + /* reserved for future use, keeps reg_off offset stable */
> + __u8 reserved;
See above about bitfields. It's cleaner approach and won't need
"idx_packed", it would be just two fields.
We only need to be careful about big-endian, so doing an
#ifdef/#else/#endif and defining different order of bitfields would be
necessary. See how struct iphdr in kernel does this for ihl and
version fields.
> /* offset of referenced register within struct pt_regs */
> short reg_off;
> /* whether arg should be interpreted as signed value */
> @@ -52,6 +70,10 @@ struct __bpf_usdt_arg_spec {
> char arg_bitshift;
> };
>
> +/* Helpers to (un)pack SIB extras from idx_packed without relying on bitfields. */
> +#define USDT_IDX_OFF(packed) ((packed) >> 4)
> +#define USDT_IDX_SCALE_SHIFT(packed) ((packed) & 0x000f)
no need for this when using bitfields either
> +
> /* should match USDT_MAX_ARG_CNT in usdt.c exactly */
> #define BPF_USDT_MAX_ARG_CNT 12
> struct __bpf_usdt_spec {
> @@ -149,8 +171,9 @@ int bpf_usdt_arg(struct pt_regs *ctx, __u64 arg_num, long *res)
> {
> struct __bpf_usdt_spec *spec;
> struct __bpf_usdt_arg_spec *arg_spec;
> - unsigned long val;
> + unsigned long val, idx;
> int err, spec_id;
> + int idx_off = 0, scale = 0;
>
> *res = 0;
>
> @@ -202,6 +225,33 @@ int bpf_usdt_arg(struct pt_regs *ctx, __u64 arg_num, long *res)
> return err;
> #if __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__
> val >>= arg_spec->arg_bitshift;
> +#endif
> + break;
> + case BPF_USDT_ARG_SIB:
> + /* Arg is in memory addressed by SIB (Scale-Index-Base) mode
> + * (e.g., "-1@-96(%rbp,%rax,8)" in USDT arg spec). Register
> + * is identified like with BPF_USDT_ARG_SIB case, the offset
> + * is in arg_spec->val_off, the scale factor is in arg_spec->scale.
> + * Firstly, we fetch the base register contents and the index
> + * register contents from pt_regs. Secondly, we multiply the
> + * index register contents by the scale factor, then add the
> + * base address and the offset to get the final address. Finally,
> + * we do another user-space probe read to fetch argument value
> + * itself.
> + */
> + idx_off = USDT_IDX_OFF(arg_spec->idx_packed);
> + scale = 1UL << USDT_IDX_SCALE_SHIFT(arg_spec->idx_packed);
> + err = bpf_probe_read_kernel(&val, sizeof(val), (void *)ctx + arg_spec->reg_off);
> + if (err)
> + return err;
> + err = bpf_probe_read_kernel(&idx, sizeof(idx), (void *)ctx + idx_off);
> + if (err)
> + return err;
> + err = bpf_probe_read_user(&val, sizeof(val), (void *)val + idx * scale + arg_spec->val_off);
you have scale_shift, just (idx << scale_shift) and avoid unnecessary
multiplication and transformations
> + if (err)
> + return err;
> +#if __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__
> + val >>= arg_spec->arg_bitshift;
> #endif
> break;
> default:
> diff --git a/tools/lib/bpf/usdt.c b/tools/lib/bpf/usdt.c
> index 3373b9d45ac4..0a6499f654fc 100644
> --- a/tools/lib/bpf/usdt.c
> +++ b/tools/lib/bpf/usdt.c
> @@ -200,12 +200,15 @@ enum usdt_arg_type {
> USDT_ARG_CONST,
> USDT_ARG_REG,
> USDT_ARG_REG_DEREF,
> + USDT_ARG_SIB,
> };
>
> /* should match exactly struct __bpf_usdt_arg_spec from usdt.bpf.h */
> struct usdt_arg_spec {
> __u64 val_off;
> - enum usdt_arg_type arg_type;
> + __u8 arg_type; /* enum value stored as u8 */
> + __u16 idx_packed; /* [15:4]=idx_reg_off, [3:0]=scale_shift */
> + __u8 reserved; /* keep reg_off offset stable */
> short reg_off;
> bool arg_signed;
> char arg_bitshift;
> @@ -214,6 +217,10 @@ struct usdt_arg_spec {
> /* should match BPF_USDT_MAX_ARG_CNT in usdt.bpf.h */
> #define USDT_MAX_ARG_CNT 12
>
> +/* Helpers to (un)pack SIB extras from idx_packed without relying on bitfields. */
> +#define BPF_USDT_IDX_PACK(idx_off, scale_shift) \
> + ((__u16)(((__u16)((idx_off) & 0x0fff)) << 4) | (__u16)((scale_shift) & 0x000f))
> +
> /* should match struct __bpf_usdt_spec from usdt.bpf.h */
> struct usdt_spec {
> struct usdt_arg_spec args[USDT_MAX_ARG_CNT];
> @@ -1283,11 +1290,54 @@ static int calc_pt_regs_off(const char *reg_name)
>
> static int parse_usdt_arg(const char *arg_str, int arg_num, struct usdt_arg_spec *arg, int *arg_sz)
> {
> - char reg_name[16];
> - int len, reg_off;
> - long off;
> + char reg_name[16] = {0}, idx_reg_name[16] = {0};
> + int len, reg_off, idx_reg_off, scale = 1;
> + long off = 0;
> + __u16 scale_shift;
> +
> + if (sscanf(arg_str, " %d @ %ld ( %%%15[^,] , %%%15[^,] , %d ) %n",
> + arg_sz, &off, reg_name, idx_reg_name, &scale, &len) == 5 ||
> + sscanf(arg_str, " %d @ ( %%%15[^,] , %%%15[^,] , %d ) %n",
> + arg_sz, reg_name, idx_reg_name, &scale, &len) == 4 ||
> + sscanf(arg_str, " %d @ %ld ( %%%15[^,] , %%%15[^)] ) %n",
> + arg_sz, &off, reg_name, idx_reg_name, &len) == 4 ||
> + sscanf(arg_str, " %d @ ( %%%15[^,] , %%%15[^)] ) %n",
> + arg_sz, reg_name, idx_reg_name, &len) == 3
> + ) {
> + /*
> + * Scale Index Base case:
> + * 1@-96(%rbp,%rax,8)
> + * 1@(%rbp,%rax,8)
> + * 1@-96(%rbp,%rax)
> + * 1@(%rbp,%rax)
> + */
> + arg->arg_type = USDT_ARG_SIB;
> + arg->val_off = off;
>
> - if (sscanf(arg_str, " %d @ %ld ( %%%15[^)] ) %n", arg_sz, &off, reg_name, &len) == 3) {
> + reg_off = calc_pt_regs_off(reg_name);
> + if (reg_off < 0)
> + return reg_off;
> + arg->reg_off = reg_off;
> +
> + idx_reg_off = calc_pt_regs_off(idx_reg_name);
> + if (idx_reg_off < 0)
> + return idx_reg_off;
> + /* pack idx_reg_off and scale shift (scale in {1,2,4,8}) */
> + if (scale == 1)
> + scale_shift = 0;
> + else if (scale == 2)
> + scale_shift = 1;
> + else if (scale == 4)
> + scale_shift = 2;
> + else if (scale == 8)
> + scale_shift = 3;
> + else {
> + pr_warn("usdt: invalid SIB scale %d, expected 1,2,4,8; defaulting to 1\n", scale);
> + return -EINVAL;
> + }
keep {} usage consistent within same if/else if/else block (even
though most of branches are single-line, they need {} due to the last
else clause having {})
pw-bot: cr
> + arg->idx_packed = BPF_USDT_IDX_PACK(idx_reg_off, scale_shift);
> + } else if (sscanf(arg_str, " %d @ %ld ( %%%15[^)] ) %n",
> + arg_sz, &off, reg_name, &len) == 3) {
> /* Memory dereference case, e.g., -4@-20(%rbp) */
> arg->arg_type = USDT_ARG_REG_DEREF;
> arg->val_off = off;
> @@ -1306,6 +1356,7 @@ static int parse_usdt_arg(const char *arg_str, int arg_num, struct usdt_arg_spec
> } else if (sscanf(arg_str, " %d @ %%%15s %n", arg_sz, reg_name, &len) == 2) {
> /* Register read case, e.g., -4@...x */
> arg->arg_type = USDT_ARG_REG;
> + /* register read has no memory offset */
> arg->val_off = 0;
>
> reg_off = calc_pt_regs_off(reg_name);
> --
> 2.43.0
>
Powered by blists - more mailing lists