On Tue, May 12, 2020 at 08:41:19AM -0700, Yonghong Song wrote:
>
>
> On 5/9/20 5:41 PM, Alexei Starovoitov wrote:
> > On Sat, May 09, 2020 at 10:59:00AM -0700, Yonghong Song wrote:
> > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > > index 70ad009577f8..d725ff7d11db 100644
> > > --- a/kernel/bpf/verifier.c
> > > +++ b/kernel/bpf/verifier.c
> > > @@ -7101,6 +7101,10 @@ static int check_return_code(struct bpf_verifier_env *env)
> > > return 0;
> > > range = tnum_const(0);
> > > break;
> > > + case BPF_PROG_TYPE_TRACING:
> > > + if (env->prog->expected_attach_type != BPF_TRACE_ITER)
> > > + return 0;
> > > + break;
> >
> > Not related to this set, but I just noticed that I managed to forget to
> > add this check for fentry/fexit/freplace.
> > While it's not too late let's enforce return 0 for them ?
> > Could you follow up with a patch for bpf tree?
>
> Just want to double check. In selftests, we have
>
> SEC("fentry/__set_task_comm")
> int BPF_PROG(prog4, struct task_struct *tsk, const char *buf, bool exec)
> {
> return !tsk;
> }
>
> SEC("fexit/__set_task_comm")
> int BPF_PROG(prog5, struct task_struct *tsk, const char *buf, bool exec)
> {
> return !tsk;
> }
>
> fentry/fexit may returrn 1. What is the intention here? Does this mean
> we should allow [0, 1] instead of [0, 0]?
Argh. I missed that bit when commit ac065870d9282 tweaked the return
value. For fentry/exit the return value is ignored by trampoline.
imo it's misleading to users and should be rejected by the verifier.
so [0,0] for fentry/fexit
> For freplace, we have
>
> __u64 test_get_skb_len = 0;
> SEC("freplace/get_skb_len")
> int new_get_skb_len(struct __sk_buff *skb)
> {
> int len = skb->len;
>
> if (len != 74)
> return 0;
> test_get_skb_len = 1;
> return 74; /* original get_skb_len() returns skb->len */
> }
>
> That means freplace may return arbitrary values depending on what
> to replace?
yes. freplace and fmod_ret can return anything.
