2018-05-18 21:24 GMT+01:00 Daniel Borkmann <dan...@iogearbox.net>:
>> +#define MAX_PROG_NAME 256
>> +static const struct nla_policy bpf_prog_policy[LWT_BPF_PROG_MAX + 1] = {
>> + [LWT_BPF_PROG_FD] = { .type = NLA_U32, },
>
> From UAPI point of view, I wouldn't name it LWT_BPF_PROG_FD but rather
> something like
> LWT_BPF_PROG for example. That way, the setup can contain the fd number, but
> on the
> dump you can put the prog->aux->id in there so that prog lookup can be done
> again.
Good idea.
>> + [LWT_BPF_PROG_NAME] = { .type = NLA_NUL_STRING,
>> + .len = MAX_PROG_NAME },
>> +};
>> +
>> +static int parse_nla_bpf(struct nlattr **attrs, struct seg6_local_lwt *slwt)
>> +{
>> + struct nlattr *tb[LWT_BPF_PROG_MAX + 1];
>> + struct bpf_prog *p;
>> + int ret;
>> + u32 fd;
>> +
>> + ret = nla_parse_nested(tb, LWT_BPF_PROG_MAX, attrs[SEG6_LOCAL_BPF],
>> + bpf_prog_policy, NULL);
>> + if (ret < 0)
>> + return ret;
>> +
>> + if (!tb[LWT_BPF_PROG_FD] || !tb[LWT_BPF_PROG_NAME])
>> + return -EINVAL;
>> +
>> + slwt->bpf.name = nla_memdup(tb[LWT_BPF_PROG_NAME], GFP_KERNEL);
>> + if (!slwt->bpf.name)
>> + return -ENOMEM;
>> +
>> + fd = nla_get_u32(tb[LWT_BPF_PROG_FD]);
>> + p = bpf_prog_get_type(fd, BPF_PROG_TYPE_LWT_SEG6LOCAL);
>> + if (IS_ERR(p))
>> + return PTR_ERR(p);
>
> Here in the above error path is definitely a bug in that you don't free the
> prior allocated slwt->bpf.name from nla_memdup().
Indeed, I took this part from the lwt bpf infrastructure. I sent a
patch to fix it there too.
> Also when you destroy the struct seg6_local_lwt object, what I'm not getting
> is where you drop the prog reference again and free slwt->bpf.name there?
I totally missed this. Sending a v7 with all of this fixed.
Thanks for your comments !
