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

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

[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。
  
  The patch is against 2.6.26-rc4.

Signed-off-by: Shan Wei<shanwei@cn.fujitsu.com>
---
 net/ipv6/datagram.c |   11 ++++++-----
 1 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/net/ipv6/datagram.c b/net/ipv6/datagram.c
index 94fa6ae..a6d402b 100644
--- a/net/ipv6/datagram.c
+++ b/net/ipv6/datagram.c
@@ -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

Re: [PATCH] IPv6: fix bug when specifying the non-exist outgoing interface

[YOSHIFUJI Hideaki / 吉藤英明]

In article <4843AB51.6010206@cn.fujitsu.com> (at Mon, 02 Jun 2008 16:12:01 +0800), Shan Wei <shanwei@cn.fujitsu.com> says:

>   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

Re: [PATCH v2] IPv6: fix bug when specifying the non-exist outgoing interface

[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。
  
  The patch is against 2.6.26-rc4.

Signed-off-by: Shan Wei<shanwei@cn.fujitsu.com>

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

diff --git a/net/ipv6/datagram.c b/net/ipv6/datagram.c
index 94fa6ae..47b85cc 100644
--- a/net/ipv6/datagram.c
+++ b/net/ipv6/datagram.c
@@ -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)) {
-- 
1.5.4.4

Re: [PATCH v2] IPv6: fix bug when specifying the non-exist outgoing interface

[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。

 The patch is against 2.6.26-rc4.

Signed-off-by: Shan Wei<shanwei@cn.fujitsu.com>

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

diff --git a/net/ipv6/datagram.c b/net/ipv6/datagram.c
index 94fa6ae..47b85cc 100644
--- a/net/ipv6/datagram.c
+++ b/net/ipv6/datagram.c
@@ -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)) {
-- 
1.5.4.4

Re: [PATCH v2] IPv6: fix bug when specifying the non-exist outgoing interface

[YOSHIFUJI Hideaki / 吉藤英明]

In article <4843B65C.1060702@cn.fujitsu.com> (at Mon, 02 Jun 2008 16:59:08 +0800), Shan Wei <shanwei@cn.fujitsu.com> says:

>   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<shanwei@cn.fujitsu.com>

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

--yoshfuji

Re: [PATCH v2] IPv6: fix bug when specifying the non-exist outgoing interface

[Brian Haley]

[-- Attachment #1: Type: text/plain, Size: 1145 bytes --]

Shan Wei wrote:
>   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 <brian.haley@hp.com>
---

[-- Attachment #2: datagram_send_ctl.floif.patch --]
[-- Type: text/x-diff, Size: 426 bytes --]

diff --git a/net/ipv6/datagram.c b/net/ipv6/datagram.c
index 94fa6ae..b6a7c7b 100644
--- a/net/ipv6/datagram.c
+++ b/net/ipv6/datagram.c
@@ -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);

Re: [PATCH v2] IPv6: fix bug when specifying the non-exist outgoing interface

[Brian Haley]

[-- Attachment #1: Type: text/plain, Size: 366 bytes --]

> 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 <brian.haley@hp.com>
---

[-- Attachment #2: datagram_send_ctl.floif.patch --]
[-- Type: text/x-diff, Size: 524 bytes --]

diff --git a/net/ipv6/datagram.c b/net/ipv6/datagram.c
index 94fa6ae..3abe181 100644
--- a/net/ipv6/datagram.c
+++ b/net/ipv6/datagram.c
@@ -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;
 				}

Re: [PATCH v2] IPv6: fix bug when specifying the non-exist outgoing interface

[YOSHIFUJI Hideaki / 吉藤英明]

In article <48441C82.1070609@hp.com> (at Mon, 02 Jun 2008 12:14:58 -0400), Brian Haley <brian.haley@hp.com> says:

> Shan Wei wrote:
> >   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.

> 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

Re: [PATCH v2] IPv6: fix bug when specifying the non-exist outgoing interface

[YOSHIFUJI Hideaki / 吉藤英明]

In article <20080603.014105.73332390.yoshfuji@linux-ipv6.org> (at Tue, 03 Jun 2008 01:41:05 +0900 (JST)), YOSHIFUJI Hideaki / 吉藤英明 <yoshfuji@linux-ipv6.org> says:

> In article <48441C82.1070609@hp.com> (at Mon, 02 Jun 2008 12:14:58 -0400), Brian Haley <brian.haley@hp.com> says:
> 
> > Shan Wei wrote:
> > >   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.

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

Re: [PATCH v2] IPv6: fix bug when specifying the non-exist outgoing interface

[YOSHIFUJI Hideaki / 吉藤英明]

In article <20080603.014604.26891059.yoshfuji@linux-ipv6.org> (at Tue, 03 Jun 2008 01:46:04 +0900 (JST)), YOSHIFUJI Hideaki / 吉藤英明 <yoshfuji@linux-ipv6.org> says:

> In article <20080603.014105.73332390.yoshfuji@linux-ipv6.org> (at Tue, 03 Jun 2008 01:41:05 +0900 (JST)), YOSHIFUJI Hideaki / 吉藤英明 <yoshfuji@linux-ipv6.org> says:
> 
> > In article <48441C82.1070609@hp.com> (at Mon, 02 Jun 2008 12:14:58 -0400), Brian Haley <brian.haley@hp.com> says:
> > 
> > > Shan Wei wrote:
> > > >   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.
> 
> 2 things:
> - Check if ifindex is valid is always needed, anyway.

> - Check for valid address ipv6_chk_addr() should not device-specific
                                                   ~~~delete this
>   if the destination (or source) is link-local (or multicast, maybe).

--yoshfuji

Re: [PATCH v2] IPv6: fix bug when specifying the non-exist outgoing interface

[YOSHIFUJI Hideaki / 吉藤英明]

In article <20080603.014819.38527291.yoshfuji@linux-ipv6.org> (at Tue, 03 Jun 2008 01:48:19 +0900 (JST)), YOSHIFUJI Hideaki / 吉藤英明 <yoshfuji@linux-ipv6.org> says:

> In article <20080603.014604.26891059.yoshfuji@linux-ipv6.org> (at Tue, 03 Jun 2008 01:46:04 +0900 (JST)), YOSHIFUJI Hideaki / 吉藤英明 <yoshfuji@linux-ipv6.org> says:
> 
> > In article <20080603.014105.73332390.yoshfuji@linux-ipv6.org> (at Tue, 03 Jun 2008 01:41:05 +0900 (JST)), YOSHIFUJI Hideaki / 吉藤英明 <yoshfuji@linux-ipv6.org> says:
> > 
> > > In article <48441C82.1070609@hp.com> (at Mon, 02 Jun 2008 12:14:58 -0400), Brian Haley <brian.haley@hp.com> says:
> > > 
> > > > Shan Wei wrote:
> > > > >   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.
> > 
> > 2 things:
> > - Check if ifindex is valid is always needed, anyway.
> 
> > - Check for valid address ipv6_chk_addr() should not device-specific
>                                                    ~~~delete this
> >   if the destination (or source) is link-local (or multicast, maybe).

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

--yoshfuji

diff --git a/net/ipv6/datagram.c b/net/ipv6/datagram.c
index 94fa6ae..ec1896c 100644
--- a/net/ipv6/datagram.c
+++ b/net/ipv6/datagram.c
@@ -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)) {

Re: [PATCH v2] IPv6: fix bug when specifying the non-exist outgoing interface

[Brian Haley]

YOSHIFUJI Hideaki / 吉藤英明 wrote:
> -			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

Re: [PATCH v2] IPv6: fix bug when specifying the non-exist outgoing interface

[YOSHIFUJI Hideaki / 吉藤英明]

In article <4844303E.1040104@hp.com> (at Mon, 02 Jun 2008 13:39:10 -0400), Brian Haley <brian.haley@hp.com> says:

> YOSHIFUJI Hideaki / 吉藤英明 wrote:
> > -			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...)

---
diff --git a/net/ipv6/datagram.c b/net/ipv6/datagram.c
index 94fa6ae..f55269a 100644
--- a/net/ipv6/datagram.c
+++ b/net/ipv6/datagram.c
@@ -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

Re: [PATCH v2] IPv6: fix bug when specifying the non-exist outgoing interface

[Shan Wei]

YOSHIFUJI Hideaki / 吉藤英明 写道:
> In article <4844303E.1040104@hp.com> (at Mon, 02 Jun 2008 13:39:10 -0400), Brian Haley <brian.haley@hp.com> says:
> 
>> YOSHIFUJI Hideaki / 吉藤英明 wrote:
>>> -			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.
> 
  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.
  
  
> 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...)
> 
> ---
> diff --git a/net/ipv6/datagram.c b/net/ipv6/datagram.c
> index 94fa6ae..f55269a 100644
> --- a/net/ipv6/datagram.c
> +++ b/net/ipv6/datagram.c
> @@ -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,
if oif==0 and address is link-local.
now it does well,not returns EINVAL.


> +					   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
> 
> 
> 


-- 
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:shanwei@cn.fujitsu.com
--------------------------------------------------
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.

Re: [PATCH v2] IPv6: fix bug when specifying the non-exist outgoing interface

[Shan Wei]

YOSHIFUJI Hideaki / 吉藤英明 写道:
> In article <4844303E.1040104@hp.com> (at Mon, 02 Jun 2008 13:39:10 -0400), Brian Haley <brian.haley@hp.com> says:
> 
>> YOSHIFUJI Hideaki / 吉藤英明 wrote:
>>> -			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.
> 
  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.
  ipv6_chk_addr()
  
> 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...)
> 
> ---
> diff --git a/net/ipv6/datagram.c b/net/ipv6/datagram.c
> index 94fa6ae..f55269a 100644
> --- a/net/ipv6/datagram.c
> +++ b/net/ipv6/datagram.c
> @@ -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,
if oif==0 and address is link-local.
now it does well,not returns EINVAL.


> +					   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
> 
> 
> 

Re: [PATCH v2] IPv6: fix bug when specifying the non-exist outgoing interface

[Brian Haley]

YOSHIFUJI Hideaki / 吉藤英明 wrote:
>> 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