[linux-morello] Re: [PATCH v5 05/13] bpf: compat64: add handler and convert bpf_attr in

On 14/11/2023 10:31, Zachary Leaf wrote:

[...]

• case BPF_LINK_CREATE:

• copy_field(dest, cattr, link_create.prog_fd);
  

• copy_field(dest, cattr, link_create.target_fd);
  

• /* u32 target_ifindex is in a union with u32 target_fd */
  

• copy_field(dest, cattr, link_create.attach_type);
  

• copy_field(dest, cattr, link_create.flags);
  

• /*
  

•  * identify the union members that require conversion (i.e. with
  

•  * pointers) by attach_type, otherwise just memcpy the lot
  

•  */
  

• if (cattr->link_create.attach_type == BPF_TRACE_ITER) {
  

• 	/*
  

• 	 * iter_info is a user pointer to union
  

• 	 * bpf_iter_link_info however since this union
  

• 	 * contains no pointers, the size/offsets are
  

• 	 * the same for compat64/purecap; hence no
  

Nit: to stay generic, we could say "regardless of the ABI".

• 	 * conversion needed
  

• 	 */
  

• 	copy_field(dest, cattr, link_create.iter_info);
  

• 	copy_field(dest, cattr, link_create.iter_info_len);
  

• 	break;
  

• }
  

• /* kprobe_multi is used in bpf_kprobe_multi_link_attach() */
  

• if (cattr->link_create.attach_type == BPF_TRACE_KPROBE_MULTI) {
  

• 	copy_field(dest, cattr, link_create.kprobe_multi.flags);
  

• 	copy_field(dest, cattr, link_create.kprobe_multi.cnt);
  

• 	copy_field(dest, cattr, link_create.kprobe_multi.syms);
  

• 	copy_field(dest, cattr, link_create.kprobe_multi.addrs);
  

• 	copy_field(dest, cattr, link_create.kprobe_multi.cookies);
  

• 	break;
  

• }
  

Nit: could make the whole thing a switch, with the memcpy() as default.

• /* remaining union members only contain fixed size integers */
  

• memcpy((u8 *)dest+offsetof(union bpf_attr,
  

• 			   link_create.netfilter.pf),
  

•        (u8 *)cattr+offsetof(union compat_bpf_attr,
  

• 			    link_create.netfilter.pf),
  

•        offsetofend(union bpf_attr, link_create.netfilter.flags) -
  

•        offsetof(union bpf_attr, link_create.netfilter.pf));
  

Since the biggest struct in the anonymous union is kprobe_multi, there is a possibility/risk that a new struct, bigger than netfilter, will be introduced later. To avoid ending up with a partial memcpy(), it would be safer to compute the size of the whole union (in the compat struct), something like offsetofend(compat, link_create) - offsetofend(compat, link_create.flags). It may look a bit strange so a comment wouldn't hurt.

Kevin