[PATCH net-next] net, cls: allow for deleting all filters for given parent

[PATCH net-next] net, cls: allow for deleting all filters for given parent

[Daniel Borkmann]

Add a possibility where the user can just specify the parent and
all filters under that parent are then being purged. Currently,
for example for scripting, one needs to specify pref/prio to have
a well-defined number for 'tc filter del' command for addressing
the previously created instance or additionally filter handle in
case of priorities being the same. Improve usage by allowing the
option for tc to specify the parent and removing the whole chain
for that given parent.

Example usage after patch, no tc changes required:

  # tc qdisc replace dev foo clsact
  # tc filter add dev foo egress bpf da obj ./bpf.o
  # tc filter add dev foo egress bpf da obj ./bpf.o
  # tc filter show dev foo egress
  filter protocol all pref 49151 bpf
  filter protocol all pref 49151 bpf handle 0x1 bpf.o:[classifier] direct-action
  filter protocol all pref 49152 bpf
  filter protocol all pref 49152 bpf handle 0x1 bpf.o:[classifier] direct-action
  # tc filter del dev foo egress
  # tc filter show dev foo egress
  #

Previously, RTM_DELTFILTER requests with invalid prio of 0 were
rejected, so only netlink requests with RTM_NEWTFILTER and NLM_F_CREATE
flag were allowed where the kernel would auto-generate a pref/prio.
We can piggyback on that and use prio of 0 as a wildcard for
requests of RTM_DELTFILTER.

Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
---
 net/sched/cls_api.c | 24 ++++++++++++++++++++----
 1 file changed, 20 insertions(+), 4 deletions(-)

diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index a75864d..caa1dd4 100644
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c
@@ -156,11 +156,22 @@ replay:
 	cl = 0;
 
 	if (prio == 0) {
-		/* If no priority is given, user wants we allocated it. */
-		if (n->nlmsg_type != RTM_NEWTFILTER ||
-		    !(n->nlmsg_flags & NLM_F_CREATE))
+		switch (n->nlmsg_type) {
+		case RTM_DELTFILTER:
+			if (protocol || t->tcm_handle)
+				return -ENOENT;
+			break;
+		case RTM_NEWTFILTER:
+			/* If no priority is provided by the user,
+			 * we allocate one.
+			 */
+			if (n->nlmsg_flags & NLM_F_CREATE) {
+				prio = TC_H_MAKE(0x80000000U, 0U);
+				break;
+			}
+		default:
 			return -ENOENT;
-		prio = TC_H_MAKE(0x80000000U, 0U);
+		}
 	}
 
 	/* Find head of filter chain. */
@@ -200,6 +211,11 @@ replay:
 	err = -EINVAL;
 	if (chain == NULL)
 		goto errout;
+	if (n->nlmsg_type == RTM_DELTFILTER && prio == 0) {
+		tcf_destroy_chain(chain);
+		err = 0;
+		goto errout;
+	}
 
 	/* Check the chain for existence of proto-tcf with this priority */
 	for (back = chain;
-- 
1.9.3

Re: [PATCH net-next] net, cls: allow for deleting all filters for given parent

[Jamal Hadi Salim]

On 16-06-04 12:24 PM, Daniel Borkmann wrote:
> Add a possibility where the user can just specify the parent and
> all filters under that parent are then being purged. Currently,
> for example for scripting, one needs to specify pref/prio to have
> a well-defined number for 'tc filter del' command for addressing
> the previously created instance or additionally filter handle in
> case of priorities being the same. Improve usage by allowing the
> option for tc to specify the parent and removing the whole chain
> for that given parent.
>
> Example usage after patch, no tc changes required:
>
>    # tc qdisc replace dev foo clsact
>    # tc filter add dev foo egress bpf da obj ./bpf.o
>    # tc filter add dev foo egress bpf da obj ./bpf.o
>    # tc filter show dev foo egress
>    filter protocol all pref 49151 bpf
>    filter protocol all pref 49151 bpf handle 0x1 bpf.o:[classifier] direct-action
>    filter protocol all pref 49152 bpf
>    filter protocol all pref 49152 bpf handle 0x1 bpf.o:[classifier] direct-action
>    # tc filter del dev foo egress
>    # tc filter show dev foo egress
>    #
>
> Previously, RTM_DELTFILTER requests with invalid prio of 0 were
> rejected, so only netlink requests with RTM_NEWTFILTER and NLM_F_CREATE
> flag were allowed where the kernel would auto-generate a pref/prio.
> We can piggyback on that and use prio of 0 as a wildcard for
> requests of RTM_DELTFILTER.
>
> Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>


Looks good to me.
Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>

cheers,
jamal

Re: [PATCH net-next] net, cls: allow for deleting all filters for given parent

[Sergei Shtylyov]

Hello.

On 6/4/2016 7:24 PM, Daniel Borkmann wrote:

> Add a possibility where the user can just specify the parent and
> all filters under that parent are then being purged. Currently,
> for example for scripting, one needs to specify pref/prio to have
> a well-defined number for 'tc filter del' command for addressing
> the previously created instance or additionally filter handle in
> case of priorities being the same. Improve usage by allowing the
> option for tc to specify the parent and removing the whole chain
> for that given parent.
>
> Example usage after patch, no tc changes required:
>
>   # tc qdisc replace dev foo clsact
>   # tc filter add dev foo egress bpf da obj ./bpf.o
>   # tc filter add dev foo egress bpf da obj ./bpf.o
>   # tc filter show dev foo egress
>   filter protocol all pref 49151 bpf
>   filter protocol all pref 49151 bpf handle 0x1 bpf.o:[classifier] direct-action
>   filter protocol all pref 49152 bpf
>   filter protocol all pref 49152 bpf handle 0x1 bpf.o:[classifier] direct-action
>   # tc filter del dev foo egress
>   # tc filter show dev foo egress
>   #
>
> Previously, RTM_DELTFILTER requests with invalid prio of 0 were
> rejected, so only netlink requests with RTM_NEWTFILTER and NLM_F_CREATE
> flag were allowed where the kernel would auto-generate a pref/prio.
> We can piggyback on that and use prio of 0 as a wildcard for
> requests of RTM_DELTFILTER.
>
> Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
> ---
>  net/sched/cls_api.c | 24 ++++++++++++++++++++----
>  1 file changed, 20 insertions(+), 4 deletions(-)
>
> diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
> index a75864d..caa1dd4 100644
> --- a/net/sched/cls_api.c
> +++ b/net/sched/cls_api.c
> @@ -156,11 +156,22 @@ replay:
>  	cl = 0;
>
>  	if (prio == 0) {
> -		/* If no priority is given, user wants we allocated it. */
> -		if (n->nlmsg_type != RTM_NEWTFILTER ||
> -		    !(n->nlmsg_flags & NLM_F_CREATE))
> +		switch (n->nlmsg_type) {
> +		case RTM_DELTFILTER:
> +			if (protocol || t->tcm_handle)
> +				return -ENOENT;
> +			break;
> +		case RTM_NEWTFILTER:
> +			/* If no priority is provided by the user,
> +			 * we allocate one.
> +			 */
> +			if (n->nlmsg_flags & NLM_F_CREATE) {
> +				prio = TC_H_MAKE(0x80000000U, 0U);
> +				break;
> +			}

    Need a comment here, something like /* FALL THRU */.

> +		default:
>  			return -ENOENT;
> -		prio = TC_H_MAKE(0x80000000U, 0U);
> +		}
>  	}
>
>  	/* Find head of filter chain. */
[...]

MBR, Sergei

Re: [PATCH net-next] net, cls: allow for deleting all filters for given parent

[Daniel Borkmann]

On 06/05/2016 02:53 PM, Sergei Shtylyov wrote:
> On 6/4/2016 7:24 PM, Daniel Borkmann wrote:
[...]
>
>     Need a comment here, something like /* FALL THRU */.

Hmm, not really, I think it's obvious enough.

>> +        default:
>>              return -ENOENT;
>> -        prio = TC_H_MAKE(0x80000000U, 0U);
>> +        }
>>      }
>>
>>      /* Find head of filter chain. */
> [...]
>
> MBR, Sergei
>

Re: [PATCH net-next] net, cls: allow for deleting all filters for given parent

[Sergei Shtylyov]

On 06/05/2016 08:47 PM, Daniel Borkmann wrote:

>> On 6/4/2016 7:24 PM, Daniel Borkmann wrote:
> [...]
>>
>>     Need a comment here, something like /* FALL THRU */.
>
> Hmm, not really, I think it's obvious enough.

    Well, at least the *switch* example in the CodingStyle chapter 1 has such 
comment (though there's no explicit statement on this matter).

>>> +        default:
>>>              return -ENOENT;
>>> -        prio = TC_H_MAKE(0x80000000U, 0U);
>>> +        }
>>>      }
[...]

MBR, Sergei

Re: [PATCH net-next] net, cls: allow for deleting all filters for given parent

[Cong Wang]

On Sat, Jun 4, 2016 at 9:24 AM, Daniel Borkmann <daniel@iogearbox.net> wrote:
> +       if (n->nlmsg_type == RTM_DELTFILTER && prio == 0) {
> +               tcf_destroy_chain(chain);
> +               err = 0;
> +               goto errout;
> +       }

We need to notify users we removed which filters, right?

Re: [PATCH net-next] net, cls: allow for deleting all filters for given parent

[Daniel Borkmann]

On 06/06/2016 07:12 PM, Cong Wang wrote:
> On Sat, Jun 4, 2016 at 9:24 AM, Daniel Borkmann <daniel@iogearbox.net> wrote:
>> +       if (n->nlmsg_type == RTM_DELTFILTER && prio == 0) {
>> +               tcf_destroy_chain(chain);
>> +               err = 0;
>> +               goto errout;
>> +       }
>
> We need to notify users we removed which filters, right?

As far as I know, most such use cases that listen on this are bypasses
that mirror kernel configs from user space ... but well, sure, I can add
a notification if people care. Would do this as a separate patch.

Looking into this, I would probably make this a single notification that
denotes this 'wild-card' removal for that parent instead of calling
tfilter_notify() for each filter separately (which allocs skb, dumps it,
etc), qdisc del doesn't loop through it either, so probably fine this way.

Thanks,
Daniel

Re: [PATCH net-next] net, cls: allow for deleting all filters for given parent

[Cong Wang]

On Mon, Jun 6, 2016 at 12:25 PM, Daniel Borkmann <daniel@iogearbox.net> wrote:
> On 06/06/2016 07:12 PM, Cong Wang wrote:
>>
>> On Sat, Jun 4, 2016 at 9:24 AM, Daniel Borkmann <daniel@iogearbox.net>
>> wrote:
>>>
>>> +       if (n->nlmsg_type == RTM_DELTFILTER && prio == 0) {
>>> +               tcf_destroy_chain(chain);
>>> +               err = 0;
>>> +               goto errout;
>>> +       }
>>
>>
>> We need to notify users we removed which filters, right?
>
>
> As far as I know, most such use cases that listen on this are bypasses
> that mirror kernel configs from user space ... but well, sure, I can add
> a notification if people care. Would do this as a separate patch.

This is fundamental for libnl to update caches.

I don't understand why it should be separated, since notification is
not a feature, we already have notifications in other paths.

>
> Looking into this, I would probably make this a single notification that
> denotes this 'wild-card' removal for that parent instead of calling
> tfilter_notify() for each filter separately (which allocs skb, dumps it,
> etc), qdisc del doesn't loop through it either, so probably fine this way.

Makes sense.

Thanks.

Re: [PATCH net-next] net, cls: allow for deleting all filters for given parent

[Daniel Borkmann]

On 06/06/2016 09:52 PM, Cong Wang wrote:
> On Mon, Jun 6, 2016 at 12:25 PM, Daniel Borkmann <daniel@iogearbox.net> wrote:
>> On 06/06/2016 07:12 PM, Cong Wang wrote:
>>>
>>> On Sat, Jun 4, 2016 at 9:24 AM, Daniel Borkmann <daniel@iogearbox.net>
>>> wrote:
>>>>
>>>> +       if (n->nlmsg_type == RTM_DELTFILTER && prio == 0) {
>>>> +               tcf_destroy_chain(chain);
>>>> +               err = 0;
>>>> +               goto errout;
>>>> +       }
>>>
>>> We need to notify users we removed which filters, right?
>>
>> As far as I know, most such use cases that listen on this are bypasses
>> that mirror kernel configs from user space ... but well, sure, I can add
>> a notification if people care. Would do this as a separate patch.
>
> This is fundamental for libnl to update caches.

I see, makes sense then. Thanks!

> I don't understand why it should be separated, since notification is
> not a feature, we already have notifications in other paths.
>
>> Looking into this, I would probably make this a single notification that
>> denotes this 'wild-card' removal for that parent instead of calling
>> tfilter_notify() for each filter separately (which allocs skb, dumps it,
>> etc), qdisc del doesn't loop through it either, so probably fine this way.
>
> Makes sense.
>
> Thanks.
>

Re: [PATCH net-next] net, cls: allow for deleting all filters for given parent

[Daniel Borkmann]

On 06/06/2016 09:52 PM, Cong Wang wrote:
> On Mon, Jun 6, 2016 at 12:25 PM, Daniel Borkmann <daniel@iogearbox.net> wrote:
[...]
> This is fundamental for libnl to update caches.
>
> I don't understand why it should be separated, since notification is
> not a feature, we already have notifications in other paths.
>
>> Looking into this, I would probably make this a single notification that
>> denotes this 'wild-card' removal for that parent instead of calling
>> tfilter_notify() for each filter separately (which allocs skb, dumps it,
>> etc), qdisc del doesn't loop through it either, so probably fine this way.
>
> Makes sense.

I've been playing around with both options and am actually currently
leaning towards the tfilter_notify() for each proto for the reason
that user space tc monitors can simply stay as is. F.e., if someone
keeps an older libnl binary that wouldn't understand such a wildcard
message, then elements in libnl cache won't receive updates since the
meta data won't match on them (average case, there probably are only
one up to a handful of classifiers per parent) ... hm, different topic
but still wondering whether libnl relying on such messages is a good
idea in general since under stress tfilter_notify() can also fail and
user space won't get the updates (except for queries with RTM_GETTFILTER).

Thanks,
Daniel

  net/sched/cls_api.c | 37 +++++++++++++++++++++++++++++++++----
  1 file changed, 33 insertions(+), 4 deletions(-)

diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index a75864d..f873bbc 100644
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c
@@ -103,6 +103,17 @@ static int tfilter_notify(struct net *net, struct sk_buff *oskb,
  			  struct nlmsghdr *n, struct tcf_proto *tp,
  			  unsigned long fh, int event);

+static void tfilter_notify_chain(struct net *net, struct sk_buff *oskb,
+				 struct nlmsghdr *n,
+				 struct tcf_proto __rcu **chain, int event)
+{
+	struct tcf_proto __rcu **it_chain;
+	struct tcf_proto *tp;
+
+	for (it_chain = chain; (tp = rtnl_dereference(*it_chain)) != NULL;
+	     it_chain = &tp->next)
+		tfilter_notify(net, oskb, n, tp, 0, event);
+}

  /* Select new prio value from the range, managed by kernel. */

@@ -156,11 +167,23 @@ replay:
  	cl = 0;

  	if (prio == 0) {
-		/* If no priority is given, user wants we allocated it. */
-		if (n->nlmsg_type != RTM_NEWTFILTER ||
-		    !(n->nlmsg_flags & NLM_F_CREATE))
+		switch (n->nlmsg_type) {
+		case RTM_DELTFILTER:
+			if (protocol || t->tcm_handle)
+				return -ENOENT;
+			break;
+		case RTM_NEWTFILTER:
+			/* If no priority is provided by the user,
+			 * we allocate one.
+			 */
+			if (n->nlmsg_flags & NLM_F_CREATE) {
+				prio = TC_H_MAKE(0x80000000U, 0U);
+				break;
+			}
+			/* fall-through */
+		default:
  			return -ENOENT;
-		prio = TC_H_MAKE(0x80000000U, 0U);
+		}
  	}

  	/* Find head of filter chain. */
@@ -200,6 +223,12 @@ replay:
  	err = -EINVAL;
  	if (chain == NULL)
  		goto errout;
+	if (n->nlmsg_type == RTM_DELTFILTER && prio == 0) {
+		tfilter_notify_chain(net, skb, n, chain, RTM_DELTFILTER);
+		tcf_destroy_chain(chain);
+		err = 0;
+		goto errout;
+	}

  	/* Check the chain for existence of proto-tcf with this priority */
  	for (back = chain;
-- 
1.9.3

Re: [PATCH net-next] net, cls: allow for deleting all filters for given parent

[Cong Wang]

On Wed, Jun 8, 2016 at 2:30 PM, Daniel Borkmann <daniel@iogearbox.net> wrote:
> On 06/06/2016 09:52 PM, Cong Wang wrote:
>>
>> On Mon, Jun 6, 2016 at 12:25 PM, Daniel Borkmann <daniel@iogearbox.net>
>> wrote:
>
> [...]
>>
>> This is fundamental for libnl to update caches.
>>
>> I don't understand why it should be separated, since notification is
>> not a feature, we already have notifications in other paths.
>>
>>> Looking into this, I would probably make this a single notification that
>>> denotes this 'wild-card' removal for that parent instead of calling
>>> tfilter_notify() for each filter separately (which allocs skb, dumps it,
>>> etc), qdisc del doesn't loop through it either, so probably fine this
>>> way.
>>
>>
>> Makes sense.
>
>
> I've been playing around with both options and am actually currently
> leaning towards the tfilter_notify() for each proto for the reason
> that user space tc monitors can simply stay as is. F.e., if someone
> keeps an older libnl binary that wouldn't understand such a wildcard
> message, then elements in libnl cache won't receive updates since the
> meta data won't match on them (average case, there probably are only
> one up to a handful of classifiers per parent) ... hm, different topic
> but still wondering whether libnl relying on such messages is a good
> idea in general since under stress tfilter_notify() can also fail and
> user space won't get the updates (except for queries with RTM_GETTFILTER).

Don't worry about any failure in tfilter_notify(), we call it in
non-wildcard removal too. Your current patch looks good to me.

Thanks!

Re: [PATCH net-next] net, cls: allow for deleting all filters for given parent

[Jamal Hadi Salim]

On 16-06-09 06:54 PM, Cong Wang wrote:
> On Wed, Jun 8, 2016 at 2:30 PM, Daniel Borkmann <daniel@iogearbox.net> wrote:
>> On 06/06/2016 09:52 PM, Cong Wang wrote:
>>>
>>> On Mon, Jun 6, 2016 at 12:25 PM, Daniel Borkmann <daniel@iogearbox.net>
>>> wrote:
>>

>> I've been playing around with both options and am actually currently
>> leaning towards the tfilter_notify() for each proto for the reason
>> that user space tc monitors can simply stay as is. F.e., if someone
>> keeps an older libnl binary that wouldn't understand such a wildcard
>> message, then elements in libnl cache won't receive updates since the
>> meta data won't match on them (average case, there probably are only
>> one up to a handful of classifiers per parent) ... hm, different topic
>> but still wondering whether libnl relying on such messages is a good
>> idea in general since under stress tfilter_notify() can also fail and
>> user space won't get the updates (except for queries with RTM_GETTFILTER).
>
> Don't worry about any failure in tfilter_notify(), we call it in
> non-wildcard removal too. Your current patch looks good to me.
>
> Thanks!
>

Daniel, I acked this already (sorry I missed the event notification
bits).

Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>

cheers,
jamal