All of lore.kernel.org
 help / color / mirror / Atom feed
From: Jiri Olsa <olsajiri@gmail.com>
To: Jiawei Zhao <phoenix500526@163.com>
Cc: ast@kernel.org, daniel@iogearbox.net, andrii@kernel.org,
	yonghong.song@linux.dev, bpf@vger.kernel.org,
	linux-kselftest@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v6 1/2] libbpf: fix USDT SIB argument handling causing unrecognized register error
Date: Mon, 4 Aug 2025 12:23:59 +0200	[thread overview]
Message-ID: <aJCKP1Cja3DCm0EG@krava> (raw)
In-Reply-To: <20250802084803.108777-2-phoenix500526@163.com>

On Sat, Aug 02, 2025 at 08:48:02AM +0000, Jiawei Zhao 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@163.com>
> ---
>  tools/lib/bpf/usdt.bpf.h | 33 +++++++++++++++++++++++++++++-
>  tools/lib/bpf/usdt.c     | 43 ++++++++++++++++++++++++++++++++++------
>  2 files changed, 69 insertions(+), 7 deletions(-)
> 
> diff --git a/tools/lib/bpf/usdt.bpf.h b/tools/lib/bpf/usdt.bpf.h
> index 2a7865c8e3fe..246513088c3a 100644
> --- a/tools/lib/bpf/usdt.bpf.h
> +++ b/tools/lib/bpf/usdt.bpf.h
> @@ -34,6 +34,7 @@ enum __bpf_usdt_arg_type {
>  	BPF_USDT_ARG_CONST,
>  	BPF_USDT_ARG_REG,
>  	BPF_USDT_ARG_REG_DEREF,
> +	BPF_USDT_ARG_SIB,
>  };
>  
>  struct __bpf_usdt_arg_spec {
> @@ -43,6 +44,10 @@ struct __bpf_usdt_arg_spec {
>  	enum __bpf_usdt_arg_type arg_type;
>  	/* offset of referenced register within struct pt_regs */
>  	short reg_off;
> +	/* offset of index register in pt_regs, only used in SIB mode */
> +	short idx_reg_off;
> +	/* scale factor for index register, only used in SIB mode */
> +	short scale;
>  	/* whether arg should be interpreted as signed value */
>  	bool arg_signed;
>  	/* number of bits that need to be cleared and, optionally,
> @@ -149,7 +154,7 @@ 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;
>  
>  	*res = 0;
> @@ -202,6 +207,32 @@ 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.
> +		 */
> +		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 + arg_spec->idx_reg_off);
> +		if (err)
> +			return err;
> +		err = bpf_probe_read_user(&val, sizeof(val),
> +				(void *)val + idx * arg_spec->scale + arg_spec->val_off);
> +		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 4e4a52742b01..1f8b9e1c9819 100644
> --- a/tools/lib/bpf/usdt.c
> +++ b/tools/lib/bpf/usdt.c
> @@ -200,6 +200,7 @@ 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 */
> @@ -207,6 +208,8 @@ struct usdt_arg_spec {
>  	__u64 val_off;
>  	enum usdt_arg_type arg_type;
>  	short reg_off;
> +	short idx_reg_off;
> +	short scale;
>  	bool arg_signed;
>  	char arg_bitshift;
>  };
> @@ -1283,11 +1286,39 @@ 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;
> +
> +	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, e.g., 1@-96(%rbp,%rax,8)
> +		 * 1@(%rbp,%rax,8)
> +		 * 1@-96(%rbp,%rax)
> +		 * 1@(%rbp,%rax)
> +		 */

hi,
I'm getting following error from the test:

subtest_multispec_usdt:PASS:skel_open 0 nsec
libbpf: usdt: unrecognized arg #10 spec '-2@nums(%rax,%rax) -1@$-127'
libbpf: prog 'usdt12': failed to auto-attach: -EINVAL
subtest_multispec_usdt:FAIL:skel_attach unexpected error: -22 (errno 22)
#480/2   usdt/multispec:FAIL

arguments look like:
    Arguments: -4@$3 -4@$4 -8@$42 -8@$45 -4@$5 -8@$6 8@%rdx 8@%rsi -4@$-9 -2@%cx -2@nums(%rax,%rax) -1@$-127

not sure why there's variable name in the arg10 definition

gcc (GCC) 15.1.1 20250521 (Red Hat 15.1.1-2)
clang version 20.1.8 (Fedora 20.1.8-3.fc42)

thanks,
jirka


> +		arg->arg_type = USDT_ARG_SIB;
> +		arg->val_off = off;
> +		arg->scale = scale;
> +
> +		reg_off = calc_pt_regs_off(reg_name);
> +		if (reg_off < 0)
> +			return reg_off;
> +		arg->reg_off = reg_off;
>  
> -	if (sscanf(arg_str, " %d @ %ld ( %%%15[^)] ) %n", arg_sz, &off, reg_name, &len) == 3) {
> +		idx_reg_off = calc_pt_regs_off(idx_reg_name);
> +		if (idx_reg_off < 0)
> +			return idx_reg_off;
> +		arg->idx_reg_off = idx_reg_off;
> +	} 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;
> @@ -1298,7 +1329,7 @@ static int parse_usdt_arg(const char *arg_str, int arg_num, struct usdt_arg_spec
>  	} else if (sscanf(arg_str, " %d @ ( %%%15[^)] ) %n", arg_sz, reg_name, &len) == 2) {
>  		/* Memory dereference case without offset, e.g., 8@(%rsp) */
>  		arg->arg_type = USDT_ARG_REG_DEREF;
> -		arg->val_off = 0;
> +		arg->val_off = off;
>  		reg_off = calc_pt_regs_off(reg_name);
>  		if (reg_off < 0)
>  			return reg_off;
> @@ -1306,7 +1337,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@%eax */
>  		arg->arg_type = USDT_ARG_REG;
> -		arg->val_off = 0;
> +		arg->val_off = off;
>  
>  		reg_off = calc_pt_regs_off(reg_name);
>  		if (reg_off < 0)
> -- 
> 2.43.0
> 
> 

  reply	other threads:[~2025-08-04 10:24 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2025-08-02  8:48 [PATCH v6 0/2] libbpf: fix USDT SIB argument handling causing unrecognized register error Jiawei Zhao
2025-08-02  8:48 ` [PATCH v6 1/2] " Jiawei Zhao
2025-08-04 10:23   ` Jiri Olsa [this message]
2025-08-05  4:15     ` 赵佳炜
2025-08-02  8:48 ` [PATCH v6 2/2] selftests/bpf: Force -O2 for USDT selftests to cover SIB handling logic Jiawei Zhao
2025-08-05 19:42   ` Yonghong Song
2025-08-06  9:47     ` 赵佳炜

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=aJCKP1Cja3DCm0EG@krava \
    --to=olsajiri@gmail.com \
    --cc=andrii@kernel.org \
    --cc=ast@kernel.org \
    --cc=bpf@vger.kernel.org \
    --cc=daniel@iogearbox.net \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-kselftest@vger.kernel.org \
    --cc=phoenix500526@163.com \
    --cc=yonghong.song@linux.dev \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.