Re: [PATCH] net: Keep interface binding when sending packets with ipi_ifindex = 0

Re: [PATCH] net: Keep interface binding when sending packets with ipi_ifindex = 0

[Andrew Morton]

(cc netdev)

On Thu, 23 Jul 2009 14:39:28 -0700
Chia-chi Yeh (_________) <chiachi@android.com> wrote:

> In IPv4, sending a packet with ipi_ifindex = 0 via an interface-bound
> socket will unbind the packet to that interface. This behavior is
> different from IPv6 which treats ipi6_ifindex = 0 as unspecified.
> Furthermore, IPv6 does not allow sending packets to other interface
> than the bound one, but I am not sure if it is necessary in IPv4. The
> following patch keeps the interface binding when ipi_ifindex = 0.
> 
> Thanks,
> Chia-chi
> 
> --- a/net/ipv4/ip_sockglue.c	2009-07-24 04:50:45.000000000 +0800
> +++ b/net/ipv4/ip_sockglue.c	2009-07-24 04:51:09.000000000 +0800
> @@ -213,7 +213,8 @@
>  			if (cmsg->cmsg_len != CMSG_LEN(sizeof(struct in_pktinfo)))
>  				return -EINVAL;
>  			info = (struct in_pktinfo *)CMSG_DATA(cmsg);
> -			ipc->oif = info->ipi_ifindex;
> +			if (info->ipi_ifindex)
> +				ipc->oif = info->ipi_ifindex;
>  			ipc->addr = info->ipi_spec_dst.s_addr;
>  			break;
>  		}

Re: [PATCH] net: Keep interface binding when sending packets with ipi_ifindex = 0

[John Dykstra]

On Mon, 2009-07-27 at 14:44 -0700, Andrew Morton wrote:
> (cc netdev)
> 
> On Thu, 23 Jul 2009 14:39:28 -0700
> Chia-chi Yeh (_________) <chiachi@android.com> wrote:
> 
> > In IPv4, sending a packet with ipi_ifindex = 0 via an interface-bound
> > socket will unbind the packet to that interface. This behavior is
> > different from IPv6 which treats ipi6_ifindex = 0 as unspecified.
> > Furthermore, IPv6 does not allow sending packets to other interface
> > than the bound one, but I am not sure if it is necessary in IPv4. The
> > following patch keeps the interface binding when ipi_ifindex = 0.
> > 
> > Thanks,
> > Chia-chi
> > 
> > --- a/net/ipv4/ip_sockglue.c	2009-07-24 04:50:45.000000000 +0800
> > +++ b/net/ipv4/ip_sockglue.c	2009-07-24 04:51:09.000000000 +0800
> > @@ -213,7 +213,8 @@
> >  			if (cmsg->cmsg_len != CMSG_LEN(sizeof(struct in_pktinfo)))
> >  				return -EINVAL;
> >  			info = (struct in_pktinfo *)CMSG_DATA(cmsg);
> > -			ipc->oif = info->ipi_ifindex;
> > +			if (info->ipi_ifindex)
> > +				ipc->oif = info->ipi_ifindex;
> >  			ipc->addr = info->ipi_spec_dst.s_addr;
> >  			break;
> >  		}

A convenient copy of the man page says "When ipi_ifindex is not zero the
primary local address of the interface specified by the index overwrites
ipi_spec_dst for the routing table lookup."

But this is changing API behavior that's been this way since at least
the beginning of git history.

I guess Dave's letting this stand.  I'm posting this just to make sure
this is an explicit decision.

--
John

Re: [PATCH] net: Keep interface binding when sending packets with ipi_ifindex = 0

[David Miller]

From: John Dykstra <john.dykstra1@gmail.com>
Date: Wed, 29 Jul 2009 19:10:21 -0500

> I guess Dave's letting this stand.  I'm posting this just to make sure
> this is an explicit decision.

I'm still thinking about this.

Re: [PATCH] net: Keep interface binding when sending packets with ipi_ifindex = 0

[Chia-chi Yeh (葉家齊)]

After thinking more deeply, I believe that IPv6 does the right thing
and IPv4 does not. SO_BINDTODEVICE requires CAP_NET_RAW, so it is a
privileged operation. Therefore, it looks weird to me if one can
specify other interface than the bound one without the same
capability. The following patch makes the behavior in IPv4 and IPv6
identical. Thanks for your help.

Chia-chi

--- a/net/ipv4/ip_sockglue.c    2009-08-04 15:11:39.000000000 +0800
+++ b/net/ipv4/ip_sockglue.c    2009-08-04 15:17:05.000000000 +0800
@@ -213,7 +213,11 @@
                        if (cmsg->cmsg_len != CMSG_LEN(sizeof(struct
in_pktinfo)))
                                return -EINVAL;
                        info = (struct in_pktinfo *)CMSG_DATA(cmsg);
-                       ipc->oif = info->ipi_ifindex;
+                       if (info->ipi_ifindex) {
+                               if (ipc->oif && info->ipi_ifindex != ipc->oif)
+                                       return -EINVAL;
+                               ipc->oif = info->ipi_ifindex;
+                       }
                        ipc->addr = info->ipi_spec_dst.s_addr;
                        break;
                }

On Tue, Aug 4, 2009 at 12:23 PM, David Miller<davem@davemloft.net> wrote:
> From: John Dykstra <john.dykstra1@gmail.com>
> Date: Wed, 29 Jul 2009 19:10:21 -0500
>
>> I guess Dave's letting this stand.  I'm posting this just to make sure
>> this is an explicit decision.
>
> I'm still thinking about this.
>

Re: [PATCH] net: Keep interface binding when sending packets with ipi_ifindex = 0

[Badalian Vyacheslav]

Chia-chi Yeh (葉家齊) пишет:
> After thinking more deeply, I believe that IPv6 does the right thing
> and IPv4 does not. SO_BINDTODEVICE requires CAP_NET_RAW, so it is a
> privileged operation. Therefore, it looks weird to me if one can
> specify other interface than the bound one without the same
> capability. The following patch makes the behavior in IPv4 and IPv6
> identical. Thanks for your help.
> 
> Chia-chi
> 
> --- a/net/ipv4/ip_sockglue.c    2009-08-04 15:11:39.000000000 +0800
> +++ b/net/ipv4/ip_sockglue.c    2009-08-04 15:17:05.000000000 +0800
> @@ -213,7 +213,11 @@
>                         if (cmsg->cmsg_len != CMSG_LEN(sizeof(struct
> in_pktinfo)))
>                                 return -EINVAL;
>                         info = (struct in_pktinfo *)CMSG_DATA(cmsg);
> -                       ipc->oif = info->ipi_ifindex;
> +                       if (info->ipi_ifindex) {
> +                               if (ipc->oif && info->ipi_ifindex != ipc->oif)
> +                                       return -EINVAL;
> +                               ipc->oif = info->ipi_ifindex;

Hello
Sorry if its my mistake or i someone not understand :)

if (ipc->oif && info->ipi_ifindex != ipc->oif)
	// if match: info->ipi_ifindex != ipc->oif
	return ...
else
	// else match: info->ipi_ifindex == ipc->oif
	// but you do 
	ipc->oif = info->ipi_ifindex; 
	// why if you else match allready check for it?

Thanks



> +                       }
>                         ipc->addr = info->ipi_spec_dst.s_addr;
>                         break;
>                 }
> 
> On Tue, Aug 4, 2009 at 12:23 PM, David Miller<davem@davemloft.net> wrote:
>> From: John Dykstra <john.dykstra1@gmail.com>
>> Date: Wed, 29 Jul 2009 19:10:21 -0500
>>
>>> I guess Dave's letting this stand.  I'm posting this just to make sure
>>> this is an explicit decision.
>> I'm still thinking about this.
>>
> --
> 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
> 
> 

Re: [PATCH] net: Keep interface binding when sending packets with ipi_ifindex = 0

[Chia-chi Yeh (葉家齊)]

On Tue, Aug 4, 2009 at 3:57 PM, Badalian Vyacheslav<slavon@bigtelecom.ru> wrote:
>
> Hello
> Sorry if its my mistake or i someone not understand :)
>
> if (ipc->oif && info->ipi_ifindex != ipc->oif)
>        // if match: info->ipi_ifindex != ipc->oif
>        return ...
> else
>        // else match: info->ipi_ifindex == ipc->oif
>        // but you do
>        ipc->oif = info->ipi_ifindex;
>        // why if you else match allready check for it?
>
> Thanks

Hi Badalian,

Consider ipc->oif = 0 before the if-condition. :)

Chia-chi

Re: [PATCH] net: Keep interface binding when sending packets with ipi_ifindex = 0

[David Miller]

From: Chia-chi Yeh (葉家齊) <chiachi@android.com>
Date: Tue, 4 Aug 2009 15:36:46 +0800

> After thinking more deeply, I believe that IPv6 does the right thing
> and IPv4 does not. SO_BINDTODEVICE requires CAP_NET_RAW, so it is a
> privileged operation. Therefore, it looks weird to me if one can
> specify other interface than the bound one without the same
> capability. The following patch makes the behavior in IPv4 and IPv6
> identical. Thanks for your help.

I think we really cannot change behavior here.  If the user specifies
"0" in ipi_ifindex we must respect that in ipc->oif.  This is an
override, and the ability to override is the very purpose of this
control message.

Even GLIBC makes use of that case of specifying "0" in ipi_ifindex.
We must respect it.

I'm not applying any of these patches, sorry.

Re: [PATCH] net: Keep interface binding when sending packets with ipi_ifindex = 0

[Chia-chi Yeh (葉家齊)]

2009/8/5 David Miller <davem@davemloft.net>:
> I think we really cannot change behavior here.  If the user specifies
> "0" in ipi_ifindex we must respect that in ipc->oif.  This is an
> override, and the ability to override is the very purpose of this
> control message.
>
> Even GLIBC makes use of that case of specifying "0" in ipi_ifindex.
> We must respect it.
>
> I'm not applying any of these patches, sorry.
>

If you treat ipi_ifindex as an override, do you want to do that in
ipi6_ifindex as well? Also, CAP_NET_RAW check for SO_BINDTODEVICE
becomes meaningless in this case.

I did not find the usage of ipi_ifindex in glibc. It would be great if
you can give me some pointers. Thanks for your help.

Chia-chi

Re: [PATCH] net: Keep interface binding when sending packets with ipi_ifindex = 0

[David Miller]

From: Chia-chi Yeh (葉家齊) <chiachi@android.com>
Date: Wed, 5 Aug 2009 08:06:25 +0800

> 2009/8/5 David Miller <davem@davemloft.net>:
>> I think we really cannot change behavior here.  If the user specifies
>> "0" in ipi_ifindex we must respect that in ipc->oif.  This is an
>> override, and the ability to override is the very purpose of this
>> control message.
>>
>> Even GLIBC makes use of that case of specifying "0" in ipi_ifindex.
>> We must respect it.
>>
>> I'm not applying any of these patches, sorry.
>>
> 
> If you treat ipi_ifindex as an override, do you want to do that in
> ipi6_ifindex as well? Also, CAP_NET_RAW check for SO_BINDTODEVICE
> becomes meaningless in this case.

ipi_ifindex influences routing decisions, it doesn't also influence
socket matching like SO_BINDTODEVICE does.

That's the crucial difference.

This interface index gets passed into the routing lookup and that is
completely harmless to allow users to do.

About ipv6 it is indeed an important issue, because apps that work now
with ipv4's behavior are going to break if they care about this case
when they start using ipv6.  Probably the thing to do is make ipv6
work the same as ipv4.

> I did not find the usage of ipi_ifindex in glibc. It would be great if
> you can give me some pointers. Thanks for your help.

If grep is broken on your system, I'm sorry to hear that:

	find . -type f | xargs egrep IP_PKTINFO

Give that a try on your glibc tree.