All of lore.kernel.org
 help / color / mirror / Atom feed
From: Slava Imameev <slava.imameev@crowdstrike.com>
To: <eddyz87@gmail.com>
Cc: <andrii@kernel.org>, <ast@kernel.org>, <bpf@vger.kernel.org>,
	<daniel@iogearbox.net>, <davem@davemloft.net>,
	<edumazet@google.com>, <haoluo@google.com>, <horms@kernel.org>,
	<john.fastabend@gmail.com>, <jolsa@kernel.org>,
	<kpsingh@kernel.org>, <kuba@kernel.org>,
	<linux-kernel@vger.kernel.org>, <linux-kselftest@vger.kernel.org>,
	<linux-open-source@crowdstrike.com>, <martin.lau@linux.dev>,
	<netdev@vger.kernel.org>, <pabeni@redhat.com>, <sdf@fomichev.me>,
	<shuah@kernel.org>, <slava.imameev@crowdstrike.com>,
	<song@kernel.org>, <yonghong.song@linux.dev>
Subject: Re: [PATCH bpf-next v4 1/2] bpf: Support new pointer param types via SCALAR_VALUE for trampolines
Date: Wed, 4 Mar 2026 11:22:05 +1100	[thread overview]
Message-ID: <20260304002205.15728-1-slava.imameev@crowdstrike.com> (raw)
In-Reply-To: <84c687e56ed8f04f3f318f090272fb5ef7520e96.camel@gmail.com>

On Tue, 03 Mar 2026 14:43:01, Eduard Zingerman wrote:
> On Wed, 2026-03-04 at 08:49 +1100, Slava Imameev wrote:
> > On 2026-03-03 20:05 UTC, Eduard Zingerman wrote:
> >
> > > > @@ -6902,11 +6921,7 @@ bool btf_ctx_access(int off, int size, enum bpf_access_type type,
> > > >               }
> > > >       }
> > > >
> > > > -     /*
> > > > -      * If it's a pointer to void, it's the same as scalar from the verifier
> > > > -      * safety POV. Either way, no futher pointer walking is allowed.
> > > > -      */
> > > > -     if (is_void_or_int_ptr(btf, t))
> > > > +     if (is_ptr_treated_as_scalar(btf, t))
> > > >               return true;
> > >
> > > I'm probably missing a point here, but what's wrong with Alexei's
> > > suggestion to do this instead:
> > >
> > >         if (is_ptr_treated_as_scalar(btf, t))
> > >                  return true;
> > > ?
> 
> Uh-oh, I copy-pasted the wrong snippet, sorry.
> The correct snippet is:
> 
>          if (btf_type_is_struct_ptr(btf, t))
>                   return true;
> 
> With it the selftests pass (except for `float` tests noted earlier).
> And regardless of selftests, the code below this point will
> error out if `t` is not a pointer to struct.

I think you tested with

	if (!btf_type_is_struct_ptr(btf, t))
		return true;

I decided on a narrower condition, as

- if (!btf_type_is_struct_ptr(btf, t)) -

changes the existing selection condition from "treat only these types
as scalar" to "treat as scalar any type that is not a pointer to
structure". Technically both approaches cover the problem I'm trying
to solve - multilevel pointer support for structures, but the latter is
open-ended and changes the current approach, which checks for pointers
to int and void. So I'm extending this to int, void, enum 32/64,
function, and corresponding multilevel pointers to these types and
multilevel pointers to structures.

It seems - if (!btf_type_is_struct_ptr(btf, t)) - works, but it's
challenging to strictly prove it's sufficiently future-proof.

> > This reflects my belief in a cautious approach: adding support
> > only for selected types with tests added for each new type. That said,
> > I can add the suggested broader condition and make it pass the tests,
> but I cannot be sure it will be future-proof against conflicts.
> >
> > I think the broader check like
> >
> >       /* skip modifiers */
> >       tt = t;
> >       while (btf_type_is_modifier(tt))
> >               tt = btf_type_by_id(btf, tt->type);
> >       if (!btf_type_is_struct(tt))
> >               return true;
> 
> btf_type_is_struct_ptr() is almost identical to the snippet above.
> 
> > might have some incompatibility with future changes, compared to
> > explicit type checks for selected types. This condition is
> > open-ended, including anything instead of selecting specific types.
> 
> What potential incompatibility do you expect?
> Two things change:
> - types other then `struct foo *` or `int` can be read:
>   - do you expect we would want to deny reading some ctx
>     fields in the future?
> - the value read is marked as scalar:
>   - not much can be done with a scalar, except for leaking it to
>     e.g. some map or ring buffer. Do you expect this to problematic?
> 
> Note that the above are selected based on type, not on the
> function/parameter combination, which is already not a very effective
> filter if some parameters need to be hidden.

I do not think any of these represent a real problem. As I said,
my approach is based mostly on narrowing the supported types to
reduce potential conflicts.

I do not have a good example of such conflicts.
The added tests for pointer to float, which failed with -
if (!btf_type_is_struct_ptr(btf, t)) - might be an example when adding
a new type might silently pass this check because of missing tests.

I  was not able to convince myself a conflict will not  happen.

That said, changing

	if (is_ptr_treated_as_scalar(btf, t))
		return true;

	to

	if (!btf_type_is_struct_ptr(btf, t))
		return true;

just makes the scope of these changes wider. This was
my initial approach to this problem, but I was worried
by its wide scope.


  reply	other threads:[~2026-03-04  0:22 UTC|newest]

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-03-03  9:54 [PATCH bpf-next v4 0/2] bpf: Add multi-level pointer parameter support for trampolines Slava Imameev
2026-03-03  9:54 ` [PATCH bpf-next v4 1/2] bpf: Support new pointer param types via SCALAR_VALUE " Slava Imameev
2026-03-03 20:05   ` Eduard Zingerman
2026-03-03 21:49     ` Slava Imameev
2026-03-03 22:43       ` Eduard Zingerman
2026-03-04  0:22         ` Slava Imameev [this message]
2026-03-04  0:36           ` Alexei Starovoitov
2026-03-04  0:38           ` Eduard Zingerman
2026-03-10 12:16             ` Slava Imameev
2026-03-10 18:52               ` Eduard Zingerman
2026-03-11 13:07                 ` Slava Imameev
2026-03-11 16:31                   ` Eduard Zingerman
2026-03-03  9:54 ` [PATCH bpf-next v4 2/2] selftests/bpf: Add trampolines single and multi-level pointer params test coverage Slava Imameev
2026-03-03 20:08   ` Eduard Zingerman
2026-03-03 22:14     ` Slava Imameev

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=20260304002205.15728-1-slava.imameev@crowdstrike.com \
    --to=slava.imameev@crowdstrike.com \
    --cc=andrii@kernel.org \
    --cc=ast@kernel.org \
    --cc=bpf@vger.kernel.org \
    --cc=daniel@iogearbox.net \
    --cc=davem@davemloft.net \
    --cc=eddyz87@gmail.com \
    --cc=edumazet@google.com \
    --cc=haoluo@google.com \
    --cc=horms@kernel.org \
    --cc=john.fastabend@gmail.com \
    --cc=jolsa@kernel.org \
    --cc=kpsingh@kernel.org \
    --cc=kuba@kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-kselftest@vger.kernel.org \
    --cc=linux-open-source@crowdstrike.com \
    --cc=martin.lau@linux.dev \
    --cc=netdev@vger.kernel.org \
    --cc=pabeni@redhat.com \
    --cc=sdf@fomichev.me \
    --cc=shuah@kernel.org \
    --cc=song@kernel.org \
    --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.