IPv6 multicast routing
diff mbox

Message ID 20090120200248.GA13928@boeing.com
State Accepted, archived
Delegated to: David Miller
Headers show

Commit Message

Tom Goff Jan. 20, 2009, 8:02 p.m. UTC
This patch addresses the IPv6 multicast routing issues described
below.  It was tested with XORP 1.4/1.5 as the IPv6 PIM-SM routing
daemon against FreeBSD peers.

net/ipv6/ip6_input.c:

  - Don't try to forward link-local multicast packets.

  - Don't reset skb2->dev before calling ip6_mr_input() so packets can
    be identified as coming from the PIM register vif properly.

net/ipv6/ip6mr.c:

  - Fix incoming PIM register messages processing:

    * The IPv6 pseudo-header should be included when checksumming PIM
      messages (RFC 4601 section 4.9; RFC 3973 section 4.7.1).

    * Packets decapsulated from PIM register messages should have
      skb->protocol ETH_P_IPV6.

  - Enable/disable IPv6 multicast forwarding on the corresponding
    interface when a routing daemon adds/removes a multicast virtual
    interface.

  - Remove incorrect skb_pull() to fix userspace signaling.

  - Enable/disable global IPv6 multicast forwarding when an IPv6
    multicast routing socket is opened/closed.

net/ipv6/route.c:

  - Don't use strict routing logic for packets decapsulated from PIM
    register messages (similar to disabling rp_filter for the IPv4
    case).

Signed-off-by: Thomas Goff <thomas.goff@boeing.com>
Reviewed-by: Fred Templin <fred.l.templin@boeing.com>

---
 net/ipv6/ip6_input.c |    2 +-
 net/ipv6/ip6mr.c     |   23 ++++++++++++++++++-----
 net/ipv6/route.c     |    2 +-
 3 files changed, 20 insertions(+), 7 deletions(-)

Comments

YOSHIFUJI Hideaki / 吉藤英明 Jan. 21, 2009, 1:04 a.m. UTC | #1
In article <20090120200248.GA13928@boeing.com> (at Tue, 20 Jan 2009 12:02:48 -0800), Tom Goff <thomas.goff@boeing.com> says:

> This patch addresses the IPv6 multicast routing issues described
> below.  It was tested with XORP 1.4/1.5 as the IPv6 PIM-SM routing
> daemon against FreeBSD peers.
:
>   - Enable/disable IPv6 multicast forwarding on the corresponding
>     interface when a routing daemon adds/removes a multicast virtual
>     interface.

Thank you for tha patch!

However, I believe configuring forwarding is user's responsibility
and I'm not in favor of managing mc_forwarding automatically.

If we go your way, we need to make the proc interface read-only, but
you know, they will become non-"conf" interface. :-(
And, well, we will introduce another possible overflow/underflow here...

Another thought was to allow non-VIF routing settings (or manual settings).

So, I decided to drop that bits.


Other fixes seem okay.

--yoshfuji
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tom Goff Jan. 22, 2009, 11:30 p.m. UTC | #2
On Wed, Jan 21, 2009 at 10:04:31AM +0900, YOSHIFUJI Hideaki / 吉藤英明 wrote:

> >   - Enable/disable IPv6 multicast forwarding on the corresponding
> >     interface when a routing daemon adds/removes a multicast virtual
> >     interface.
:
> However, I believe configuring forwarding is user's responsibility
> and I'm not in favor of managing mc_forwarding automatically.
>
> If we go your way, we need to make the proc interface read-only, but
> you know, they will become non-"conf" interface. :-(
> And, well, we will introduce another possible overflow/underflow here...
>
> Another thought was to allow non-VIF routing settings (or manual settings).
>
> So, I decided to drop that bits.

Thanks for looking it over.

I think your approach to handling mc_forwarding makes sense, although
it differs from the IPv4 behavior.  In any case, it looks like the
current IPv4 and IPv6 multicast routing code does not consider an
interface's mc_forwarding value at all.

  Tom
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller Jan. 26, 2009, 2:15 a.m. UTC | #3
From: "Goff, Thomas" <Thomas.Goff@boeing.com>
Date: Thu, 22 Jan 2009 15:30:38 -0800

> On Wed, Jan 21, 2009 at 10:04:31AM +0900, YOSHIFUJI Hideaki / 吉藤英明 wrote:
> 
> > >   - Enable/disable IPv6 multicast forwarding on the corresponding
> > >     interface when a routing daemon adds/removes a multicast virtual
> > >     interface.
> :
> > However, I believe configuring forwarding is user's responsibility
> > and I'm not in favor of managing mc_forwarding automatically.
> >
> > If we go your way, we need to make the proc interface read-only, but
> > you know, they will become non-"conf" interface. :-(
> > And, well, we will introduce another possible overflow/underflow here...
> >
> > Another thought was to allow non-VIF routing settings (or manual settings).
> >
> > So, I decided to drop that bits.
> 
> Thanks for looking it over.
> 
> I think your approach to handling mc_forwarding makes sense, although
> it differs from the IPv4 behavior.  In any case, it looks like the
> current IPv4 and IPv6 multicast routing code does not consider an
> interface's mc_forwarding value at all.

Thanks guys.

Hideaki-san, did you queue this patch up or do you want me to
apply it with the mc_forwarding counter bumps removed?

These are bug fixes so I'd like to see this in the tree soon.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller Jan. 27, 2009, 5:31 a.m. UTC | #4
From: David Miller <davem@davemloft.net>
Date: Sun, 25 Jan 2009 18:15:10 -0800 (PST)

> Hideaki-san, did you queue this patch up or do you want me to
> apply it with the mc_forwarding counter bumps removed?
> 
> These are bug fixes so I'd like to see this in the tree soon.

Ping?
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller Jan. 28, 2009, 6:39 a.m. UTC | #5
From: Tom Goff <thomas.goff@boeing.com>
Date: Tue, 20 Jan 2009 12:02:48 -0800

>   - Enable/disable IPv6 multicast forwarding on the corresponding
>     interface when a routing daemon adds/removes a multicast virtual
>     interface.

Since Hideaki-san is being unresponsive I looked at this patch again
so I can apply it, or at least part of it, to get the bugs fixed.

I disagree with his objection to the mc_forwarding changes here, they
are correct and in fact desirable.  They are desirable since without
this we have yet another disconnect between ipv4 and ipv6 behavior.

We already have way too many of those.

Every time I notice a behavior difference between our ipv4 and ipv6
stack I almost believe there is an active conspiracy to make the
transition to ipv6 as difficult as possible :-)

So I'm going to apply this patch as-is, with the addition of making
the mc_forwarding sysctl read-only (perms 0444) as it is in ipv4.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch
diff mbox

diff --git a/net/ipv6/ip6_input.c b/net/ipv6/ip6_input.c
index 936f489..f171e8d 100644
--- a/net/ipv6/ip6_input.c
+++ b/net/ipv6/ip6_input.c
@@ -255,6 +255,7 @@  int ip6_mc_input(struct sk_buff *skb)
 	 *      IPv6 multicast router mode is now supported ;)
 	 */
 	if (dev_net(skb->dev)->ipv6.devconf_all->mc_forwarding &&
+	    !(ipv6_addr_type(&hdr->daddr) & IPV6_ADDR_LINKLOCAL) &&
 	    likely(!(IP6CB(skb)->flags & IP6SKB_FORWARDED))) {
 		/*
 		 * Okay, we try to forward - split and duplicate
@@ -316,7 +317,6 @@  int ip6_mc_input(struct sk_buff *skb)
 		}

 		if (skb2) {
-			skb2->dev = skb2->dst->dev;
 			ip6_mr_input(skb2);
 		}
 	}
diff --git a/net/ipv6/ip6mr.c b/net/ipv6/ip6mr.c
index 3c51b2d..d19a84b 100644
--- a/net/ipv6/ip6mr.c
+++ b/net/ipv6/ip6mr.c
@@ -365,7 +365,9 @@  static int pim6_rcv(struct sk_buff *skb)
 	pim = (struct pimreghdr *)skb_transport_header(skb);
 	if (pim->type != ((PIM_VERSION << 4) | PIM_REGISTER) ||
 	    (pim->flags & PIM_NULL_REGISTER) ||
-	    (ip_compute_csum((void *)pim, sizeof(*pim)) != 0 &&
+	    (csum_ipv6_magic(&ipv6_hdr(skb)->saddr, &ipv6_hdr(skb)->daddr,
+			     sizeof(*pim), IPPROTO_PIM,
+			     csum_partial((void *)pim, sizeof(*pim), 0)) &&
 	     csum_fold(skb_checksum(skb, 0, skb->len, 0))))
 		goto drop;

@@ -392,7 +394,7 @@  static int pim6_rcv(struct sk_buff *skb)
 	skb_pull(skb, (u8 *)encap - skb->data);
 	skb_reset_network_header(skb);
 	skb->dev = reg_dev;
-	skb->protocol = htons(ETH_P_IP);
+	skb->protocol = htons(ETH_P_IPV6);
 	skb->ip_summed = 0;
 	skb->pkt_type = PACKET_HOST;
 	dst_release(skb->dst);
@@ -481,6 +483,7 @@  static int mif6_delete(struct net *net, int vifi)
 {
 	struct mif_device *v;
 	struct net_device *dev;
+	struct inet6_dev *in6_dev;
 	if (vifi < 0 || vifi >= net->ipv6.maxvif)
 		return -EADDRNOTAVAIL;

@@ -513,6 +516,10 @@  static int mif6_delete(struct net *net, int vifi)

 	dev_set_allmulti(dev, -1);

+	in6_dev = __in6_dev_get(dev);
+	if (in6_dev)
+		in6_dev->cnf.mc_forwarding--;
+
 	if (v->flags & MIFF_REGISTER)
 		unregister_netdevice(dev);

@@ -622,6 +629,7 @@  static int mif6_add(struct net *net, struct mif6ctl *vifc, int mrtsock)
 	int vifi = vifc->mif6c_mifi;
 	struct mif_device *v = &net->ipv6.vif6_table[vifi];
 	struct net_device *dev;
+	struct inet6_dev *in6_dev;
 	int err;

 	/* Is vif busy ? */
@@ -662,6 +670,10 @@  static int mif6_add(struct net *net, struct mif6ctl *vifc, int mrtsock)
 		return -EINVAL;
 	}

+	in6_dev = __in6_dev_get(dev);
+	if (in6_dev)
+		in6_dev->cnf.mc_forwarding++;
+
 	/*
 	 *	Fill in the VIF structures
 	 */
@@ -838,8 +850,6 @@  static int ip6mr_cache_report(struct net *net, struct sk_buff *pkt, mifi_t mifi,

 	skb->dst = dst_clone(pkt->dst);
 	skb->ip_summed = CHECKSUM_UNNECESSARY;
-
-	skb_pull(skb, sizeof(struct ipv6hdr));
 	}

 	if (net->ipv6.mroute6_sk == NULL) {
@@ -1222,8 +1232,10 @@  static int ip6mr_sk_init(struct sock *sk)

 	rtnl_lock();
 	write_lock_bh(&mrt_lock);
-	if (likely(net->ipv6.mroute6_sk == NULL))
+	if (likely(net->ipv6.mroute6_sk == NULL)) {
 		net->ipv6.mroute6_sk = sk;
+		net->ipv6.devconf_all->mc_forwarding++;
+	}
 	else
 		err = -EADDRINUSE;
 	write_unlock_bh(&mrt_lock);
@@ -1242,6 +1254,7 @@  int ip6mr_sk_done(struct sock *sk)
 	if (sk == net->ipv6.mroute6_sk) {
 		write_lock_bh(&mrt_lock);
 		net->ipv6.mroute6_sk = NULL;
+		net->ipv6.devconf_all->mc_forwarding--;
 		write_unlock_bh(&mrt_lock);

 		mroute_clean_tables(net);
diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index 76f06b9..f83275f 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -794,7 +794,7 @@  void ip6_route_input(struct sk_buff *skb)
 		.proto = iph->nexthdr,
 	};

-	if (rt6_need_strict(&iph->daddr))
+	if (rt6_need_strict(&iph->daddr) && skb->dev->type != ARPHRD_PIMREG)
 		flags |= RT6_LOOKUP_F_IFACE;

 	skb->dst = fib6_rule_lookup(net, &fl, flags, ip6_pol_route_input);