diff mbox

[IPv6] interface-local multicast escapes the local node

Message ID 20130209121005.GB23281@order.stressinduktion.org
State RFC, archived
Delegated to: David Miller
Headers show

Commit Message

Hannes Frederic Sowa Feb. 9, 2013, 12:10 p.m. UTC 
On Wed, Feb 06, 2013 at 05:54:15PM +0100, Hannes Frederic Sowa wrote:
> On Thu, Feb 07, 2013 at 12:24:14AM +0900, YOSHIFUJI Hideaki wrote:
> > NAK.  I think we should select routes via loopback device here.
> 
> Will try your idea, thanks.

Does this patch look reasonable? Btw. i am pleased to see this kind of
things work out as expected most of the time (addrtype checking etc. all
in place). :)

I have a few questions:

a) Do we have a way to lock down routes in the kernel that they are
untouchable from userspace?

b) We don't set IFF_MULTICAST in the loopback interface driver by default.
Is there a reason we couldn't do so? This patch tries to also bind the
interface local all router multicast address on lo, but will only do so
if multicast is enabled on lo before changing the sysctl.

c) I will send a patch as soon as the errata thing is resolved to drop
ff00::/15 from entering the host from the wire. In case of outgoing
traffic to ff00::/16, should this be stopped with a net-unreachable route?

Btw, I saw that the errata on RFC 4291 is already submitted:
http://www.rfc-editor.org/errata_search.php?rfc=4291
Thanks, Erik!

[PATCH net-next RFC] ipv6: let loopback join ff01::{1,2} and create a loopback route to ff00::/16

This patch ensures that traffic to ff01::/16 is directed to the loopback
interface by default and does not leave the host. The interface-local
all nodes and all routers (in case forwarding is enabled) are now joined
by default, too.

Reported-by: Erik Hugne <erik.hugne@ericsson.com>
Cc: Erik Hugne <erik.hugne@ericsson.com>
Cc: YOSHIFUJI Hideaki <yoshfuji@linux-ipv6.org>
Signed-off-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
---
 include/linux/in6.h |  6 ++++++
 net/ipv6/addrconf.c | 37 +++++++++++++++++++++++++++++++++++--
 2 files changed, 41 insertions(+), 2 deletions(-)

Comments

YOSHIFUJI Hideaki / 吉藤英明 Feb. 9, 2013, 2:12 p.m. UTC | #1 
Hi,

Hannes Frederic Sowa wrote:
> On Wed, Feb 06, 2013 at 05:54:15PM +0100, Hannes Frederic Sowa wrote:
>> On Thu, Feb 07, 2013 at 12:24:14AM +0900, YOSHIFUJI Hideaki wrote:
>>> NAK.  I think we should select routes via loopback device here.
>>
>> Will try your idea, thanks.
> 
> Does this patch look reasonable? Btw. i am pleased to see this kind of
> things work out as expected most of the time (addrtype checking etc. all
> in place). :)
> 

Well, I rethink of what "interface-local" means.

It seems applications will join ff01::/16%eth0 instead of ff01::/16%lo.
If so, your original patch seems better.  My bad, sorry.

Would you update original one, with minor modification that defers
kfree_skb() after incrementing MIB, please?

If you think we should join ff01::{1,2} by default, you can send another
patch for it.  (BTW, why don't we join ff05::2, then? ;-))

Thanks.

--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
YOSHIFUJI Hideaki / 吉藤英明 Feb. 9, 2013, 3:50 p.m. UTC | #2 
Hannes Frederic Sowa wrote:

> c) I will send a patch as soon as the errata thing is resolved to drop
> ff00::/15 from entering the host from the wire. In case of outgoing
> traffic to ff00::/16, should this be stopped with a net-unreachable route?

No, check should placed around ipv6_addr_loopback(), like you have
posted.  Would you cook a patch for ffx0::/16 anyway, please?

--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
Hannes Frederic Sowa Feb. 9, 2013, 11:08 p.m. UTC | #3 
On Sat, Feb 09, 2013 at 11:12:46PM +0900, YOSHIFUJI Hideaki wrote:
> Hi,
> 
> Hannes Frederic Sowa wrote:
> > On Wed, Feb 06, 2013 at 05:54:15PM +0100, Hannes Frederic Sowa wrote:
> >> On Thu, Feb 07, 2013 at 12:24:14AM +0900, YOSHIFUJI Hideaki wrote:
> >>> NAK.  I think we should select routes via loopback device here.
> >>
> >> Will try your idea, thanks.
> > 
> > Does this patch look reasonable? Btw. i am pleased to see this kind of
> > things work out as expected most of the time (addrtype checking etc. all
> > in place). :)
> > 
> 
> Well, I rethink of what "interface-local" means.
> 
> It seems applications will join ff01::/16%eth0 instead of ff01::/16%lo.
> If so, your original patch seems better.  My bad, sorry.

No problem, will do. I have not checked carefully, but this would mean we have
to do changes to glibc to have a correct behaving getaddrinfo?

> Would you update original one, with minor modification that defers
> kfree_skb() after incrementing MIB, please?

Yes. Will send the patch tomorrow at the latest.
> 
> If you think we should join ff01::{1,2} by default, you can send another
> patch for it.  (BTW, why don't we join ff05::2, then? ;-))

Ok, I'll split it up. You are right about ff05::2, seems like the right thing
to do, somehow. I hope this won't impact any multicast routing daemons. 

Thanks,

  Hannes

--
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
Hannes Frederic Sowa Feb. 10, 2013, 9:59 a.m. UTC | #4 
On Sat, Feb 09, 2013 at 11:12:46PM +0900, YOSHIFUJI Hideaki wrote:
> It seems applications will join ff01::/16%eth0 instead of ff01::/16%lo.
> If so, your original patch seems better.  My bad, sorry.
> 
> Would you update original one, with minor modification that defers
> kfree_skb() after incrementing MIB, please?

I would add another constraint to the if "&& !(dev->flags & IFF_LOOPBACK)", so
it becomes:

                if (IPV6_ADDR_MC_SCOPE(&ipv6_hdr(skb)->daddr) <=
                    IPV6_ADDR_SCOPE_NODELOCAL &&
                    !(dev->flags & IFF_LOOPBACK))
                        kfree_skb(skb);
                        return 0;
                }


Otherwise ff01::/16%lo would not work because the multicast mirroring through
dev_loopback_xmit won't be taken and the packet would be dropped after that.

Can you confirm? Thanks.

--
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
YOSHIFUJI Hideaki / 吉藤英明 Feb. 10, 2013, 10:57 a.m. UTC | #5 
Hannes Frederic Sowa wrote:
> On Sat, Feb 09, 2013 at 11:12:46PM +0900, YOSHIFUJI Hideaki wrote:
>> It seems applications will join ff01::/16%eth0 instead of ff01::/16%lo.
>> If so, your original patch seems better.  My bad, sorry.
>>
>> Would you update original one, with minor modification that defers
>> kfree_skb() after incrementing MIB, please?
> 
> I would add another constraint to the if "&& !(dev->flags & IFF_LOOPBACK)", so
> it becomes:
> 
>                 if (IPV6_ADDR_MC_SCOPE(&ipv6_hdr(skb)->daddr) <=
>                     IPV6_ADDR_SCOPE_NODELOCAL &&
>                     !(dev->flags & IFF_LOOPBACK))
>                         kfree_skb(skb);
>                         return 0;
>                 }
> 
> 
> Otherwise ff01::/16%lo would not work because the multicast mirroring through
> dev_loopback_xmit won't be taken and the packet would be dropped after that.
> 
> Can you confirm? Thanks.

Ack.

--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
Hannes Frederic Sowa Feb. 10, 2013, 3:32 p.m. UTC | #6 
On Sat, Feb 09, 2013 at 11:12:46PM +0900, YOSHIFUJI Hideaki wrote:
> Hi,
> 
> Hannes Frederic Sowa wrote:
> > On Wed, Feb 06, 2013 at 05:54:15PM +0100, Hannes Frederic Sowa wrote:
> >> On Thu, Feb 07, 2013 at 12:24:14AM +0900, YOSHIFUJI Hideaki wrote:
> >>> NAK.  I think we should select routes via loopback device here.
> >>
> >> Will try your idea, thanks.
> > 
> > Does this patch look reasonable? Btw. i am pleased to see this kind of
> > things work out as expected most of the time (addrtype checking etc. all
> > in place). :)
> > 
> 
> Well, I rethink of what "interface-local" means.
> 
> It seems applications will join ff01::/16%eth0 instead of ff01::/16%lo.
> If so, your original patch seems better.  My bad, sorry.

I was looking at getpeername et. al. where we should report the scope
back to the user. A common pattern is:

                        if (ipv6_addr_type(&sin->sin6_addr) & IPV6_ADDR_LINKLOCAL)
                                sin->sin6_scope_id = IP6CB(skb)->iif;

I propose to introduce something like 'bool ipv6_addr_intf_scoped(in6_addr)'
and let it check for ll addresses and interface scoped addresses.

--
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
YOSHIFUJI Hideaki / 吉藤英明 Feb. 10, 2013, 6:42 p.m. UTC | #7 
Hannes Frederic Sowa wrote:
> On Sat, Feb 09, 2013 at 11:12:46PM +0900, YOSHIFUJI Hideaki wrote:
>> Hi,
>>
>> Hannes Frederic Sowa wrote:
>>> On Wed, Feb 06, 2013 at 05:54:15PM +0100, Hannes Frederic Sowa wrote:
>>>> On Thu, Feb 07, 2013 at 12:24:14AM +0900, YOSHIFUJI Hideaki wrote:
>>>>> NAK.  I think we should select routes via loopback device here.
>>>>
>>>> Will try your idea, thanks.
>>>
>>> Does this patch look reasonable? Btw. i am pleased to see this kind of
>>> things work out as expected most of the time (addrtype checking etc. all
>>> in place). :)
>>>
>>
>> Well, I rethink of what "interface-local" means.
>>
>> It seems applications will join ff01::/16%eth0 instead of ff01::/16%lo.
>> If so, your original patch seems better.  My bad, sorry.
> 
> I was looking at getpeername et. al. where we should report the scope
> back to the user. A common pattern is:
> 
>                         if (ipv6_addr_type(&sin->sin6_addr) & IPV6_ADDR_LINKLOCAL)
>                                 sin->sin6_scope_id = IP6CB(skb)->iif;
> 
> I propose to introduce something like 'bool ipv6_addr_intf_scoped(in6_addr)'
> and let it check for ll addresses and interface scoped addresses.

Hmm, maybe, we might want to say:

__u32 __ipv6_iface_scope_id(int type, unsigned int iface)
{
	if (type == IPV6_ADDR_ANY ||
	    type & IPV6_ADDR_LOOPBACK ||
	    __ipv6_addr_src_scope(type) > IPV6_ADDR_SCOPE_LINKLOCAL)
		return 0;
	return iface;
}

__u32 ipv6_iface_scope_id(const struct in6_addr *addr, unsigned int iface)
{
	return __ipv6_iface_scope_id(__ipv6_addr_type(addr), iface);
}

And then,
	sin->sin6_scope_id = __ipv6_iface_scope_id(__ipv6_addr_type(&sin->sin6_addr),
						   IP6CB(skb)->iif);
or
	sin->sin6_scope_id = ipv6_iface_scope_id(&sin->sin6_addr, IP6CB(skb)->iif);

--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
Hannes Frederic Sowa Feb. 10, 2013, 6:49 p.m. UTC | #8 
On Mon, Feb 11, 2013 at 03:42:53AM +0900, YOSHIFUJI Hideaki wrote:
> Hannes Frederic Sowa wrote:
> > I was looking at getpeername et. al. where we should report the scope
> > back to the user. A common pattern is:
> > 
> >                         if (ipv6_addr_type(&sin->sin6_addr) & IPV6_ADDR_LINKLOCAL)
> >                                 sin->sin6_scope_id = IP6CB(skb)->iif;
> > 
> > I propose to introduce something like 'bool ipv6_addr_intf_scoped(in6_addr)'
> > and let it check for ll addresses and interface scoped addresses.
> 
> Hmm, maybe, we might want to say:
> 
> __u32 __ipv6_iface_scope_id(int type, unsigned int iface)
> {
> 	if (type == IPV6_ADDR_ANY ||
> 	    type & IPV6_ADDR_LOOPBACK ||
> 	    __ipv6_addr_src_scope(type) > IPV6_ADDR_SCOPE_LINKLOCAL)
> 		return 0;
> 	return iface;
> }
> 
> __u32 ipv6_iface_scope_id(const struct in6_addr *addr, unsigned int iface)
> {
> 	return __ipv6_iface_scope_id(__ipv6_addr_type(addr), iface);
> }
> 
> And then,
> 	sin->sin6_scope_id = __ipv6_iface_scope_id(__ipv6_addr_type(&sin->sin6_addr),
> 						   IP6CB(skb)->iif);
> or
> 	sin->sin6_scope_id = ipv6_iface_scope_id(&sin->sin6_addr, IP6CB(skb)->iif);

I like it. Will try to convert some checks and let you know how it turned out.

--
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
diff mbox

Patch

diff --git a/include/linux/in6.h b/include/linux/in6.h
index a16e193..fd0e86d 100644
--- a/include/linux/in6.h
+++ b/include/linux/in6.h
@@ -36,4 +36,10 @@  extern const struct in6_addr in6addr_linklocal_allnodes;
 extern const struct in6_addr in6addr_linklocal_allrouters;
 #define IN6ADDR_LINKLOCAL_ALLROUTERS_INIT \
 		{ { { 0xff,2,0,0,0,0,0,0,0,0,0,0,0,0,0,2 } } }
+extern const struct in6_addr in6addr_interfacelocal_allnodes;
+#define IN6ADR_INTERFACELOCAL_ALLNODES_INIT \
+		{ { { 0xff,1,0,0,0,0,0,0,0,0,0,0,0,0,0,1 } } }
+extern const struct in6_addr in6addr_interfacelocal_allrouters;
+#define IN6ADR_INTERFACELOCAL_ALLROUTERS_INIT \
+		{ { { 0xff,1,0,0,0,0,0,0,0,0,0,0,0,0,0,2 } } }
 #endif
diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index bd9f936..c15b98f 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -244,6 +244,8 @@  const struct in6_addr in6addr_any = IN6ADDR_ANY_INIT;
 const struct in6_addr in6addr_loopback = IN6ADDR_LOOPBACK_INIT;
 const struct in6_addr in6addr_linklocal_allnodes = IN6ADDR_LINKLOCAL_ALLNODES_INIT;
 const struct in6_addr in6addr_linklocal_allrouters = IN6ADDR_LINKLOCAL_ALLROUTERS_INIT;
+const struct in6_addr in6addr_interfacelocal_allnodes = IN6ADR_INTERFACELOCAL_ALLNODES_INIT;
+const struct in6_addr in6addr_interfacelocal_allrouters = IN6ADR_INTERFACELOCAL_ALLROUTERS_INIT;
 
 /* Check if a valid qdisc is available */
 static inline bool addrconf_qdisc_ok(const struct net_device *dev)
@@ -611,10 +613,17 @@  static void dev_forward_change(struct inet6_dev *idev)
 	if (idev->cnf.forwarding)
 		dev_disable_lro(dev);
 	if (dev->flags & IFF_MULTICAST) {
-		if (idev->cnf.forwarding)
+		if (idev->cnf.forwarding) {
 			ipv6_dev_mc_inc(dev, &in6addr_linklocal_allrouters);
-		else
+			if (dev->flags & IFF_LOOPBACK)
+				ipv6_dev_mc_inc(dev,
+						&in6addr_interfacelocal_allrouters);
+		} else {
 			ipv6_dev_mc_dec(dev, &in6addr_linklocal_allrouters);
+			if (dev->flags & IFF_LOOPBACK)
+				ipv6_dev_mc_dec(dev,
+						&in6addr_interfacelocal_allrouters);
+		}
 	}
 
 	list_for_each_entry(ifa, &idev->addr_list, if_list) {
@@ -2518,6 +2527,29 @@  static void sit_add_v4_addrs(struct inet6_dev *idev)
 }
 #endif
 
+static void init_loopback_mc(struct net_device *dev)
+{
+	struct fib6_config cfg = {
+		.fc_table = RT6_TABLE_LOCAL,
+		.fc_metric = IP6_RT_PRIO_ADDRCONF,
+		.fc_dst_len = 16,
+		.fc_ifindex = dev->ifindex,
+		.fc_flags = RTF_UP,
+		.fc_protocol = RTPROT_KERNEL,
+		.fc_type = RTN_MULTICAST,
+		.fc_nlinfo.nl_net = dev_net(dev),
+	};
+
+	ipv6_addr_set(&cfg.fc_dst, htonl(0xFF010000), 0, 0, 0);
+	if (ip6_route_add(&cfg))
+		pr_debug("%s: ip6_route_add failed for node local multicast\n",
+			 __func__);
+
+	if (ipv6_dev_mc_inc(dev, &in6addr_interfacelocal_allnodes))
+		pr_debug("%s: failed to join interface-local all nodes mc\n",
+			 __func__);
+}
+
 static void init_loopback(struct net_device *dev)
 {
 	struct inet6_dev  *idev;
@@ -2532,6 +2564,7 @@  static void init_loopback(struct net_device *dev)
 	}
 
 	add_addr(idev, &in6addr_loopback, 128, IFA_HOST);
+	init_loopback_mc(dev);
 }
 
 static void addrconf_add_linklocal(struct inet6_dev *idev, const struct in6_addr *addr)