Re: [PATCH bpf-next v4 3/6] bpf: Move is_valid_bpf_tramp_flags() to the public trampoline code

On Tue, May 17, 2022 at 03:18:35AM -0400, Xu Kuohai wrote:

+static bool is_valid_bpf_tramp_flags(unsigned int flags) +{

• if ((flags & BPF_TRAMP_F_RESTORE_REGS) &&

•    (flags & BPF_TRAMP_F_SKIP_FRAME))
  

• return false;
  

• /* BPF_TRAMP_F_RET_FENTRY_RET is only used by bpf_struct_ops,

• * and it must be used alone.
  

• */
  

• if ((flags & BPF_TRAMP_F_RET_FENTRY_RET) &&

•    (flags & ~BPF_TRAMP_F_RET_FENTRY_RET))
  

• return false;
  

• return true;

+}

+int bpf_prepare_trampoline(struct bpf_tramp_image *tr, void *image,

• 	   void *image_end, const struct btf_func_model *m,
  

• 	   u32 flags, struct bpf_tramp_links *tlinks,
  

• 	   void *orig_call)
  

+{

• if (!is_valid_bpf_tramp_flags(flags))

• return -EINVAL;
  

• return arch_prepare_bpf_trampoline(tr, image, image_end, m, flags,

• 			   tlinks, orig_call);
  

+}

It's an overkill to introduce a new helper function just to validate flags that almost compile time constants. The flags are not user supplied. Please move /* BPF_TRAMP_F_RET_FENTRY_RET is only used by bpf_struct_ops ... */ comment to bpf_struct_ops.c right before it calls arch_prepare_bpf_trampoline() And add a comment to trampoline.c saying that BPF_TRAMP_F_RESTORE_REGS and BPF_TRAMP_F_SKIP_FRAME should not be set together. We could add a warn_on there or in arch code, but feels like overkill.