On Wed, 2026-03-04 at 11:22 +1100, Slava Imameev wrote: > 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)) -
Yes, sorry again. > 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. BTF is defined for the following non-modifier types: - void [allowed already] - int [allowed already] - ptr [multi-level pointers allowed by your patch] - array [disallowed?] - struct [single level pointers allowed already, - union multi-level allowed by your patch] - enum/enum64 [allowed by your patch] - func_proto [allowed by your patch] - float [disallowed] And a few not reachable from function fields (I think BTF validation checks that these can't be met, but would be good to double-check. If it doesn't, it should): - func - var - datasec So, effectively you disallow reading from tracing context fields of type: struct (non-pointer), array, float and a few types that can't be specified for struct fields. Does not seem necessary, tbh. > 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. Yes, but that does not really matter if verifier treats floats as unbound scalars. > 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. Let's see what Alexei would say, but I'd say there is no need to complicate things w/o clear necessity.
