diff mbox

[net] net_sched: do not broadcast RTM_GETTFILTER result

Message ID 1476069955.28155.292.camel@edumazet-glaptop3.roam.corp.google.com
State Accepted, archived
Delegated to: David Miller
Headers show

Commit Message

Eric Dumazet Oct. 10, 2016, 3:25 a.m. UTC 
From: Eric Dumazet <edumazet@google.com>

There are two ways to get tc filters from kernel to user space.

1) Full dump (tc_dump_tfilter())
2) RTM_GETTFILTER to get one precise filter, reducing overhead.

The second operation is unfortunately broadcasting its result,
polluting "tc monitor" users.

This patch makes sure only the requester gets the result, using
netlink_unicast() instead of rtnetlink_send()

Jamal cooked an iproute2 patch to implement "tc filter get" operation,
but other user space libraries already use RTM_GETTFILTER when a single
filter is queried, instead of dumping all filters.

Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: Jamal Hadi Salim <jhs@mojatatu.com>
---
 net/sched/cls_api.c |   18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

Comments

Jamal Hadi Salim Oct. 10, 2016, 1:16 p.m. UTC | #1 
On 16-10-09 11:25 PM, Eric Dumazet wrote:
> From: Eric Dumazet <edumazet@google.com>
>
> There are two ways to get tc filters from kernel to user space.
>
> 1) Full dump (tc_dump_tfilter())
> 2) RTM_GETTFILTER to get one precise filter, reducing overhead.
>
> The second operation is unfortunately broadcasting its result,
> polluting "tc monitor" users.
>
> This patch makes sure only the requester gets the result, using
> netlink_unicast() instead of rtnetlink_send()
>
> Jamal cooked an iproute2 patch to implement "tc filter get" operation,
> but other user space libraries already use RTM_GETTFILTER when a single
> filter is queried, instead of dumping all filters.
>
> Signed-off-by: Eric Dumazet <edumazet@google.com>

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

I will send the iproute2 patch

cheers,
jamal
Cong Wang Oct. 12, 2016, 4:36 p.m. UTC | #2 
On Sun, Oct 9, 2016 at 8:25 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> +       if (unicast)
> +               return netlink_unicast(net->rtnl, skb, portid, MSG_DONTWAIT);

Nit: rtnl_unicast() is simpler.
Eric Dumazet Oct. 13, 2016, 7:46 a.m. UTC | #3 
On Wed, 2016-10-12 at 09:36 -0700, Cong Wang wrote:
> On Sun, Oct 9, 2016 at 8:25 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> > +       if (unicast)
> > +               return netlink_unicast(net->rtnl, skb, portid, MSG_DONTWAIT);
> 
> Nit: rtnl_unicast() is simpler.

I copied code in rtnetlink_send(), I guess we could use rtnl_unicast()
there as well.
Jamal Hadi Salim Oct. 13, 2016, 11:54 a.m. UTC | #4 
On 16-10-13 03:46 AM, Eric Dumazet wrote:
> On Wed, 2016-10-12 at 09:36 -0700, Cong Wang wrote:
>> On Sun, Oct 9, 2016 at 8:25 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>>> +       if (unicast)
>>> +               return netlink_unicast(net->rtnl, skb, portid, MSG_DONTWAIT);
>>
>> Nit: rtnl_unicast() is simpler.
>
> I copied code in rtnetlink_send(), I guess we could use rtnl_unicast()
> there as well.

I would toss a coin and if it lands on tail then make the change ;->
(probably in a separate patch).

cheers,
jamal
David Miller Oct. 13, 2016, 1:54 p.m. UTC | #5 
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Sun, 09 Oct 2016 20:25:55 -0700

> From: Eric Dumazet <edumazet@google.com>
> 
> There are two ways to get tc filters from kernel to user space.
> 
> 1) Full dump (tc_dump_tfilter())
> 2) RTM_GETTFILTER to get one precise filter, reducing overhead.
> 
> The second operation is unfortunately broadcasting its result,
> polluting "tc monitor" users.
> 
> This patch makes sure only the requester gets the result, using
> netlink_unicast() instead of rtnetlink_send()
> 
> Jamal cooked an iproute2 patch to implement "tc filter get" operation,
> but other user space libraries already use RTM_GETTFILTER when a single
> filter is queried, instead of dumping all filters.
> 
> Signed-off-by: Eric Dumazet <edumazet@google.com>

Applied, thanks Eric.

Want me to queue this up for -stable too?
Eric Dumazet Oct. 13, 2016, 9:08 p.m. UTC | #6 
On Thu, 2016-10-13 at 09:54 -0400, David Miller wrote:
> Applied, thanks Eric.
> 
> Want me to queue this up for -stable too?

No thanks, it just occurred to me during a debugging session.
diff mbox

Patch

diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index 11da7da0b7c4..2ee29a3375f6 100644
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c
@@ -101,7 +101,7 @@  EXPORT_SYMBOL(unregister_tcf_proto_ops);
 
 static int tfilter_notify(struct net *net, struct sk_buff *oskb,
 			  struct nlmsghdr *n, struct tcf_proto *tp,
-			  unsigned long fh, int event);
+			  unsigned long fh, int event, bool unicast);
 
 static void tfilter_notify_chain(struct net *net, struct sk_buff *oskb,
 				 struct nlmsghdr *n,
@@ -112,7 +112,7 @@  static void tfilter_notify_chain(struct net *net, struct sk_buff *oskb,
 
 	for (it_chain = chain; (tp = rtnl_dereference(*it_chain)) != NULL;
 	     it_chain = &tp->next)
-		tfilter_notify(net, oskb, n, tp, 0, event);
+		tfilter_notify(net, oskb, n, tp, 0, event, false);
 }
 
 /* Select new prio value from the range, managed by kernel. */
@@ -319,7 +319,8 @@  replay:
 
 			RCU_INIT_POINTER(*back, next);
 
-			tfilter_notify(net, skb, n, tp, fh, RTM_DELTFILTER);
+			tfilter_notify(net, skb, n, tp, fh,
+				       RTM_DELTFILTER, false);
 			tcf_destroy(tp, true);
 			err = 0;
 			goto errout;
@@ -345,14 +346,14 @@  replay:
 				struct tcf_proto *next = rtnl_dereference(tp->next);
 
 				tfilter_notify(net, skb, n, tp, fh,
-					       RTM_DELTFILTER);
+					       RTM_DELTFILTER, false);
 				if (tcf_destroy(tp, false))
 					RCU_INIT_POINTER(*back, next);
 			}
 			goto errout;
 		case RTM_GETTFILTER:
 			err = tfilter_notify(net, skb, n, tp, fh,
-					     RTM_NEWTFILTER);
+					     RTM_NEWTFILTER, true);
 			goto errout;
 		default:
 			err = -EINVAL;
@@ -367,7 +368,7 @@  replay:
 			RCU_INIT_POINTER(tp->next, rtnl_dereference(*back));
 			rcu_assign_pointer(*back, tp);
 		}
-		tfilter_notify(net, skb, n, tp, fh, RTM_NEWTFILTER);
+		tfilter_notify(net, skb, n, tp, fh, RTM_NEWTFILTER, false);
 	} else {
 		if (tp_created)
 			tcf_destroy(tp, true);
@@ -419,7 +420,7 @@  nla_put_failure:
 
 static int tfilter_notify(struct net *net, struct sk_buff *oskb,
 			  struct nlmsghdr *n, struct tcf_proto *tp,
-			  unsigned long fh, int event)
+			  unsigned long fh, int event, bool unicast)
 {
 	struct sk_buff *skb;
 	u32 portid = oskb ? NETLINK_CB(oskb).portid : 0;
@@ -433,6 +434,9 @@  static int tfilter_notify(struct net *net, struct sk_buff *oskb,
 		return -EINVAL;
 	}
 
+	if (unicast)
+		return netlink_unicast(net->rtnl, skb, portid, MSG_DONTWAIT);
+
 	return rtnetlink_send(skb, net, portid, RTNLGRP_TC,
 			      n->nlmsg_flags & NLM_F_ECHO);
 }