On Mon, Sep 25, 2017 at 2:16 PM, Daniel Borkmann <dan...@iogearbox.net> wrote:
> On 09/25/2017 07:13 PM, Cong Wang wrote:
>> ret = cls_bpf_offload(tp, prog, oldprog);
>> if (ret) {
>> + if (!oldprog)
>> + idr_remove_ext(&head->handle_idr, prog->handle);
>
>
> Shouldn't we also call idr_remove_ext() when there was an
> oldprog, but we didn't care about reusing the same handle,
> so it was handle == 0 initially?
When oldprog is non-NULL, we are replacing the oldprog with
a new one, therefore we should call idr_replace_ext() which
happens after this. So no need to call idr_remove_ext() at
this point.
>
> There's this condition in the code before above idr allocations,
> I think also in other classifiers:
>
> if (oldprog) {
> if (handle && oldprog->handle != handle) {
> ret = -EINVAL;
> goto errout;
> }
> }
Sure. If we use handle to find oldprog, it should have the
same handle. cls_bpf_get() guarantees it. This check is
redundant.
>
>> __cls_bpf_delete_prog(prog);
>> return ret;
>> }
>> @@ -499,6 +494,7 @@ static int cls_bpf_change(struct net *net, struct
>> sk_buff *in_skb,
>> prog->gen_flags |= TCA_CLS_FLAGS_NOT_IN_HW;
>>
>> if (oldprog) {
>> + idr_replace_ext(&head->handle_idr, prog, handle);
>
>
> And here, we should probably use prog->handle for the above
> mentioned case as well, no?
Since are replacing oldprog with a new one, prog->handle is
same with handle.
>
> Would be great if all this (and e.g. the fact that we use idr itself)
> could optionally be hidden behind some handle generator api given
> we could reuse that api also for cls_basic and cls_u32. Could also
> be followed-up perhaps.
>
Yeah, the idr_alloc_ext(.., handle, handle+1,) is ugly. Ideally we should
specify the range during initialization rather than in each idr_alloc_ext().
Commit c15ab236d69d already did the same thing. We can refactor
this later.
