Re: [PATCH bpf-next 0/1] use preserve_static_offset in bpf uapi headers

[Yonghong Song <yonghong.song@linux.dev>]

On 12/8/23 6:34 AM, Eduard Zingerman wrote:
> On Thu, 2023-12-07 at 18:28 -0800, Yonghong Song wrote:
> [...]
>> All context types are defined in include/linux/bpf_types.h.
>> The context type bpf_nf_ctx is missing.
> convert_ctx_access() is not applied for bpf_nf_ctx. Searching through
> kernel code shows that BPF programs access this structure directly
> (net/netfilter/nf_bpf_link.c):
>
>      static unsigned int nf_hook_run_bpf(void *bpf_prog, struct sk_buff *skb,
>                          const struct nf_hook_state *s)
>      {
>          const struct bpf_prog *prog = bpf_prog;
>          struct bpf_nf_ctx ctx = {
>              .state = s,
>              .skb = skb,
>          };
>
>          return bpf_prog_run(prog, &ctx);
>      }
>
> I added __bpf_ctx only for types that are subject to convert_ctx_access()
> transformation. On the other hand, applying it to each context type
> should not hurt either. Which way would you prefer?
>
> [...]

The error message should happen here:

check_mem_access
  ...
  } else if (reg->type == PTR_TO_CTX) {
   check_ptr_off_reg
    __check_ptr_off_reg
         if (!fixed_off_ok && reg->off) {
                 verbose(env, "dereference of modified %s ptr R%d off=%d disallowed\n",
                         reg_type_str(env, reg->type), regno, reg->off);
                 return -EACCES;
         }
   ...

So the verification error message will be emitted earlier, before convert_ctx_access.
Could you double check?

>
>>> How to add the same definitions in vmlinux.h is an open question,
>>> and most likely requires bpftool modification:
>>> - Hard code generation of __bpf_ctx based on type names?
>>> - Mark context types with some special
>>>     __attribute__((btf_decl_tag("preserve_static_offset")))
>>>     and convert it to __attribute__((preserve_static_offset))?
>> The number of context types is limited, I would just go through
>> the first approach with hard coding the list of ctx types and
>> mark them with preserve_static_offset attribute in vmlinux.h.
> Tbh, I'm with Alan here, generic approach seems a tad nicer.
> Lets collect some more votes :)