IPv6: fix bug when specifying the non-exist outgoing interface

PATCH In review Shan Wei 3 participants 16 messages Jun 2, 2008 – Jun 3, 2008

Subsystems (2)

Networking Network drivers

lore

Share & tools

lore

Shan Wei

author

2008-06-02 08:13 UTC #0 lore raw

PATCH 1/1 IPv6: fix bug when specifying the non-exist outgoing interface

When specifying the outgoing interface with sendmsg, if the ipi6_addr is
the unspecified address and the ipi6_ifindex is the not-exist interface, it should be fail and the errno should be set ENODEV.
Actually, it does well(sendmsg returns on success ), because the kernel
don't check the interface。

The patch is against 2.6.26-rc4.

Signed-off-by Shan Wei<[email protected]>

---
 net/ipv6/datagram.c |   11 ++++++-----
 1 files changed, 6 insertions(+), 5 deletions(-)

net/ipv6/datagram.c +6 -5

index 94fa6ae..a6d402b 100644
@@ -535,6 +535,12 @@ int datagram_send_ctl(struct msghdr *msg, struct flowi *fl,
 				fl->oif = src_info->ipi6_ifindex;
 			}
 
+			if (fl->oif){
+				dev = dev_get_by_index(&init_net, fl->oif);
+				if (!dev)
+					return -ENODEV;
+			}
+
 			addr_type = ipv6_addr_type(&src_info->ipi6_addr);
 
 			if (addr_type == IPV6_ADDR_ANY)

@@ -543,11 +549,6 @@ int datagram_send_ctl(struct msghdr *msg, struct flowi *fl,
 			if (addr_type & IPV6_ADDR_LINKLOCAL) {
 				if (!src_info->ipi6_ifindex)
 					return -EINVAL;
-				else {
-					dev = dev_get_by_index(&init_net, src_info->ipi6_ifindex);
-					if (!dev)
-						return -ENODEV;
-				}
 			}
 			if (!ipv6_chk_addr(&init_net, &src_info->ipi6_addr,
 					   dev, 0)) {

-- 1.5.4.4

YOSHIFUJI Hideaki / 吉藤英明

2008-06-02 08:19 UTC #1 lore raw

In article <[email protected]> (at Mon, 02 Jun 2008 16:12:01 +0800), Shan Wei <[email protected]> says:

Quoted 7 lines

When specifying the outgoing interface with sendmsg, if the ipi6_addr is
the unspecified address and the ipi6_ifindex is the not-exist interface, it should be fail and the errno should be set ENODEV.
Actually, it does well(sendmsg returns on success ), because the kernel
don't check the interface。
The patch is against 2.6.26-rc4.

Please fix dev leakage.

--yoshfuji

Shan Wei

author

2008-06-02 08:53 UTC #2 lore raw

The patch is against 2.6.26-rc4.

Signed-off-by Shan Wei<[email protected]>

---
net/ipv6/datagram.c | 24 +++++++++++++++---------

1 files changed, 15 insertions(+), 9 deletions(-)

net/ipv6/datagram.c +0 -0

index 94fa6ae..47b85cc 100644
@@ -534,20 +534,26 @@ int datagram_send_ctl(struct msghdr *msg, struct

flowi *fl,
return -EINVAL;
fl->oif = src_info->ipi6_ifindex;
}
+
+ if (fl->oif) {
+ dev = dev_get_by_index(&init_net, fl->oif);
+ if (!dev)
+ return -ENODEV;
+ }

addr_type = ipv6_addr_type(&src_info->ipi6_addr);

- if (addr_type == IPV6_ADDR_ANY)
+ if (addr_type == IPV6_ADDR_ANY) {
+ if (dev)
+ dev_put(dev);
break;
+ }

- if (addr_type & IPV6_ADDR_LINKLOCAL) {
- if (!src_info->ipi6_ifindex)
- return -EINVAL;
- else {
- dev = dev_get_by_index(&init_net, src_info->ipi6_ifindex);
- if (!dev)
- return -ENODEV;
- }
+ if ((addr_type & IPV6_ADDR_LINKLOCAL) &&
+ !src_info->ipi6_ifindex) {
+ if (dev)
+ dev_put(dev);
+ return -EINVAL;
}
if (!ipv6_chk_addr(&init_net, &src_info->ipi6_addr, dev, 0)) {

Signature · 1 line

1.5.4.4

Shan Wei

author

2008-06-02 08:59 UTC #3 lore raw

In reply to YOSHIFUJI Hideaki / 吉藤英明

The patch is against 2.6.26-rc4.

Signed-off-by Shan Wei<[email protected]>

---
 net/ipv6/datagram.c |   24 +++++++++++++++---------
 1 files changed, 15 insertions(+), 9 deletions(-)

net/ipv6/datagram.c +0 -0

index 94fa6ae..47b85cc 100644
@@ -534,20 +534,26 @@ int datagram_send_ctl(struct msghdr *msg, struct

flowi *fl,

                     return -EINVAL;
                 fl->oif = src_info->ipi6_ifindex;
             }

+
+ if (fl->oif) {
+ dev = dev_get_by_index(&init_net, fl->oif);
+ if (!dev)
+ return -ENODEV;
+ }

             addr_type = ipv6_addr_type(&src_info->ipi6_addr);

- if (addr_type == IPV6_ADDR_ANY)
+ if (addr_type == IPV6_ADDR_ANY) {
+ if (dev)

+                    dev_put(dev);
                 break;

+ }

- if (addr_type & IPV6_ADDR_LINKLOCAL) {
- if (!src_info->ipi6_ifindex)
- return -EINVAL;
- else {
- dev = dev_get_by_index(&init_net,
src_info->ipi6_ifindex);
- if (!dev)
- return -ENODEV;
- }
+ if ((addr_type & IPV6_ADDR_LINKLOCAL) &&
+ !src_info->ipi6_ifindex) {
+ if (dev)
+ dev_put(dev);

+                return -EINVAL;
             }
             if (!ipv6_chk_addr(&init_net, &src_info->ipi6_addr,
                        dev, 0)) {

Signature · 1 line

1.5.4.4

YOSHIFUJI Hideaki / 吉藤英明

2008-06-02 09:17 UTC #4 lore raw

In article <[email protected]> (at Mon, 02 Jun 2008 16:59:08 +0800), Shan Wei <[email protected]> says:

Quoted 9 lines

When specifying the outgoing interface with sendmsg, if the ipi6_addr is
the unspecified address and the ipi6_ifindex is the not-exist interface, it should be fail and the errno should be set ENODEV.
Actually, it does well(sendmsg returns on success ), because the kernel
don't check the interface。
The patch is against 2.6.26-rc4.
Signed-off-by Shan Wei<[email protected]>

Patch (white spaces) is broken, but I'm going to apply this by hand. Thanks.

--yoshfuji

Brian Haley

2008-06-02 16:15 UTC #5 lore raw

In reply to Shan Wei

Quoted 4 lines · Shan Wei

When specifying the outgoing interface with sendmsg, if the ipi6_addr is
the unspecified address and the ipi6_ifindex is the not-exist interface, it should be fail and the errno should be set ENODEV.
Actually, it does well(sendmsg returns on success ), because the kernel
don't check the interface。

This patch changes this code path to be different than most others that completely ignore the device for the unspecified address - for example inet6_bind() and rawv6_bind(). Those paths only care about the device for a link-local address, so I don't think this patch is correct.

In the current git tree, this :: address is going to turn-into ::1, so the ifindex is irrelevant, the packet will be looped-back. Older kernels could transmit the packet on the wire using the default route. Can you elaborate on the problem you were seeing?

I did notice a slight problem where if fl->oif is set in the caller (for example via SO_BINDTODEVICE), it's ignored in datagram_send_ctl() if src_info->ipi6_ifindex is zero, attached patch fixes that.

-Brian

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

---

net/ipv6/datagram.c +1 -1

index 94fa6ae..b6a7c7b 100644
@@ -541,7 +541,7 @@ int datagram_send_ctl(struct msghdr *msg, struct flowi *fl,
 				break;
 
 			if (addr_type & IPV6_ADDR_LINKLOCAL) {
-				if (!src_info->ipi6_ifindex)
+				if (!fl->oif)
 					return -EINVAL;
 				else {
 					dev = dev_get_by_index(&init_net, src_info->ipi6_ifindex);

Brian Haley

2008-06-02 16:26 UTC #6 lore raw

Quoted 1 line

I did notice a slight problem where if fl->oif is set in the caller (for example via SO_BINDTODEVICE), it's ignored in datagram_send_ctl() if src_info->ipi6_ifindex is zero, attached patch fixes that.

Actually, previous patch wasn't exactly correct either, I missed one conversion, below is ok.

-Brian

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

---

net/ipv6/datagram.c +2 -2

index 94fa6ae..3abe181 100644
@@ -541,10 +541,10 @@ int datagram_send_ctl(struct msghdr *msg, struct flowi *fl,
 				break;
 
 			if (addr_type & IPV6_ADDR_LINKLOCAL) {
-				if (!src_info->ipi6_ifindex)
+				if (!fl->oif)
 					return -EINVAL;
 				else {
-					dev = dev_get_by_index(&init_net, src_info->ipi6_ifindex);
+					dev = dev_get_by_index(&init_net, fl->oif);
 					if (!dev)
 						return -ENODEV;
 				}

YOSHIFUJI Hideaki / 吉藤英明

2008-06-02 16:40 UTC #7 lore raw

In reply to Brian Haley

In article <[email protected]> (at Mon, 02 Jun 2008 12:14:58 -0400), Brian Haley <[email protected]> says:

Quoted 11 lines

Quoted 5 lines · Shan Wei
When specifying the outgoing interface with sendmsg, if the ipi6_addr is
the unspecified address and the ipi6_ifindex is the not-exist interface, it should be fail and the errno should be set ENODEV.
Actually, it does well(sendmsg returns on success ), because the kernel
don't check the interface。
This patch changes this code path to be different than most others that completely ignore the device for the unspecified address - for example inet6_bind() and rawv6_bind(). Those paths only care about the device for a link-local address, so I don't think this patch is correct.

Semantics are different. sin6_scope_id is valid ifindex only if the scope is link-local (Note: scope-id is scope-specific; for example, for "site-local" addresses, the values cannot be directly mapped onto ifindex space). On the other hand, ipi6_ifindex is always effective even if the source address is global.

Quoted 4 lines

In the current git tree, this :: address is going to turn-into ::1, so the ifindex is irrelevant, the packet will be looped-back. Older kernels could transmit the packet on the wire using the default route. Can you elaborate on the problem you were seeing?

Confused. We are not talking about destination address but source address, right?

--yoshfuji

YOSHIFUJI Hideaki / 吉藤英明

2008-06-02 16:45 UTC #8 lore raw

In article <[email protected]> (at Tue, 03 Jun 2008 01:41:05 +0900 (JST)), YOSHIFUJI Hideaki / 吉藤英明 <[email protected]> says:

Quoted 14 lines

In article <[email protected]> (at Mon, 02 Jun 2008 12:14:58 -0400), Brian Haley <[email protected]> says:
Quoted 11 lines
Quoted 5 lines · Shan Wei
When specifying the outgoing interface with sendmsg, if the ipi6_addr is
the unspecified address and the ipi6_ifindex is the not-exist interface, it should be fail and the errno should be set ENODEV.
Actually, it does well(sendmsg returns on success ), because the kernel
don't check the interface。
This patch changes this code path to be different than most others that completely ignore the device for the unspecified address - for example inet6_bind() and rawv6_bind(). Those paths only care about the device for a link-local address, so I don't think this patch is correct.

Quoted 5 lines

Semantics are different. sin6_scope_id is valid ifindex only if the scope is link-local (Note: scope-id is scope-specific; for example, for "site-local" addresses, the values cannot be directly mapped onto ifindex space). On the other hand, ipi6_ifindex is always effective even if the source address is global.

2 things:
- Check if ifindex is valid is always needed, anyway.
- Check for valid address ipv6_chk_addr() should not device-specific if the destination (or source) is link-local (or multicast, maybe).

--yoshfuji

YOSHIFUJI Hideaki / 吉藤英明

2008-06-02 16:47 UTC #9 lore raw

In article <[email protected]> (at Tue, 03 Jun 2008 01:46:04 +0900 (JST)), YOSHIFUJI Hideaki / 吉藤英明 <[email protected]> says:

Quoted 22 lines

In article <[email protected]> (at Tue, 03 Jun 2008 01:41:05 +0900 (JST)), YOSHIFUJI Hideaki / 吉藤英明 <[email protected]> says:
Quoted 14 lines
In article <[email protected]> (at Mon, 02 Jun 2008 12:14:58 -0400), Brian Haley <[email protected]> says:
Quoted 11 lines
Quoted 5 lines · Shan Wei
When specifying the outgoing interface with sendmsg, if the ipi6_addr is
the unspecified address and the ipi6_ifindex is the not-exist interface, it should be fail and the errno should be set ENODEV.
Actually, it does well(sendmsg returns on success ), because the kernel
don't check the interface。
This patch changes this code path to be different than most others that completely ignore the device for the unspecified address - for example inet6_bind() and rawv6_bind(). Those paths only care about the device for a link-local address, so I don't think this patch is correct.
Quoted 5 lines
Semantics are different. sin6_scope_id is valid ifindex only if the scope is link-local (Note: scope-id is scope-specific; for example, for "site-local" addresses, the values cannot be directly mapped onto ifindex space). On the other hand, ipi6_ifindex is always effective even if the source address is global.

Quoted 4 lines

2 things:
- Check if ifindex is valid is always needed, anyway.
- Check for valid address ipv6_chk_addr() should not device-specific

~~~delete this

Quoted 1 line

if the destination (or source) is link-local (or multicast, maybe).

--yoshfuji

YOSHIFUJI Hideaki / 吉藤英明

2008-06-02 17:06 UTC #10 lore raw

In article <[email protected]> (at Tue, 03 Jun 2008 01:48:19 +0900 (JST)), YOSHIFUJI Hideaki / 吉藤英明 <[email protected]> says:

Quoted 24 lines

In article <[email protected]> (at Tue, 03 Jun 2008 01:46:04 +0900 (JST)), YOSHIFUJI Hideaki / 吉藤英明 <[email protected]> says:
Quoted 22 lines
In article <[email protected]> (at Tue, 03 Jun 2008 01:41:05 +0900 (JST)), YOSHIFUJI Hideaki / 吉藤英明 <[email protected]> says:
Quoted 19 lines
In article <[email protected]> (at Mon, 02 Jun 2008 12:14:58 -0400), Brian Haley <[email protected]> says:
Quoted 11 lines
Quoted 5 lines · Shan Wei
When specifying the outgoing interface with sendmsg, if the ipi6_addr is
the unspecified address and the ipi6_ifindex is the not-exist interface, it should be fail and the errno should be set ENODEV.
Actually, it does well(sendmsg returns on success ), because the kernel
don't check the interface。
This patch changes this code path to be different than most others that completely ignore the device for the unspecified address - for example inet6_bind() and rawv6_bind(). Those paths only care about the device for a link-local address, so I don't think this patch is correct.
Semantics are different. sin6_scope_id is valid ifindex only if the scope is link-local (Note: scope-id is scope-specific; for example, for "site-local" addresses, the values cannot be directly mapped onto ifindex space). On the other hand, ipi6_ifindex is always effective even if the source address is global.

Quoted 6 lines

Quoted 4 lines
2 things:
- Check if ifindex is valid is always needed, anyway.
- Check for valid address ipv6_chk_addr() should not device-specific
~~~delete this
Quoted 1 line
if the destination (or source) is link-local (or multicast, maybe).

It is enough to check scope of source address, maybe.

--yoshfuji

net/ipv6/datagram.c +18 ~~-13~~

index 94fa6ae..ec1896c 100644
@@ -509,7 +509,6 @@ int datagram_send_ctl(struct msghdr *msg, struct flowi *fl,
 
 	for (cmsg = CMSG_FIRSTHDR(msg); cmsg; cmsg = CMSG_NXTHDR(msg, cmsg)) {
 		int addr_type;
-		struct net_device *dev = NULL;
 
 		if (!CMSG_OK(msg, cmsg)) {
 			err = -EINVAL;

@@ -522,6 +521,9 @@ int datagram_send_ctl(struct msghdr *msg, struct flowi *fl,
 		switch (cmsg->cmsg_type) {
 		case IPV6_PKTINFO:
 		case IPV6_2292PKTINFO:
+		    {
+			struct net_device *dev = NULL;
+
 			if (cmsg->cmsg_len < CMSG_LEN(sizeof(struct in6_pktinfo))) {
 				err = -EINVAL;
 				goto exit_f;

@@ -535,32 +537,35 @@ int datagram_send_ctl(struct msghdr *msg, struct flowi *fl,
 				fl->oif = src_info->ipi6_ifindex;
 			}
 
-			addr_type = ipv6_addr_type(&src_info->ipi6_addr);
+			if (fl->oif) {
+				dev = dev_get_by_index(&init_net, fl->oif);
+				if (!dev)
+					return -ENODEV;
+			}
 
-			if (addr_type == IPV6_ADDR_ANY)
+			addr_type = ipv6_addr_type(&src_info->ipi6_addr);
+			if (addr_type == IPV6_ADDR_ANY ||
+			    addr_type & IPV6_ADDR_MULTICAST) {
+				if (dev)
+					dev_put(dev);
 				break;
-
-			if (addr_type & IPV6_ADDR_LINKLOCAL) {
-				if (!src_info->ipi6_ifindex)
-					return -EINVAL;
-				else {
-					dev = dev_get_by_index(&init_net, src_info->ipi6_ifindex);
-					if (!dev)
-						return -ENODEV;
-				}
 			}
+
 			if (!ipv6_chk_addr(&init_net, &src_info->ipi6_addr,
-					   dev, 0)) {
+					   addr_type & IPV6_ADDR_LINKLOCAL ? dev : NULL,
+					   0)) {
 				if (dev)
 					dev_put(dev);
 				err = -EINVAL;
 				goto exit_f;
 			}
+
 			if (dev)
 				dev_put(dev);
 
 			ipv6_addr_copy(&fl->fl6_src, &src_info->ipi6_addr);
 			break;
+		    }
 
 		case IPV6_FLOWINFO:
 			if (cmsg->cmsg_len < CMSG_LEN(4)) {

Brian Haley

2008-06-02 17:39 UTC #11 lore raw

net/ipv6/datagram.c int datagram_send_ctl(struct msghdr *msg, struct flowi *fl, · line 551

Quoted 7 lines · YOSHIFUJI Hideaki / 吉藤英明

net/ipv6/datagram.c +5 -1

-			if (addr_type == IPV6_ADDR_ANY)
+			addr_type = ipv6_addr_type(&src_info->ipi6_addr);
+			if (addr_type == IPV6_ADDR_ANY ||
+			    addr_type & IPV6_ADDR_MULTICAST) {
+				if (dev)
+					dev_put(dev);
 				break;

What about link-local multicast? We should check ifindex there too. I think that check should just be for IPV6_ADDR_ANY. I think making this more like inet6_bind() and not doing the ipv6_chk_addr() call for Multicast would be the best thing, right?

-Brian

YOSHIFUJI Hideaki / 吉藤英明

2008-06-03 04:52 UTC #12 lore raw

In article <[email protected]> (at Mon, 02 Jun 2008 13:39:10 -0400), Brian Haley <[email protected]> says:

Quoted 13 lines

Review
Quoted 7 lines · YOSHIFUJI Hideaki / 吉藤英明
(quoted diff) +5 -1
-			if (addr_type == IPV6_ADDR_ANY)
+			addr_type = ipv6_addr_type(&src_info->ipi6_addr);
+			if (addr_type == IPV6_ADDR_ANY ||
+			    addr_type & IPV6_ADDR_MULTICAST) {
+				if (dev)
+					dev_put(dev);
 				break;
What about link-local multicast? We should check ifindex there too. I think that check should just be for IPV6_ADDR_ANY. I think making this more like inet6_bind() and not doing the ipv6_chk_addr() call for Multicast would be the best thing, right?

My brain was sleeping. I intended to check if the source address is NOT an multicast, but I think we can let ipv6_chk_addr() check it.

BTW we do not check if the address is valid unicast when we assign new address on interface. That does not seem good to me... (but (some?) BSDs do not seem to check this, hmm...)

---

net/ipv6/datagram.c +17 ~~-13~~

index 94fa6ae..f55269a 100644
@@ -509,7 +509,6 @@ int datagram_send_ctl(struct msghdr *msg, struct flowi *fl,
 
 	for (cmsg = CMSG_FIRSTHDR(msg); cmsg; cmsg = CMSG_NXTHDR(msg, cmsg)) {
 		int addr_type;
-		struct net_device *dev = NULL;
 
 		if (!CMSG_OK(msg, cmsg)) {
 			err = -EINVAL;

@@ -522,6 +521,9 @@ int datagram_send_ctl(struct msghdr *msg, struct flowi *fl,
 		switch (cmsg->cmsg_type) {
 		case IPV6_PKTINFO:
 		case IPV6_2292PKTINFO:
+		    {
+			struct net_device *dev = NULL;
+
 			if (cmsg->cmsg_len < CMSG_LEN(sizeof(struct in6_pktinfo))) {
 				err = -EINVAL;
 				goto exit_f;

@@ -535,32 +537,34 @@ int datagram_send_ctl(struct msghdr *msg, struct flowi *fl,
 				fl->oif = src_info->ipi6_ifindex;
 			}
 
-			addr_type = ipv6_addr_type(&src_info->ipi6_addr);
+			if (fl->oif) {
+				dev = dev_get_by_index(&init_net, fl->oif);
+				if (!dev)
+					return -ENODEV;
+			}
 
-			if (addr_type == IPV6_ADDR_ANY)
+			addr_type = ipv6_addr_type(&src_info->ipi6_addr);
+			if (addr_type == IPV6_ADDR_ANY) {
+				if (dev)
+					dev_put(dev);
 				break;
-
-			if (addr_type & IPV6_ADDR_LINKLOCAL) {
-				if (!src_info->ipi6_ifindex)
-					return -EINVAL;
-				else {
-					dev = dev_get_by_index(&init_net, src_info->ipi6_ifindex);
-					if (!dev)
-						return -ENODEV;
-				}
 			}
+
 			if (!ipv6_chk_addr(&init_net, &src_info->ipi6_addr,
-					   dev, 0)) {
+					   addr_type & IPV6_ADDR_LINKLOCAL ? dev : NULL,
+					   0)) {
 				if (dev)
 					dev_put(dev);
 				err = -EINVAL;
 				goto exit_f;
 			}
+
 			if (dev)
 				dev_put(dev);
 
 			ipv6_addr_copy(&fl->fl6_src, &src_info->ipi6_addr);
 			break;
+		    }
 
 		case IPV6_FLOWINFO:
 			if (cmsg->cmsg_len < CMSG_LEN(4)) {

--yoshfuji

Shan Wei

author

2008-06-03 07:04 UTC #13 lore raw

Quoted 15 lines · YOSHIFUJI Hideaki / 吉藤英明

In article <[email protected]> (at Mon, 02 Jun 2008 13:39:10 -0400), Brian Haley <[email protected]> says:
Quoted 12 lines
net/ipv6/datagram.c int datagram_send_ctl(struct msghdr *msg, struct flowi *fl, · line 550
Quoted 7 lines · YOSHIFUJI Hideaki / 吉藤英明
net/ipv6/datagram.c +5 -1
-			if (addr_type == IPV6_ADDR_ANY)
+			addr_type = ipv6_addr_type(&src_info->ipi6_addr);
+			if (addr_type == IPV6_ADDR_ANY ||
+			    addr_type & IPV6_ADDR_MULTICAST) {
+				if (dev)
+					dev_put(dev);
 				break;
What about link-local multicast? We should check ifindex there too. I think that check should just be for IPV6_ADDR_ANY. I think making this more like inet6_bind() and not doing the ipv6_chk_addr() call for Multicast would be the best thing, right?

Quoted 4 lines

My brain was sleeping. I intended to check if the source address is NOT an multicast, but I think we can let ipv6_chk_addr() check it.

RFC3542 6.2 says: the kernel must verify that the requested source address
is indeed a unicast address. If a multicast address is specified, what should kernel do ? returns error or choose source address by itself.

Quoted 48 lines

BTW we do not check if the address is valid unicast when we assign new address on interface. That does not seem good to me... (but (some?) BSDs do not seem to check this, hmm...)

---

net/ipv6/datagram.c +12 -7

index 94fa6ae..f55269a 100644
@@ -509,7 +509,6 @@ int datagram_send_ctl(struct msghdr *msg, struct flowi *fl,
 
 	for (cmsg = CMSG_FIRSTHDR(msg); cmsg; cmsg = CMSG_NXTHDR(msg, cmsg)) {
 		int addr_type;
-		struct net_device *dev = NULL;
 
 		if (!CMSG_OK(msg, cmsg)) {
 			err = -EINVAL;

@@ -522,6 +521,9 @@ int datagram_send_ctl(struct msghdr *msg, struct flowi *fl,
 		switch (cmsg->cmsg_type) {
 		case IPV6_PKTINFO:
 		case IPV6_2292PKTINFO:
+		    {
+			struct net_device *dev = NULL;
+
 			if (cmsg->cmsg_len < CMSG_LEN(sizeof(struct in6_pktinfo))) {
 				err = -EINVAL;
 				goto exit_f;

@@ -535,32 +537,34 @@ int datagram_send_ctl(struct msghdr *msg, struct flowi *fl,
 				fl->oif = src_info->ipi6_ifindex;
 			}
 
-			addr_type = ipv6_addr_type(&src_info->ipi6_addr);
+			if (fl->oif) {
+				dev = dev_get_by_index(&init_net, fl->oif);
+				if (!dev)
+					return -ENODEV;
+			}
 
-			if (addr_type == IPV6_ADDR_ANY)
+			addr_type = ipv6_addr_type(&src_info->ipi6_addr);
+			if (addr_type == IPV6_ADDR_ANY) {
+				if (dev)
+					dev_put(dev);
 				break;
-
-			if (addr_type & IPV6_ADDR_LINKLOCAL) {
-				if (!src_info->ipi6_ifindex)
-					return -EINVAL;

net/ipv6/datagram.c int datagram_send_ctl(struct msghdr *msg, struct flowi *fl, · line 554

Quoted 10 lines

net/ipv6/datagram.c +2 -6

-				else {
-					dev = dev_get_by_index(&init_net, src_info->ipi6_ifindex);
-					if (!dev)
-						return -ENODEV;
-				}
 			}
+
 			if (!ipv6_chk_addr(&init_net, &src_info->ipi6_addr,
-					   dev, 0)) {
+					   addr_type & IPV6_ADDR_LINKLOCAL ? dev : NULL,

if oif==0 and address is link-local.
now it does well,not returns EINVAL.

Quoted 21 lines

net/ipv6/datagram.c +3 -1

+					   0)) {
 				if (dev)
 					dev_put(dev);
 				err = -EINVAL;
 				goto exit_f;
 			}
+
 			if (dev)
 				dev_put(dev);
 
 			ipv6_addr_copy(&fl->fl6_src, &src_info->ipi6_addr);
 			break;
+		    }
 
 		case IPV6_FLOWINFO:
 			if (cmsg->cmsg_len < CMSG_LEN(4)) {
 
--yoshfuji

Signature · 15 lines

Regards
単 衛
--------------------------------------------------
Shan Wei
Development Dept.I

Nanjing Fujitsu Nanda Software Tech. Co., Ltd.(FNST)
8/F., Civil Defense Building, No.189 Guangzhou Road,
Nanjing, 210029, China
TEL:+86+25-86630566-836
FUJITSU INTERNAL:79955-836
FAX:+86+25-83317685
Mail:[email protected]
--------------------------------------------------
This communication is for use by the intended recipient(s) only and may contain information that is privileged, confidential and exempt from disclosure under applicable law. If you are not an intended recipient of this communication, you are hereby notified that any dissemination, distribution or copying hereof is strictly prohibited.  If you have received this communication in error, please notify me by reply e-mail, permanently delete this communication from your system, and destroy any hard copies you may have printed.

Shan Wei

author

2008-06-03 07:05 UTC #14 lore raw

In reply to YOSHIFUJI Hideaki / 吉藤英明

Quoted 15 lines · YOSHIFUJI Hideaki / 吉藤英明

In article <[email protected]> (at Mon, 02 Jun 2008 13:39:10 -0400), Brian Haley <[email protected]> says:
Quoted 12 lines
net/ipv6/datagram.c int datagram_send_ctl(struct msghdr *msg, struct flowi *fl, · line 550
Quoted 7 lines · YOSHIFUJI Hideaki / 吉藤英明
net/ipv6/datagram.c +5 -1
-			if (addr_type == IPV6_ADDR_ANY)
+			addr_type = ipv6_addr_type(&src_info->ipi6_addr);
+			if (addr_type == IPV6_ADDR_ANY ||
+			    addr_type & IPV6_ADDR_MULTICAST) {
+				if (dev)
+					dev_put(dev);
 				break;
What about link-local multicast? We should check ifindex there too. I think that check should just be for IPV6_ADDR_ANY. I think making this more like inet6_bind() and not doing the ipv6_chk_addr() call for Multicast would be the best thing, right?

Quoted 4 lines

My brain was sleeping. I intended to check if the source address is NOT an multicast, but I think we can let ipv6_chk_addr() check it.

Quoted 48 lines

BTW we do not check if the address is valid unicast when we assign new address on interface. That does not seem good to me... (but (some?) BSDs do not seem to check this, hmm...)

---

net/ipv6/datagram.c +12 -7

index 94fa6ae..f55269a 100644
@@ -509,7 +509,6 @@ int datagram_send_ctl(struct msghdr *msg, struct flowi *fl,
 
 	for (cmsg = CMSG_FIRSTHDR(msg); cmsg; cmsg = CMSG_NXTHDR(msg, cmsg)) {
 		int addr_type;
-		struct net_device *dev = NULL;
 
 		if (!CMSG_OK(msg, cmsg)) {
 			err = -EINVAL;

@@ -522,6 +521,9 @@ int datagram_send_ctl(struct msghdr *msg, struct flowi *fl,
 		switch (cmsg->cmsg_type) {
 		case IPV6_PKTINFO:
 		case IPV6_2292PKTINFO:
+		    {
+			struct net_device *dev = NULL;
+
 			if (cmsg->cmsg_len < CMSG_LEN(sizeof(struct in6_pktinfo))) {
 				err = -EINVAL;
 				goto exit_f;

@@ -535,32 +537,34 @@ int datagram_send_ctl(struct msghdr *msg, struct flowi *fl,
 				fl->oif = src_info->ipi6_ifindex;
 			}
 
-			addr_type = ipv6_addr_type(&src_info->ipi6_addr);
+			if (fl->oif) {
+				dev = dev_get_by_index(&init_net, fl->oif);
+				if (!dev)
+					return -ENODEV;
+			}
 
-			if (addr_type == IPV6_ADDR_ANY)
+			addr_type = ipv6_addr_type(&src_info->ipi6_addr);
+			if (addr_type == IPV6_ADDR_ANY) {
+				if (dev)
+					dev_put(dev);
 				break;
-
-			if (addr_type & IPV6_ADDR_LINKLOCAL) {
-				if (!src_info->ipi6_ifindex)
-					return -EINVAL;

net/ipv6/datagram.c int datagram_send_ctl(struct msghdr *msg, struct flowi *fl, · line 554

Quoted 10 lines

net/ipv6/datagram.c +2 -6

-				else {
-					dev = dev_get_by_index(&init_net, src_info->ipi6_ifindex);
-					if (!dev)
-						return -ENODEV;
-				}
 			}
+
 			if (!ipv6_chk_addr(&init_net, &src_info->ipi6_addr,
-					   dev, 0)) {
+					   addr_type & IPV6_ADDR_LINKLOCAL ? dev : NULL,

if oif==0 and address is link-local.
now it does well,not returns EINVAL.

Quoted 21 lines

net/ipv6/datagram.c +3 -1

+					   0)) {
 				if (dev)
 					dev_put(dev);
 				err = -EINVAL;
 				goto exit_f;
 			}
+
 			if (dev)
 				dev_put(dev);
 
 			ipv6_addr_copy(&fl->fl6_src, &src_info->ipi6_addr);
 			break;
+		    }
 
 		case IPV6_FLOWINFO:
 			if (cmsg->cmsg_len < CMSG_LEN(4)) {
 
--yoshfuji

Brian Haley

2008-06-02 17:39 UTC #15 lore raw

In reply to YOSHIFUJI Hideaki / 吉藤英明

Quoted 7 lines · YOSHIFUJI Hideaki / 吉藤英明

Quoted 4 lines
In the current git tree, this :: address is going to turn-into ::1, so the ifindex is irrelevant, the packet will be looped-back. Older kernels could transmit the packet on the wire using the default route. Can you elaborate on the problem you were seeing?
Confused. We are not talking about destination address but source address, right?

I was testing with both set to :: to see if the source address was ever getting transmitted as ::, but only noticed the destination address was on an older kernel. That was my confusion, but there is still some bug here.

-Brian