Skip to contentSkip to conversation

: IPV6_MULTICAST_IF setting is ignored on link-local connect()

PATCH In review Brian Haley 4 participants 15 messages Dec 18, 2007 – Jan 9, 2008

Subsystems (2)
Networking Network drivers
lore
Share & tools
lore

Brian Haley

author
#0 lore raw

PATCH 1/1 : IPV6_MULTICAST_IF setting is ignored on link-local connect()

Trying to connect() to an IPv6 link-local multicast address by specifying the outgoing multicast interface doesn't work, you have to bind to a device first with an SO_BINDTODEVICE setsockopt() call. This patch allows the IPV6_MULTICAST_IF setting to also control which interface should be used for the connection, as specified in RFC 3493.

Signed-off-by Brian Haley <[email protected]>

---

net/ipv6/datagram.c +9 -3
index 2ed689a..0b1e7eb 100644
@@ -123,9 +123,15 @@ ipv4_connected:
 				goto out;
 			}
 			sk->sk_bound_dev_if = usin->sin6_scope_id;
-			if (!sk->sk_bound_dev_if &&
-			    (addr_type & IPV6_ADDR_MULTICAST))
-				fl.oif = np->mcast_oif;
+		}
+
+		if ((addr_type & IPV6_ADDR_MULTICAST) && np->mcast_oif) {
+			if (sk->sk_bound_dev_if &&
+			    sk->sk_bound_dev_if != np->mcast_oif) {
+				err = -EINVAL;
+				goto out;
+			}
+			sk->sk_bound_dev_if = np->mcast_oif;
 		}
 
 		/* Connect to link-local address requires an interface */
#1 lore raw

Brian Haley <[email protected]> wrote on 12/18/2007 12:57:54 PM:

Quoted 3 lines

Trying to connect() to an IPv6 link-local multicast address by specifying the outgoing multicast interface doesn't work, you have to bind to a device first with an SO_BINDTODEVICE setsockopt() call.

No, you simply have to specify sin6_scope_id for link-scope
addresses, like you do in unicast cases. Your patch requires them to match (if specified), but I don't think IPV6_MULTICAST_IF should override or require a match for a valid sin6_scope_id (or be an error).
If I read it correctly, the existing code uses IPV6_MULTICAST_IF
if the sin6_scope_id is not set, otherwise honors the interface specified in the connect. That seems like correct behaviour to me, and RFC 3493 doesn't address the relative precedence of the two that I see. This is in the "linklocal" branch, and all unicast linklocal's require specifying sin6_scope_id. Multicast doesn't if require a scope_id in the case where you've done an IPV6_MULTICAST_IF, but it should still allow a different scope_id when you have used IPV6_MULTICAST_IF.

Do you have application code that you believe is correct that
doesn't work?

                                                                +-DLS
author
#2 lore raw
Quoted 5 lines · David Stevens

Brian Haley <[email protected]> wrote on 12/18/2007 12:57:54 PM:

Quoted 3 lines

Trying to connect() to an IPv6 link-local multicast address by specifying the outgoing multicast interface doesn't work, you have to bind to a device first with an SO_BINDTODEVICE setsockopt() call.

Other OSes allow this operation, like FreeBSD, Tru64 UNIX and Solaris.

Quoted 2 lines

No, you simply have to specify sin6_scope_id for link-scope
addresses, like you do in unicast cases.

But isn't this why IPV6_MULTICAST_IF exists? So you don't have to bind to an interface or use the scope id? RFC 3493 does not mention having to set a scope id in order to send multicast packets:

IPv6 applications may send multicast packets by simply specifying an IPv6 multicast address as the destination address, for example in the destination address argument of the sendto() function.

Quoted 3 lines

Your patch requires them
to match (if specified), but I don't think IPV6_MULTICAST_IF should override or require a match for a valid sin6_scope_id (or be an error).

The patch won't override sk_bound_dev_if, or sin6_scope_id, it's a last resort for link-local multicast. As far as matching, I think they should if you set both SO_BINDTODEVICE/sin6_scope_id and IPV6_MULTICAST_IF. I can relax that check if you like.

The one thing my patch does do is set sk_bound_dev_if, which it never did - that seemed like the right thing to do since that's what the scope id path does, and makes sure we always continue to use this interface.

Quoted 8 lines

If I read it correctly, the existing code uses IPV6_MULTICAST_IF
if the sin6_scope_id is not set, otherwise honors the interface specified in the connect. That seems like correct behaviour to me, and RFC 3493 doesn't address the relative precedence of the two that I see. This is in the "linklocal" branch, and all unicast linklocal's require specifying sin6_scope_id. Multicast doesn't if require a scope_id in the case where you've done an IPV6_MULTICAST_IF, but it should still allow a different scope_id when you have used IPV6_MULTICAST_IF.

The IPV6_ADDR_MULTICAST check is inside the sin6_scope_id if() statement, so will never get checked if the scope hasn't been specified, that's the bug. Since that isn't required for multicast we always get an EINVAL here.

Quoted 2 lines

Do you have application code that you believe is correct that
doesn't work?

Yes, a customer does.

-Brian

#3 lore raw

Brian,

OK, I see what you're trying to fix now.

I think the scope_id checks are not quite right-- they should be something like this:

if (addr_type&IPV6_ADDR_LINKLOCAL) {
        if (addr_len >= sizeof(struct sockaddr_in6)) {
                if (sk->sk_bound_dev_if && usin->sin6_scope_id &&
                    sk->sk_bound_dev_if != usin->sin6_scope_id) {
                        err = -EINVAL;
                        goto out;
                }
                if (usin->sin6_scope_id)
                        sk->sk_bound_dev_if = usin->sin6_scope_id;
                if (!sk->sk_bound_dev_if &&
                     (addr_type & IPV6_ADDR_MULTICAST))
                        fl.oif = np->mcast_oif;
        }

        /* connect to the link-local addres requires an interface */
        if (!sk->sk_bound_dev_if) {
                err = -EINVAL;
                goto out;
        }
}

That is (in English):

If I did an SO_BINDTODEVICE and specified sin6_scope_id,
then they better agree.
If I specified sin6_scope_id without SO_BINDTODEVICE, set
the device to that.
If I get this far without a device and it's multicast, use
mcast_oif
If I get all through that and don't have a device, EINVAL.

Does that work for you?

+-DLS

#4 lore raw

Daven

Quoted 19 lines · David Stevens

Brian,

OK, I see what you're trying to fix now.

I think the scope_id checks are not quite right-- they should be something like this:

if (addr_type&IPV6_ADDR_LINKLOCAL) {
        if (addr_len >= sizeof(struct sockaddr_in6)) {
                if (sk->sk_bound_dev_if && usin->sin6_scope_id &&
                    sk->sk_bound_dev_if != usin->sin6_scope_id) {
                        err = -EINVAL;
                        goto out;
                }
                if (usin->sin6_scope_id)
                        sk->sk_bound_dev_if = usin->sin6_scope_id;
                if (!sk->sk_bound_dev_if &&
                     (addr_type & IPV6_ADDR_MULTICAST))
                        fl.oif = np->mcast_oif;
Quoted 8 lines
        }

        /* connect to the link-local addres requires an interface */
        if (!sk->sk_bound_dev_if) {
                err = -EINVAL;
                goto out;
        }
}

But this still requires either a SO_BINDTODEVICE or sin6_scope_id. This means the an application can call BINDTODEVICE(eth0), MULTICAST_IF(eth1) issue a connect on a UDP socket an succeed? Seems wrong to me.

Can you check section 6.7 of RFC 3542.

Thanks
-vlad

Quoted 20 lines

That is (in English):

If I did an SO_BINDTODEVICE and specified sin6_scope_id,
then they better agree.
If I specified sin6_scope_id without SO_BINDTODEVICE, set
the device to that.
If I get this far without a device and it's multicast, use
mcast_oif
If I get all through that and don't have a device, EINVAL.

Does that work for you?

+-DLS

--
To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

#5 lore raw

Vlad Yasevich <[email protected]> wrote on 12/19/2007 07:20:53 AM:

Quoted 5 lines

But this still requires either a SO_BINDTODEVICE or sin6_scope_id. This means the an application can call BINDTODEVICE(eth0), MULTICAST_IF(eth1) issue a connect on a UDP socket an succeed? Seems wrong to me.

Can you check section 6.7 of RFC 3542.

No, it requires one of SO_BINDTODEVICE, sin6_scope_id, or
IPV6_MULTICAST_IF.
If you do an SO_BINDTODEVICE(eth0) and then an IPV6_MULTICAST_IF(eth1), the
IPV6_MULTICAST_IF will fail in setsockopt (EINVAL), because it requires a match
for bound sockets. I'm not sure if SO_BINDTODEVICE resets mcast_oif if you do
them in the reverse order, but that would be a bug in SO_BINDTODEVICE.
The precedence order as implemented already is:

SO_BINDTODEVICE is highest and always wins sin6_scope_id next IPV6_MULTICAST_IF

and the existing code has the rule that all link-local addresses require a sin6_scope_id. The change (intended) is to relax the sin6_scope_id rule only
for link-local multicasts that have done either an SO_BINDTODEVICE or IPV6_MULTICAST_IF already.

+-DLS

#6 lore raw
Quoted 8 lines · David Stevens

Vlad Yasevich <[email protected]> wrote on 12/19/2007 07:20:53 AM:

Quoted 5 lines

But this still requires either a SO_BINDTODEVICE or sin6_scope_id. This means the an application can call BINDTODEVICE(eth0), MULTICAST_IF(eth1) issue a connect on a UDP socket an succeed? Seems wrong to me.

Can you check section 6.7 of RFC 3542.

Quoted 7 lines

No, it requires one of SO_BINDTODEVICE, sin6_scope_id, or
IPV6_MULTICAST_IF.
If you do an SO_BINDTODEVICE(eth0) and then an IPV6_MULTICAST_IF(eth1), the
IPV6_MULTICAST_IF will fail in setsockopt (EINVAL), because it requires a match
for bound sockets.

I should have checked that... so the case I thought off is not possible...

Quoted 3 lines

I'm not sure if SO_BINDTODEVICE resets mcast_oif if you
do
them in the reverse order, but that would be a bug in SO_BINDTODEVICE.

I don't think that would be needed since SO_BINDTODEVICE always wins over IPV6_MULTICAST_IF, so even if they mismatch, SO_BINDTODEVICE is still used.

Quoted 6 lines

The precedence order as implemented already is:

SO_BINDTODEVICE is highest and always wins sin6_scope_id next IPV6_MULTICAST_IF

Quoted 6 lines

and the existing code has the rule that all link-local addresses require a sin6_scope_id. The change (intended) is to relax the sin6_scope_id rule only
for link-local multicasts that have done either an SO_BINDTODEVICE or IPV6_MULTICAST_IF already.

Ok, but I don't think your patch accomplishes that, as Brian pointed out.

-vlad

author
#7 lore raw
In reply to David Stevens
Quoted 6 lines · David Stevens

Vlad Yasevich <[email protected]> wrote on 12/19/2007 07:20:53 AM:

Quoted 4 lines

But this still requires either a SO_BINDTODEVICE or sin6_scope_id. This means the an application can call BINDTODEVICE(eth0), MULTICAST_IF(eth1) issue a connect on a UDP socket an succeed? Seems wrong to me.

Quoted 8 lines
Quoted 1 line

Can you check section 6.7 of RFC 3542.

No, it requires one of SO_BINDTODEVICE, sin6_scope_id, or
IPV6_MULTICAST_IF.
If you do an SO_BINDTODEVICE(eth0) and then an IPV6_MULTICAST_IF(eth1), the IPV6_MULTICAST_IF will fail in setsockopt (EINVAL), because it requires a match for bound sockets. I'm not sure if SO_BINDTODEVICE resets mcast_oif if you do them in the reverse order, but that would be a bug in SO_BINDTODEVICE.

It doesn't, that was one way I tested my first patch by forcing a mis-match.

Quoted 10 lines

The precedence order as implemented already is:

SO_BINDTODEVICE is highest and always wins sin6_scope_id next IPV6_MULTICAST_IF

and the existing code has the rule that all link-local addresses require a sin6_scope_id. The change (intended) is to relax the sin6_scope_id rule only for link-local multicasts that have done either an SO_BINDTODEVICE or IPV6_MULTICAST_IF already.

Yes, that was the intention of my patch.

-Brian

author
#8 lore raw
In reply to David Stevens

Hi David,

Quoted 5 lines · David Stevens

OK, I see what you're trying to fix now.

I think the scope_id checks are not quite right-- they should be something like this:

Quoted 12 lines
if (addr_type&IPV6_ADDR_LINKLOCAL) {
        if (addr_len >= sizeof(struct sockaddr_in6)) {
                if (sk->sk_bound_dev_if && usin->sin6_scope_id &&
                    sk->sk_bound_dev_if != usin->sin6_scope_id) {
                        err = -EINVAL;
                        goto out;
                }
                if (usin->sin6_scope_id)
                        sk->sk_bound_dev_if = usin->sin6_scope_id;
                if (!sk->sk_bound_dev_if &&
                     (addr_type & IPV6_ADDR_MULTICAST))
                        fl.oif = np->mcast_oif;

This assignment will not get us past the next check...

Quoted 5 lines
        /* connect to the link-local addres requires an interface */
        if (!sk->sk_bound_dev_if) {
                err = -EINVAL;
                goto out;
        }

... and even if it did, fl.oif is over-written by sk_bound_dev_if just a few lines down.

Quoted 7 lines

If I did an SO_BINDTODEVICE and specified sin6_scope_id,
then they better agree.
If I specified sin6_scope_id without SO_BINDTODEVICE, set
the device to that.
If I get this far without a device and it's multicast, use
mcast_oif
If I get all through that and don't have a device, EINVAL.

You also need to check if mcast_oif matches sk_bind_dev_if here - it's actually done in the setsockopt() code already when we set it, duplicating it here isn't that big a deal.

How about the following patch? It does not set sk_bound_dev_if to mcast_oif, but does allow the connect() to succeed.

-Brian

Signed-off-by Brian Haley <[email protected]>

---

net/ipv6/datagram.c +7 -6
index 2ed689a..3226970 100644
@@ -114,6 +114,8 @@ ipv4_connected:
 		goto out;
 	}
 
+	fl.oif = sk->sk_bound_dev_if;
+
 	if (addr_type&IPV6_ADDR_LINKLOCAL) {
 		if (addr_len >= sizeof(struct sockaddr_in6) &&
 		    usin->sin6_scope_id) {

@@ -122,14 +124,14 @@ ipv4_connected:
 				err = -EINVAL;
 				goto out;
 			}
-			sk->sk_bound_dev_if = usin->sin6_scope_id;
-			if (!sk->sk_bound_dev_if &&
-			    (addr_type & IPV6_ADDR_MULTICAST))
-				fl.oif = np->mcast_oif;
+			fl.oif = sk->sk_bound_dev_if = usin->sin6_scope_id;
 		}
 
+		if (!fl.oif && (addr_type & IPV6_ADDR_MULTICAST))
+			fl.oif = np->mcast_oif;
+
 		/* Connect to link-local address requires an interface */
-		if (!sk->sk_bound_dev_if) {
+		if (!fl.oif) {
 			err = -EINVAL;
 			goto out;
 		}

@@ -148,7 +150,6 @@ ipv4_connected:
 	fl.proto = sk->sk_protocol;
 	ipv6_addr_copy(&fl.fl6_dst, &np->daddr);
 	ipv6_addr_copy(&fl.fl6_src, &np->saddr);
-	fl.oif = sk->sk_bound_dev_if;
 	fl.fl_ip_dport = inet->dport;
 	fl.fl_ip_sport = inet->sport;
#9 lore raw

Brian Haley <[email protected]> wrote on 12/19/2007 07:35:46 AM:
...

Quoted 7 lines
Quoted 5 lines
                if (usin->sin6_scope_id)
                        sk->sk_bound_dev_if = usin->sin6_scope_id;
                if (!sk->sk_bound_dev_if &&
                     (addr_type & IPV6_ADDR_MULTICAST))
                        fl.oif = np->mcast_oif;

This assignment will not get us past the next check...

Yeah, that's what I get for typing in off-the-cuff code. What
I was thinking was the fl.oif assignment instead was:

        if (!sk->sk_bound_dev_if &&
                (addr_type & IPV6_ADDR_MULTICAST))
                        sk->sk_bound_dev_if = np->mcast_oif;

Which it is not, but maybe it could be, since this is a connect().

That patch looks better, but I'm wondering if we could just remove the requirement that sin6_scope_id be set here if it's multicast, since it is doing the following later in the code:

        if (!fl.oif && (addr_type&IPV6_ADDR_MULTICAST))
                fl.oif = np->mcast_oif;

So, really, all we need to do is get through the LINKLOCAL section without error in the multicast case and we can remove the redundant multicast check there. I think that'd be simpler.

I also note that sin6_scope_id appears not to be honored at all in the non-linklocal case, which may be correct, but surprises me.

I want to look a little more at this; I know you have a customer issue, so I'll make it quick.

                                                                +-DLS
author
#10 lore raw
Quoted 9 lines · David Stevens

Brian Haley <[email protected]> wrote on 12/19/2007 07:35:46 AM:
...

Quoted 6 lines
Quoted 5 lines
                if (usin->sin6_scope_id)
                        sk->sk_bound_dev_if = usin->sin6_scope_id;
                if (!sk->sk_bound_dev_if &&
                     (addr_type & IPV6_ADDR_MULTICAST))
                        fl.oif = np->mcast_oif;

This assignment will not get us past the next check...

Quoted 7 lines

Yeah, that's what I get for typing in off-the-cuff code. What
I was thinking was the fl.oif assignment instead was:

        if (!sk->sk_bound_dev_if &&
                (addr_type & IPV6_ADDR_MULTICAST))
                        sk->sk_bound_dev_if = np->mcast_oif;

Which it is not, but maybe it could be, since this is a connect().

My original patch did this, but also checked for a possible mis-match with sk_bound_dev_if - it would actually wind-up setting it to the same value if it was already set correctly.

Quoted 6 lines

That patch looks better, but I'm wondering if we could just remove the requirement that sin6_scope_id be set here if it's multicast, since it is doing the following later in the code:

        if (!fl.oif && (addr_type&IPV6_ADDR_MULTICAST))
                fl.oif = np->mcast_oif;

We would still have to check np->mcast_oif is set in the link-local case since we shouldn't be getting here with a zero.

Quoted 9 lines

So, really, all we need to do is get through the LINKLOCAL section without error in the multicast case and we can remove the redundant multicast check there. I think that'd be simpler.

I also note that sin6_scope_id appears not to be honored at all in the non-linklocal case, which may be correct, but surprises me.

I want to look a little more at this; I know you have a customer issue, so I'll make it quick.

Don't worry about that, they can wait, and I'm leaving for 10 days anyways...

-Brian

#11 lore raw
Quoted 4 lines

We would still have to check np->mcast_oif is set in the link-local case

since we shouldn't be getting here with a zero.

Actually, that's one of the things I wanted to look into. I'm not
sure if there's a path through here with (even non-linklocal) multicasts that end up without an interface set. And I'd to do some testing of different cases to see if they behave as expected; also compare to the similar sendmsg() cases.

Quoted 3 lines

Don't worry about that, they can wait, and I'm leaving for 10 days anyways...

Good, thanks. It has been this way for years now, and I'm out
too. :-) Enjoy,

                                                                +-DLS
author
#12 lore raw
In reply to David Stevens
Quoted 7 lines · David Stevens

Yeah, that's what I get for typing in off-the-cuff code. What
I was thinking was the fl.oif assignment instead was:

        if (!sk->sk_bound_dev_if &&
                (addr_type & IPV6_ADDR_MULTICAST))
                        sk->sk_bound_dev_if = np->mcast_oif;

Which it is not, but maybe it could be, since this is a connect().

How about the simple patch below? I just removed the ENINVAL check from my original patch, but it accomplishes the same thing.

Quoted 10 lines

That patch looks better, but I'm wondering if we could just remove the requirement that sin6_scope_id be set here if it's multicast, since it is doing the following later in the code:

        if (!fl.oif && (addr_type&IPV6_ADDR_MULTICAST))
                fl.oif = np->mcast_oif;

So, really, all we need to do is get through the LINKLOCAL section without error in the multicast case and we can remove the redundant multicast check there. I think that'd be simpler.

I don't think we can remove that check since it covers the non-multicast case.

-Brian

Signed-off-by Brian Haley <[email protected]>

---

net/ipv6/datagram.c +3 -3
index 2ed689a..5d4245a 100644
@@ -123,11 +123,11 @@ ipv4_connected:
 				goto out;
 			}
 			sk->sk_bound_dev_if = usin->sin6_scope_id;
-			if (!sk->sk_bound_dev_if &&
-			    (addr_type & IPV6_ADDR_MULTICAST))
-				fl.oif = np->mcast_oif;
 		}
 
+		if (!sk->sk_bound_dev_if && (addr_type & IPV6_ADDR_MULTICAST))
+			sk->sk_bound_dev_if = np->mcast_oif;
+
 		/* Connect to link-local address requires an interface */
 		if (!sk->sk_bound_dev_if) {
 			err = -EINVAL;
Acked-by
#13 lore raw

Brian,
Looks good to me.

+-DLS

Acked-by David L Stevens <[email protected]>
Quoted 3 lines

How about the simple patch below? I just removed the ENINVAL check from

my original patch, but it accomplishes the same thing.

...

Quoted 22 lines
Signed-off-by Brian Haley <[email protected]>

---

net/ipv6/datagram.c +3 -3
index 2ed689a..5d4245a 100644
@@ -123,11 +123,11 @@ ipv4_connected:
             goto out;
          }
          sk->sk_bound_dev_if = usin->sin6_scope_id;
-         if (!sk->sk_bound_dev_if &&
-             (addr_type & IPV6_ADDR_MULTICAST))
-            fl.oif = np->mcast_oif;
       }
 
+      if (!sk->sk_bound_dev_if && (addr_type & IPV6_ADDR_MULTICAST))
+         sk->sk_bound_dev_if = np->mcast_oif;
+
       /* Connect to link-local address requires an interface */
       if (!sk->sk_bound_dev_if) {
          err = -EINVAL;