[iproute2] IPoIB link layer address bug

[iproute2] IPoIB link layer address bug

[James Lentini]

The ip(8) command has a bug when dealing with IPoIB link layer 
addresses. Specifically it does not correctly handle the addition of 
new entries in the neighbor/arp table. For example, this command will 
fail:

ip neigh add 192.168.0.138 lladdr 00:00:04:04:fe:80:00:00:00:00:00:00:00:01:73:00:00:00:8a:91 nud permanent dev ib0

An IPoIB link layer address is 20-bytes (see 
http://www.ietf.org/internet-drafts/draft-ietf-ipoib-ip-over-infiniband-09.txt, 
section 9.1.1).

The command line parsing code expects link layer addresses to be a 
maximum of 16-bytes. Addresses over 16-bytes are truncated.

This patch (against the iproute2 cvs repository) fixes the problem:

============================================
--- iproute2/ip/ipneigh.c.orig	2005-09-01 15:21:50.000000000 -0400
+++ iproute2/ip/ipneigh.c	2006-03-16 17:03:41.339759000 -0500
@@ -165,7 +165,7 @@ static int ipneigh_modify(int cmd, int f
 	addattr_l(&req.n, sizeof(req), NDA_DST, &dst.data, dst.bytelen);
 
 	if (lla && strcmp(lla, "null")) {
-		char llabuf[16];
+		char llabuf[20];
 		int l;
 
 		l = ll_addr_a2n(llabuf, sizeof(llabuf), lla);
============================================

P.S. - I've found a similar issue with the arp command, see

http://openib.org/pipermail/openib-general/2006-March/018270.html

Re: [iproute2] IPoIB link layer address bug

[Stephen Hemminger]

On Thu, 16 Mar 2006 17:24:41 -0500 (EST)
James Lentini <jlentini@netapp.com> wrote:

> 
> The ip(8) command has a bug when dealing with IPoIB link layer 
> addresses. Specifically it does not correctly handle the addition of 
> new entries in the neighbor/arp table. For example, this command will 
> fail:
> 
> ip neigh add 192.168.0.138 lladdr 00:00:04:04:fe:80:00:00:00:00:00:00:00:01:73:00:00:00:8a:91 nud permanent dev ib0
> 
> An IPoIB link layer address is 20-bytes (see 
> http://www.ietf.org/internet-drafts/draft-ietf-ipoib-ip-over-infiniband-09.txt, 
> section 9.1.1).
> 
> The command line parsing code expects link layer addresses to be a 
> maximum of 16-bytes. Addresses over 16-bytes are truncated.
> 
> This patch (against the iproute2 cvs repository) fixes the problem:
> 

Okay, but there are number of other places in iproute2 that call ll_addr_a2n()
with ifr.ifr_hwaddr.sa_data. And that is 14 bytes.  If you want to fix those
it will be harder since it would increase the sizeof(struct sockaddr) and potentially
break compatibility.

Re: Re: [iproute2] IPoIB link layer address bug

[Jason Gunthorpe]

On Tue, Mar 21, 2006 at 03:56:17PM -0800, Stephen Hemminger wrote:

> Okay, but there are number of other places in iproute2 that call ll_addr_a2n()
> with ifr.ifr_hwaddr.sa_data. And that is 14 bytes.  If you want to fix those
> it will be harder since it would increase the sizeof(struct sockaddr) and potentially
> break compatibility.

Maybe the best thing is to upgrade ip (and or netlink?) to use netlink
messages instead of ioctls for the remaining problematic operations.
Since netlink already supports an arbitary length hwaddr there should
be no compatability problem.

Just browsing I see usages of SIOCSIFHWBROADCAST, SIOCSIFHWADDR,
SIOCADDMULTI, SIOCDELMULTI and SIOCGIFHWADDR that use a struct ifreq..

I know SIOCGIFHWADDR can be done over netlink, but I'm not too
familiar with the others..

Jason

Re: Re: [iproute2] IPoIB link layer address bug

[James Lentini]

On Tue, 21 Mar 2006, Jason Gunthorpe wrote:

> On Tue, Mar 21, 2006 at 03:56:17PM -0800, Stephen Hemminger wrote:
> 
> > Okay, but there are number of other places in iproute2 that call 
> > ll_addr_a2n() with ifr.ifr_hwaddr.sa_data. And that is 14 bytes.  
> > If you want to fix those it will be harder since it would increase 
> > the sizeof(struct sockaddr) and potentially break compatibility.
> 
> Maybe the best thing is to upgrade ip (and or netlink?) to use 
> netlink messages instead of ioctls for the remaining problematic 
> operations. Since netlink already supports an arbitary length hwaddr 
> there should be no compatability problem.
> 
> Just browsing I see usages of SIOCSIFHWBROADCAST, SIOCSIFHWADDR, 
> SIOCADDMULTI, SIOCDELMULTI and SIOCGIFHWADDR that use a struct 
> ifreq..
> 
> I know SIOCGIFHWADDR can be done over netlink, but I'm not too 
> familiar with the others..

Making ip neighbor work with IPoIB address is what I'm interested in 
now.

As you and Jason point out there are a lot of places where ifreqs are 
used and hence options that will not support IPoIB addresses.

Do you agree with Jason's strategy of moving the ioctls to netlink 
messages (if netlink analogs exist)?

Re: Re: [iproute2] IPoIB link layer address bug

[Mark Butler]

[-- Attachment #1.1: Type: text/plain, Size: 1460 bytes --]

James Lentini wrote:

>On Tue, 21 Mar 2006, Jason Gunthorpe wrote:
>
>  
>
>>On Tue, Mar 21, 2006 at 03:56:17PM -0800, Stephen Hemminger wrote:
>>
>>    
>>
>>>Okay, but there are number of other places in iproute2 that call 
>>>ll_addr_a2n() with ifr.ifr_hwaddr.sa_data. And that is 14 bytes.  
>>>If you want to fix those it will be harder since it would increase 
>>>the sizeof(struct sockaddr) and potentially break compatibility.
>>>      
>>>
>>Maybe the best thing is to upgrade ip (and or netlink?) to use 
>>netlink messages instead of ioctls for the remaining problematic 
>>operations. Since netlink already supports an arbitary length hwaddr 
>>there should be no compatability problem.
>>
>>Just browsing I see usages of SIOCSIFHWBROADCAST, SIOCSIFHWADDR, 
>>SIOCADDMULTI, SIOCDELMULTI and SIOCGIFHWADDR that use a struct 
>>ifreq..
>>
>>I know SIOCGIFHWADDR can be done over netlink, but I'm not too 
>>familiar with the others..
>>    
>>
>
>Making ip neighbor work with IPoIB address is what I'm interested in 
>now.
>
>As you and Jason point out there are a lot of places where ifreqs are 
>used and hence options that will not support IPoIB addresses.
>
>  
>
The sockaddr union is at the end of struct ifreq.  Couldn't the union 
sockaddr members be changed to sockaddr_storage, and the SIOCxxxx 
encoded size bits be changed?  dev_ifsioc() would just need to mask out 
(or substitute) the size bits before the switch statement.

 - Mark




[-- Attachment #1.2: Type: text/html, Size: 1989 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]