Re: [Patch net-next v2] net_sched: use idr to allocate bpf filter handles

On Mon, Sep 25, 2017 at 2:16 PM, Daniel Borkmann [off-list ref] wrote:

On 09/25/2017 07:13 PM, Cong Wang wrote:

quoted

        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.

quoted

                __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.