Multicast routing stops functioning after 4G multicast packets recived.

Quoted 12 lines · Hannes Frederic Sowa

Quoted 11 lines · Bob Falken

Hello,

I patched, kernel 3.2.53 yesterday,
8774002632packets, and ~9.1TB later, the multicast routing seems to function properly. :)

Kudos for fixing this.

I will keep checking the next days.

As for IPv6 MR, my current setup i.e: the Multicast source, does not support IPv6, so cannot do a check for that natively.

Quoted 7 lines

Quoted 1 line

Unless I can translate IPv4 multicast into IPv6 multicast easily, using some iptable prerouting rules(?).

Thank you for testing!

I'll review the RCU regions again and will prepare the patches (before introduction of ebc0ffae5dfb44 ("fib: RCU conversion of fib_lookup()") and after.

It seems ip(6)mr_fib_lookup is not always called from rcu section (ndo_start_xmit), so I had to restructure a bit. Could you retest this patch as preparation for a submission to stable? Thanks!

RCU conversion can be done later then.

Hello,
Testing this patch as im typing this. will check status in about 12hours.
Unfortuantly, I dont have any receivers avaialble for requesting the multicast stream on the edge point anymore. So there is not TX traffic a.t.m..

I will have a better test-lab available next week. (hopefully).

Ok, so I am proposing this patch. Only difference from the RFC is that I removed the superfluous arg.rule NULL-pointer checks (I hate if they are superfluous and they always seem to spread ;) ).

Maybe you could test this one instead and David could pick it up as soon as your results are in.

I'll also look for the stable kernels where FIB_LOOKUP_NOREF is not yet available.

Thank you,

Hannes

[PATCH net] net: avoid reference counter overflows on fib_rules in multicast forwarding

When introducing multiple table support for multicast forwarding in IPv4 and IPv6, necessary fib_rules_put reference count decrements were forgotten.

Bob Falken reported that after 4G packets, multicast forwarding stopped working. This was because of a rule reference counter overflow which freed the rule as soon as the overflow happend.

So, use FIB_LOOKUP_NOREF if we are already in a RCU protected section and correctly deal with reference counter if not (called from ndo_start_xmit).

Fixes f0ad0860d01e47 ("ipv4: ipmr: support multiple tables")

Fixes d1db275dd3f6e4 ("ipv6: ip6mr: support multiple tables")

Reported-by Bob Falken <[email protected]>

Cc Patrick McHardy <[email protected]>

Cc Thomas Graf <[email protected]>

Cc Julian Anastasov <[email protected]>

Cc Eric Dumazet <[email protected]>

Signed-off-by Hannes Frederic Sowa <[email protected]>

---
 net/ipv4/ipmr.c  | 23 +++++++++++++++++------
 net/ipv6/ip6mr.c | 21 +++++++++++++++------
 2 files changed, 32 insertions(+), 12 deletions(-)

net/ipv4/ipmr.c +12 -3

index 421a249..c8d0857 100644
@@ -157,9 +157,12 @@ static struct mr_table *ipmr_get_table(struct net *net, u32 id)
 static int ipmr_fib_lookup(struct net *net, struct flowi4 *flp4,
 			   struct mr_table **mrt)
 {
-	struct ipmr_result res;
-	struct fib_lookup_arg arg = { .result = &res, };
 	int err;
+	struct ipmr_result res;
+	struct fib_lookup_arg arg = {
+		.result = &res,
+		.flags = FIB_LOOKUP_NOREF,
+	};
 
 	err = fib_rules_lookup(net->ipv4.mr_rules_ops,
 			       flowi4_to_flowi(flp4), 0, &arg);

@@ -448,16 +451,22 @@ failure:
 
 static netdev_tx_t reg_vif_xmit(struct sk_buff *skb, struct net_device *dev)
 {
+	int err;
+	struct ipmr_result res;
 	struct net *net = dev_net(dev);
-	struct mr_table *mrt;
+
+	struct fib_lookup_arg arg = {
+		.result = &res,
+	};
+

net/ipv4/ipmr.c +0 -0

 	return NETDEV_TX_OK;
 }
 

net/ipv6/ip6mr.c +7 -3

index f365310..38347a3 100644
@@ -141,9 +141,12 @@ static struct mr6_table *ip6mr_get_table(struct net *net, u32 id)
 static int ip6mr_fib_lookup(struct net *net, struct flowi6 *flp6,
 			    struct mr6_table **mrt)
 {
-	struct ip6mr_result res;
-	struct fib_lookup_arg arg = { .result = &res, };
 	int err;
+	struct ip6mr_result res;
+	struct fib_lookup_arg arg = {
+		.result = &res,
+		.flags = FIB_LOOKUP_NOREF,
+	};
 
 	err = fib_rules_lookup(net->ipv6.mr6_rules_ops,
 			       flowi6_to_flowi(flp6), 0, &arg);

@@ -693,16 +696,20 @@ static const struct inet6_protocol pim6_protocol = {
 static netdev_tx_t reg_vif_xmit(struct sk_buff *skb,
 				      struct net_device *dev)
 {
+	int err;
+	struct ip6mr_result res;
 	struct net *net = dev_net(dev);
-	struct mr6_table *mrt;
 	struct flowi6 fl6 = {
 		.flowi6_oif	= dev->ifindex,
 		.flowi6_iif	= skb->skb_iif,

net/ipv6/ip6mr.c +3 -1

 	if (err < 0) {
 		kfree_skb(skb);
 		return err;

@@ -711,9 +718,11 @@ static netdev_tx_t reg_vif_xmit(struct sk_buff *skb,
 	read_lock(&mrt_lock);
 	dev->stats.tx_bytes += skb->len;
 	dev->stats.tx_packets++;
-	ip6mr_cache_report(mrt, skb, mrt->mroute_reg_vif_num, MRT6MSG_WHOLEPKT);
+	ip6mr_cache_report(res.mrt, skb, res.mrt->mroute_reg_vif_num,
+			   MRT6MSG_WHOLEPKT);
 	read_unlock(&mrt_lock);
 	kfree_skb(skb);
+	fib_rule_put(arg.rule);
 	return NETDEV_TX_OK;
 }
 

Its not clear to me why you expand ipmr_fib_lookup()

Is there something wrong with existing code ?

Quoted 3 lines · Eric Dumazet

Its not clear to me why you expand ipmr_fib_lookup()

Is there something wrong with existing code ?

There are three users of ipmr_fib_lookup, two of them are in rcu_read_lock section, one is not.

ipmr_fib_lookup does not pass down arg.rule reference, so I don't have a chance to call fib_rule_put(arg.rule) on it. Thus I left ipmr_fib_lookup, just adding FIB_LOOKUP_NOREF and expanding ipmr_fib_lookup into the other function so I still have access to arg.rule to decrement the reference counter.

Do you agree?

Quoted 8 lines · Hannes Frederic Sowa

Quoted 3 lines · Eric Dumazet

Its not clear to me why you expand ipmr_fib_lookup()

Is there something wrong with existing code ?

There are three users of ipmr_fib_lookup, two of them are in rcu_read_lock section, one is not.

Quoted 7 lines

ipmr_fib_lookup does not pass down arg.rule reference, so I don't have a chance to call fib_rule_put(arg.rule) on it. Thus I left ipmr_fib_lookup, just adding FIB_LOOKUP_NOREF and expanding ipmr_fib_lookup into the other function so I still have access to arg.rule to decrement the reference counter.

Do you agree?

Hmm, I see the problem now.

What about adding a parameter to ipmr_fib_lookup(), to keep its spirit ?

ipmr_fib_lookup(net, &fl4, &mrt);
->
ipmr_fib_lookup(net, &fl4, &mrt, &rule);

Since ipmr_rt_fib_lookup() has the same rule leak, no ?

Its a bit late here, so maybe following is just stupid :
Cant we do the fib_rule_put() inside ipmr_fib_lookup() ?

Quoted 26 lines · Eric Dumazet

Quoted 8 lines · Hannes Frederic Sowa

Quoted 3 lines · Eric Dumazet

Its not clear to me why you expand ipmr_fib_lookup()

Is there something wrong with existing code ?

There are three users of ipmr_fib_lookup, two of them are in rcu_read_lock section, one is not.

Quoted 7 lines

ipmr_fib_lookup does not pass down arg.rule reference, so I don't have a chance to call fib_rule_put(arg.rule) on it. Thus I left ipmr_fib_lookup, just adding FIB_LOOKUP_NOREF and expanding ipmr_fib_lookup into the other function so I still have access to arg.rule to decrement the reference counter.

Do you agree?

Hmm, I see the problem now.

What about adding a parameter to ipmr_fib_lookup(), to keep its spirit ?

ipmr_fib_lookup(net, &fl4, &mrt);
->
ipmr_fib_lookup(net, &fl4, &mrt, &rule);

Quoted 1 line

Since ipmr_rt_fib_lookup() has the same rule leak, no ?

No, ipmr_rt_fib_lookup is fine. This function gets called only from rcu read locked section and we don't take table reference because of FIB_LOOKUP_NOREF, so we don't need to put reference counter on arg.table.

We could add the additional argument, just ignoring it in ipmr_rt_fib_lookup.

Quoted 3 lines

Its a bit late here, so maybe following is just stupid :
Cant we do the fib_rule_put() inside ipmr_fib_lookup() ?

We could add bool noref to ipmr_fib_lookup indicating we want to drop reference to rule just after lookup.

I'll check if freeing a rule has additional side-effects on dependencies in reg_vif_xmit. That would be a nice solution actually, thanks!

Quoted 8 lines

Quoted 2 lines

Its a bit late here, so maybe following is just stupid :
Cant we do the fib_rule_put() inside ipmr_fib_lookup() ?

We could add bool noref to ipmr_fib_lookup indicating we want to drop reference to rule just after lookup.

I'll check if freeing a rule has additional side-effects on dependencies in reg_vif_xmit. That would be a nice solution actually, thanks!

Hmm, rule holds a reference to the net namespace in use. I don't know if we want to add this special case. I guess net-namespace reference cannot be removed while processing ndo_start_xmit callback but I don't like this special case somehow. But I guess it is possible.

Your opinion on that?

Hm, Eric. If we do that we can just specifiy FIB_LOOKUP_NOREF unconditionally. FIB_LOOKUP_NOREF has no other side effects on a ipmr lookup as taking the reference on the rule, which we would drop after that.

So we would actually be going back to the first patch in this thread. I guess it is just a matter of style?

Bob Falken reported that after 4G packets, multicast forwarding stopped working. This was because of a rule reference counter overflow which freed the rule as soon as the overflow happend.

This patch solves this by adding the FIB_LOOKUP_NOREF flag to fib_rules_lookup calls. This is safe even from non-rcu locked sections as in this case the flag only implies not taking a reference to the rule, which we don't need at all.

Rules only hold references to the namespace, which are guaranteed to be available during the call of the non-rcu protected function reg_vif_xmit because of the interface reference which itself holds a reference to the net namespace.

Fixes f0ad0860d01e47 ("ipv4: ipmr: support multiple tables")

Fixes d1db275dd3f6e4 ("ipv6: ip6mr: support multiple tables")

Reported-by Bob Falken <[email protected]>

Cc Patrick McHardy <[email protected]>

Cc Thomas Graf <[email protected]>

Cc Julian Anastasov <[email protected]>

Cc Eric Dumazet <[email protected]>

Signed-off-by Hannes Frederic Sowa <[email protected]>

---
Bob Falken already tested this patch, as it is similar to my first attempt but the additional and similar fix for ipv6.

We need an additional fix for kernels without FIB_LOOKUP_NOREF, but I'll move that to tomorrow, as it is already late here.

Quoted 4 lines · Hannes Frederic Sowa

Bob Falken reported that after 4G packets, multicast forwarding stopped working. This was because of a rule reference counter overflow which freed the rule as soon as the overflow happend.

Quoted 4 lines

This patch solves this by adding the FIB_LOOKUP_NOREF flag to fib_rules_lookup calls. This is safe even from non-rcu locked sections as in this case the flag only implies not taking a reference to the rule, which we don't need at all.

We need to not forget this when/if we remove FIB_LOOKUP_NOREF flag, as all callers use it : We'll have to keep rcu_read_lock() in fib_rules_lookup()

Quoted 17 lines

Rules only hold references to the namespace, which are guaranteed to be available during the call of the non-rcu protected function reg_vif_xmit because of the interface reference which itself holds a reference to the net namespace.

Fixes f0ad0860d01e47 ("ipv4: ipmr: support multiple tables")

Fixes d1db275dd3f6e4 ("ipv6: ip6mr: support multiple tables")

Reported-by Bob Falken <[email protected]>

Cc Patrick McHardy <[email protected]>

Cc Thomas Graf <[email protected]>

Cc Julian Anastasov <[email protected]>

Cc Eric Dumazet <[email protected]>

Signed-off-by Hannes Frederic Sowa <[email protected]>

---
Bob Falken already tested this patch, as it is similar to my first attempt but the additional and similar fix for ipv6.

Quoted 2 lines

We need an additional fix for kernels without FIB_LOOKUP_NOREF, but I'll move that to tomorrow, as it is already late here.

Acked-by Eric Dumazet <[email protected]>

Bob Falken reported that after 4G packets, multicast forwarding stopped working. This was because of a rule reference counter overflow which freed the rule as soon as the overflow happend.

This patch solves this by adding the FIB_LOOKUP_NOREF flag to fib_rules_lookup calls. This is safe even from non-rcu locked sections as in this case the flag only implies not taking a reference to the rule, which we don't need at all.

Rules only hold references to the namespace, which are guaranteed to be available during the call of the non-rcu protected function reg_vif_xmit because of the interface reference which itself holds a reference to the net namespace.

Fixes f0ad0860d01e47 ("ipv4: ipmr: support multiple tables")

Fixes d1db275dd3f6e4 ("ipv6: ip6mr: support multiple tables")

Reported-by Bob Falken <[email protected]>

Cc Patrick McHardy <[email protected]>

Cc Thomas Graf <[email protected]>

Cc Julian Anastasov <[email protected]>

Cc Eric Dumazet <[email protected]>

Signed-off-by Hannes Frederic Sowa <[email protected]>