Skip to contentSkip to conversation

IPv6: Add correct padding to IPoIB link addr option

PATCH Applied Roland Dreier 5 participants 7 messages Jan 18, 2005 – Jan 19, 2005

Subsystems (3)
Networking Network drivers IPv4/IPv6
lore
Share & tools
lore

Roland Dreier

author
#0 lore raw

PATCH 1/1 IPv6: Add correct padding to IPoIB link addr option

+++ linux-bk/net/ipv6/ndisc.c 2005-01-14 21:20:55.736745091 -0800
@@ -169,12 +169,33 @@

#define NDISC_OPT_SPACE(len) (((len)+2+7)&~7)

-static u8 *ndisc_fill_option(u8 *opt, int type, void *data, int data_len) +/*
+ * Return the padding between the option length and the start of the
+ * link addr. Currently only IP-over-InfiniBand needs this, although
+ * if RFC 3831 IPv6-over-Fibre Channel is ever implemented it may
+ * also need a pad of 2.
+ */
+static int ndisc_addr_option_pad(unsigned short type)
+{
+ switch (type) {
+ case ARPHRD_INFINIBAND: return 2;
+ default: return 0;
+ }
+}
+
+static u8 *ndisc_fill_addr_option(u8 *opt, int type, void *data, int data_len,
+ unsigned short addr_type)
{
int space = NDISC_OPT_SPACE(data_len);
+ int pad = ndisc_addr_option_pad(addr_type);

opt[0] = type;
opt[1] = space>>3;
+
+ memset(opt + 2, 0, pad);
+ opt += pad;
+ space -= pad;
+
memcpy(opt+2, data, data_len); data_len += 2;
opt += data_len;
@@ -453,7 +474,8 @@
ipv6_addr_copy(&msg->target, solicited_addr);

if (inc_opt)
- ndisc_fill_option(msg->opt, ND_OPT_TARGET_LL_ADDR, dev->dev_addr, dev->addr_len);
+ ndisc_fill_addr_option(msg->opt, ND_OPT_TARGET_LL_ADDR, dev->dev_addr,
+ dev->addr_len, dev->type);

/* checksum */
msg->icmph.icmp6_cksum = csum_ipv6_magic(src_addr, daddr, len,
@@ -536,7 +558,8 @@
ipv6_addr_copy(&msg->target, solicit);

if (send_llinfo)
- ndisc_fill_option(msg->opt, ND_OPT_SOURCE_LL_ADDR, dev->dev_addr, dev->addr_len);
+ ndisc_fill_addr_option(msg->opt, ND_OPT_SOURCE_LL_ADDR, dev->dev_addr,
+ dev->addr_len, dev->type);

/* checksum */
msg->icmph.icmp6_cksum = csum_ipv6_magic(&skb->nh.ipv6h->saddr,
@@ -610,7 +633,8 @@
opt = (u8*) (hdr + 1);

if (dev->addr_len)
- ndisc_fill_option(opt, ND_OPT_SOURCE_LL_ADDR, dev->dev_addr, dev->addr_len);
+ ndisc_fill_addr_option(opt, ND_OPT_SOURCE_LL_ADDR, dev->dev_addr,
+ dev->addr_len, dev->type);

/* checksum */
hdr->icmp6_cksum = csum_ipv6_magic(&skb->nh.ipv6h->saddr, daddr, len,
@@ -717,7 +741,8 @@
}

if (ndopts.nd_opts_src_lladdr) {
- lladdr = (u8*)(ndopts.nd_opts_src_lladdr + 1);
+ lladdr = (u8*)(ndopts.nd_opts_src_lladdr + 1) +
+ ndisc_addr_option_pad(dev->type);
lladdrlen = ndopts.nd_opts_src_lladdr->nd_opt_len << 3; if (lladdrlen != NDISC_OPT_SPACE(dev->addr_len)) {
ND_PRINTK2(KERN_WARNING
@@ -874,7 +899,8 @@
return;
}
if (ndopts.nd_opts_tgt_lladdr) {
- lladdr = (u8*)(ndopts.nd_opts_tgt_lladdr + 1);
+ lladdr = (u8*)(ndopts.nd_opts_tgt_lladdr + 1) +
+ ndisc_addr_option_pad(dev->type);
lladdrlen = ndopts.nd_opts_tgt_lladdr->nd_opt_len << 3; if (lladdrlen != NDISC_OPT_SPACE(dev->addr_len)) {
ND_PRINTK2(KERN_WARNING
@@ -964,7 +990,8 @@
}

if (ndopts.nd_opts_src_lladdr) {
- lladdr = (u8 *)(ndopts.nd_opts_src_lladdr + 1);
+ lladdr = (u8 *)(ndopts.nd_opts_src_lladdr + 1) +
+ ndisc_addr_option_pad(skb->dev->type);
lladdrlen = ndopts.nd_opts_src_lladdr->nd_opt_len << 3; if (lladdrlen != NDISC_OPT_SPACE(skb->dev->addr_len))
goto out;
@@ -1130,7 +1157,8 @@
u8 *lladdr = NULL;
int lladdrlen;
if (ndopts.nd_opts_src_lladdr) {
- lladdr = (u8*)((ndopts.nd_opts_src_lladdr)+1);
+ lladdr = (u8*)((ndopts.nd_opts_src_lladdr)+1) +
+ ndisc_addr_option_pad(skb->dev->type);
lladdrlen = ndopts.nd_opts_src_lladdr->nd_opt_len << 3; if (lladdrlen != NDISC_OPT_SPACE(skb->dev->addr_len)) {
ND_PRINTK2(KERN_WARNING
@@ -1250,7 +1278,8 @@
return;
}
if (ndopts.nd_opts_tgt_lladdr) {
- lladdr = (u8*)(ndopts.nd_opts_tgt_lladdr + 1);
+ lladdr = (u8*)(ndopts.nd_opts_tgt_lladdr + 1) +
+ ndisc_addr_option_pad(skb->dev->type);
lladdrlen = ndopts.nd_opts_tgt_lladdr->nd_opt_len << 3; if (lladdrlen != NDISC_OPT_SPACE(skb->dev->addr_len)) {
ND_PRINTK2(KERN_WARNING
@@ -1379,7 +1408,8 @@
*/

if (dev->addr_len)
- opt = ndisc_fill_option(opt, ND_OPT_TARGET_LL_ADDR, neigh->ha, dev->addr_len);
+ opt = ndisc_fill_addr_option(opt, ND_OPT_TARGET_LL_ADDR, neigh->ha,
+ dev->addr_len, dev->type);

/*
* build redirect option and copy skb over to the new packet.

author
#1 lore raw

Sorry, somehow I left out the description of the patch saying why you might want to apply it....

Anand Parthasarathy pointed out that draft-ietf-ipoib-ip-over-infiniband-09.txt says:

[DISC] specifies the length of source/target option in number of 8-octets as indicated by a length of '3' above. Since the IPoIB link-layer address is only 20-octet long, two octets of zero MUST be prepended to fill the total option length of 24 octets.

The current Linux neighbour discovery code puts the padding after the link address. This patch fixes up ndisc.c to put the padding in the correct place by adding a general ndisc_addr_option_pad() function (which could be used in the future if someone ever implements RFC 3831 IPv6-over-FC or some other encapsulation that requires padding).

Signed-off-by Roland Dreier <[email protected]>
net/ipv6/ndisc.c +40 -10
@@ -169,12 +169,33 @@
 
 #define NDISC_OPT_SPACE(len) (((len)+2+7)&~7)
 
-static u8 *ndisc_fill_option(u8 *opt, int type, void *data, int data_len)
+/*
+ * Return the padding between the option length and the start of the
+ * link addr.  Currently only IP-over-InfiniBand needs this, although
+ * if RFC 3831 IPv6-over-Fibre Channel is ever implemented it may
+ * also need a pad of 2.
+ */
+static int ndisc_addr_option_pad(unsigned short type)
+{
+	switch (type) {
+	case ARPHRD_INFINIBAND: return 2;
+	default:                return 0;
+	}
+}
+
+static u8 *ndisc_fill_addr_option(u8 *opt, int type, void *data, int data_len,
+				  unsigned short addr_type)
 {
 	int space = NDISC_OPT_SPACE(data_len);
+	int pad   = ndisc_addr_option_pad(addr_type);
 
 	opt[0] = type;
 	opt[1] = space>>3;
+
+	memset(opt + 2, 0, pad);
+	opt   += pad;
+	space -= pad;
+
 	memcpy(opt+2, data, data_len);
 	data_len += 2;
 	opt += data_len;

@@ -453,7 +474,8 @@
 	ipv6_addr_copy(&msg->target, solicited_addr);
 
 	if (inc_opt)
-		ndisc_fill_option(msg->opt, ND_OPT_TARGET_LL_ADDR, dev->dev_addr, dev->addr_len);
+		ndisc_fill_addr_option(msg->opt, ND_OPT_TARGET_LL_ADDR, dev->dev_addr,
+				       dev->addr_len, dev->type);
 
 	/* checksum */
 	msg->icmph.icmp6_cksum = csum_ipv6_magic(src_addr, daddr, len, 

@@ -536,7 +558,8 @@
 	ipv6_addr_copy(&msg->target, solicit);
 
 	if (send_llinfo)
-		ndisc_fill_option(msg->opt, ND_OPT_SOURCE_LL_ADDR, dev->dev_addr, dev->addr_len);
+		ndisc_fill_addr_option(msg->opt, ND_OPT_SOURCE_LL_ADDR, dev->dev_addr,
+				       dev->addr_len, dev->type);
 
 	/* checksum */
 	msg->icmph.icmp6_cksum = csum_ipv6_magic(&skb->nh.ipv6h->saddr,

@@ -610,7 +633,8 @@
 	opt = (u8*) (hdr + 1);
 
 	if (dev->addr_len)
-		ndisc_fill_option(opt, ND_OPT_SOURCE_LL_ADDR, dev->dev_addr, dev->addr_len);
+		ndisc_fill_addr_option(opt, ND_OPT_SOURCE_LL_ADDR, dev->dev_addr,
+				       dev->addr_len, dev->type);
 
 	/* checksum */
 	hdr->icmp6_cksum = csum_ipv6_magic(&skb->nh.ipv6h->saddr, daddr, len,

@@ -717,7 +741,8 @@
 	}
 
 	if (ndopts.nd_opts_src_lladdr) {
-		lladdr = (u8*)(ndopts.nd_opts_src_lladdr + 1);
+		lladdr = (u8*)(ndopts.nd_opts_src_lladdr + 1) +
+			ndisc_addr_option_pad(dev->type);
 		lladdrlen = ndopts.nd_opts_src_lladdr->nd_opt_len << 3;
 		if (lladdrlen != NDISC_OPT_SPACE(dev->addr_len)) {
 			ND_PRINTK2(KERN_WARNING

@@ -874,7 +899,8 @@
 		return;
 	}
 	if (ndopts.nd_opts_tgt_lladdr) {
-		lladdr = (u8*)(ndopts.nd_opts_tgt_lladdr + 1);
+		lladdr = (u8*)(ndopts.nd_opts_tgt_lladdr + 1) +
+			ndisc_addr_option_pad(dev->type);
 		lladdrlen = ndopts.nd_opts_tgt_lladdr->nd_opt_len << 3;
 		if (lladdrlen != NDISC_OPT_SPACE(dev->addr_len)) {
 			ND_PRINTK2(KERN_WARNING

@@ -964,7 +990,8 @@
 	}
 
 	if (ndopts.nd_opts_src_lladdr) {
-		lladdr = (u8 *)(ndopts.nd_opts_src_lladdr + 1);
+		lladdr = (u8 *)(ndopts.nd_opts_src_lladdr + 1) +
+			ndisc_addr_option_pad(skb->dev->type);
 		lladdrlen = ndopts.nd_opts_src_lladdr->nd_opt_len << 3;
 		if (lladdrlen != NDISC_OPT_SPACE(skb->dev->addr_len))
 			goto out;

@@ -1130,7 +1157,8 @@
 		u8 *lladdr = NULL;
 		int lladdrlen;
 		if (ndopts.nd_opts_src_lladdr) {
-			lladdr = (u8*)((ndopts.nd_opts_src_lladdr)+1);
+			lladdr = (u8*)((ndopts.nd_opts_src_lladdr)+1) +
+				ndisc_addr_option_pad(skb->dev->type);
 			lladdrlen = ndopts.nd_opts_src_lladdr->nd_opt_len << 3;
 			if (lladdrlen != NDISC_OPT_SPACE(skb->dev->addr_len)) {
 				ND_PRINTK2(KERN_WARNING

@@ -1250,7 +1278,8 @@
 		return;
 	}
 	if (ndopts.nd_opts_tgt_lladdr) {
-		lladdr = (u8*)(ndopts.nd_opts_tgt_lladdr + 1);
+		lladdr = (u8*)(ndopts.nd_opts_tgt_lladdr + 1) +
+			ndisc_addr_option_pad(skb->dev->type);
 		lladdrlen = ndopts.nd_opts_tgt_lladdr->nd_opt_len << 3;
 		if (lladdrlen != NDISC_OPT_SPACE(skb->dev->addr_len)) {
 			ND_PRINTK2(KERN_WARNING

@@ -1379,7 +1408,8 @@
 	 */
 
 	if (dev->addr_len)
-		opt = ndisc_fill_option(opt, ND_OPT_TARGET_LL_ADDR, neigh->ha, dev->addr_len);
+		opt = ndisc_fill_addr_option(opt, ND_OPT_TARGET_LL_ADDR, neigh->ha,
+					     dev->addr_len, dev->type);
 
 	/*
 	 *	build redirect option and copy skb over to the new packet.
#3 lore raw
In reply to Roland Dreier

David, let me tkink about this.
Thanks.

In article <[email protected]> (at Tue, 18 Jan 2005 09:33:47 -0800), Roland Dreier <[email protected]> says:

Quoted 36 lines

+++ linux-bk/net/ipv6/ndisc.c 2005-01-14 21:20:55.736745091 -0800

(quoted diff) +22 -1
@@ -169,12 +169,33 @@
 
 #define NDISC_OPT_SPACE(len) (((len)+2+7)&~7)
 
-static u8 *ndisc_fill_option(u8 *opt, int type, void *data, int data_len)
+/*
+ * Return the padding between the option length and the start of the
+ * link addr.  Currently only IP-over-InfiniBand needs this, although
+ * if RFC 3831 IPv6-over-Fibre Channel is ever implemented it may
+ * also need a pad of 2.
+ */
+static int ndisc_addr_option_pad(unsigned short type)
+{
+	switch (type) {
+	case ARPHRD_INFINIBAND: return 2;
+	default:                return 0;
+	}
+}
+
+static u8 *ndisc_fill_addr_option(u8 *opt, int type, void *data, int data_len,
+				  unsigned short addr_type)
 {
 	int space = NDISC_OPT_SPACE(data_len);
+	int pad   = ndisc_addr_option_pad(addr_type);
 
 	opt[0] = type;
 	opt[1] = space>>3;
+
+	memset(opt + 2, 0, pad);
+	opt   += pad;
+	space -= pad;
+
 	memcpy(opt+2, data, data_len);
 	data_len += 2;
 	opt += data_len;
Signature · 2 lines
Hideaki YOSHIFUJI @ USAGI Project <[email protected]>
GPG FP: 9022 65EB 1ECF 3AD1 0BDF  80D8 4807 F894 E062 0EEA
#4 lore raw

Hi Roland,

Couple of minor comments on this :

1 : In "+static int ndisc_addr_option_pad(unsigned short type)", change the function
name to ndisc_lladdr_option_pad() and the argument name to addr_type,
to be
more precise ? addr_option seems confusing, unless it is known that
this is a LL
address option (not a big deal, though).

2. I know memset has optimized check for count, but isn't it better to check for pad
before this code since most of time it is going to be zero (unless you
are doing only
IB traffic) ?

+           if (pad) {
                  memset(opt + 2, 0, pad);
                  opt   += pad;
                  space -= pad;

+ }

3. I guess there is no clean way to avoid changing all consumers of ll_addr to not worry
about the padding after the length and before the LL address, but it
would be nice if
that were possible (eg via the ndisc_parse_options).

Also, Dave/Yoshifuji, can't ndisc_options have just nd_opt_array[5] instead of nd_opt_array[7],
with indices-1 being used to store/access the options ? In any case, I was expecting 6 rather
than 7 in the current code, since there are 5 options with the array[0] being unused.

thanks,

- KK

YOSHIFUJI Hideaki
/ 吉藤英明
<yoshfuji@linux-i To
pv6.org> [email protected],
Sent by: [email protected]
netdev-bounce@oss cc
.sgi.com [email protected],
[email protected], [email protected]
01/19/2005 05:50 Subject
AM Re: [Ipoverib] [PATCH] IPv6: Add
correct padding to IPoIB link addr option

David, let me tkink about this.
Thanks.

In article <[email protected]> (at Tue, 18 Jan 2005 09:33:47 -0800), Roland Dreier <[email protected]> says:

Quoted 1 line

+++ linux-bk/net/ipv6/ndisc.c 2005-01-14 21:20:55.736745091

-0800

Review line 172

Quoted 5 lines
(quoted diff) +0 -1
@@ -169,12 +169,33 @@
 
 #define NDISC_OPT_SPACE(len) (((len)+2+7)&~7)
 
-static u8 *ndisc_fill_option(u8 *opt, int type, void *data, int

data_len)

Quoted 5 lines
(quoted diff) +5 -0
+/*
+ * Return the padding between the option length and the start of the
+ * link addr.  Currently only IP-over-InfiniBand needs this, although
+ * if RFC 3831 IPv6-over-Fibre Channel is ever implemented it may
+ * also need a pad of 2.

Review line 186

Quoted 10 lines
(quoted diff) +10 -0
+ */
+static int ndisc_addr_option_pad(unsigned short type)
+{
+          switch (type) {
+          case ARPHRD_INFINIBAND: return 2;
+          default:                return 0;
+          }
+}
+
+static u8 *ndisc_fill_addr_option(u8 *opt, int type, void *data, int

data_len,

Review line 187

Quoted 1 line
(quoted diff) +1 -0
+                                                unsigned short

addr_type)

Quoted 4 lines
(quoted diff) +1 -0
 {
           int space = NDISC_OPT_SPACE(data_len);
+          int pad   = ndisc_addr_option_pad(addr_type);

Review line 201

Quoted 10 lines
(quoted diff) +5 -0
           opt[0] = type;
           opt[1] = space>>3;
+
+          memset(opt + 2, 0, pad);
+          opt   += pad;
+          space -= pad;
+
           memcpy(opt+2, data, data_len);
           data_len += 2;
           opt += data_len;

--
Hideaki YOSHIFUJI @ USAGI Project <[email protected]> GPG FP: 9022 65EB 1ECF 3AD1 0BDF 80D8 4807 F894 E062 0EEA

#5 lore raw

I suggest that the implementation discussion not be reflected on ipoverib. I've bcc'd to ipoverib for this mail. Those interested in the details can follow it on netdev/openib.

Thanks,
Vivek

Quoted 124 lines · Krishna Kumar

Hi Roland,

Couple of minor comments on this :

1 : In "+static int ndisc_addr_option_pad(unsigned short type)", change the function
name to ndisc_lladdr_option_pad() and the argument name to addr_type,
to be
more precise ? addr_option seems confusing, unless it is known that
this is a LL
address option (not a big deal, though).

2. I know memset has optimized check for count, but isn't it better to check for pad
before this code since most of time it is going to be zero (unless you
are doing only
IB traffic) ?

(quoted diff) +2 -0
+           if (pad) {
                  memset(opt + 2, 0, pad);
                  opt   += pad;
                  space -= pad;
+           }

3. I guess there is no clean way to avoid changing all consumers of ll_addr to not worry
about the padding after the length and before the LL address, but it
would be nice if
that were possible (eg via the ndisc_parse_options).

Also, Dave/Yoshifuji, can't ndisc_options have just nd_opt_array[5] instead of nd_opt_array[7],
with indices-1 being used to store/access the options ? In any case, I was expecting 6 rather
than 7 in the current code, since there are 5 options with the array[0] being unused.

thanks,

- KK

YOSHIFUJI Hideaki
/ $B5HF#1QL@(B
<yoshfuji@linux-i To
pv6.org> [email protected],
Sent by: [email protected]
netdev-bounce@oss cc
.sgi.com [email protected],
[email protected], [email protected]
01/19/2005 05:50 Subject
AM Re: [Ipoverib] [PATCH] IPv6: Add
correct padding to IPoIB link addr option

David, let me tkink about this.
Thanks.

In article <[email protected]> (at Tue, 18 Jan 2005 09:33:47 -0800), Roland Dreier <[email protected]> says:

Quoted 1 line

+++ linux-bk/net/ipv6/ndisc.c 2005-01-14 21:20:55.736745091

-0800

Review line 172

Quoted 5 lines
(quoted diff) +0 -1
@@ -169,12 +169,33 @@
 
 #define NDISC_OPT_SPACE(len) (((len)+2+7)&~7)
 
-static u8 *ndisc_fill_option(u8 *opt, int type, void *data, int

data_len)

Quoted 5 lines
(quoted diff) +5 -0
+/*
+ * Return the padding between the option length and the start of the
+ * link addr.  Currently only IP-over-InfiniBand needs this, although
+ * if RFC 3831 IPv6-over-Fibre Channel is ever implemented it may
+ * also need a pad of 2.

Review line 186

Quoted 10 lines
(quoted diff) +10 -0
+ */
+static int ndisc_addr_option_pad(unsigned short type)
+{
+          switch (type) {
+          case ARPHRD_INFINIBAND: return 2;
+          default:                return 0;
+          }
+}
+
+static u8 *ndisc_fill_addr_option(u8 *opt, int type, void *data, int

data_len,

Review line 187

Quoted 1 line
(quoted diff) +1 -0
+                                                unsigned short

addr_type)

Quoted 4 lines
(quoted diff) +1 -0
 {
           int space = NDISC_OPT_SPACE(data_len);
+          int pad   = ndisc_addr_option_pad(addr_type);

Review line 201

Quoted 10 lines
(quoted diff) +5 -0
           opt[0] = type;
           opt[1] = space>>3;
+
+          memset(opt + 2, 0, pad);
+          opt   += pad;
+          space -= pad;
+
           memcpy(opt+2, data, data_len);
           data_len += 2;
           opt += data_len;

--
Hideaki YOSHIFUJI @ USAGI Project <[email protected]> GPG FP: 9022 65EB 1ECF 3AD1 0BDF 80D8 4807 F894 E062 0EEA

#6 lore raw
In reply to Krishna Kumar

In article <OF89EA98A0.9BCDAC57-ON87256F8E.00204D4F-65256F8E.002B4CC8@us.ibm.com> (at Wed, 19 Jan 2005 13:19:55 +0530), Krishna Kumar <[email protected]> says:

Quoted 1 line

Couple of minor comments on this :

:

Quoted 6 lines

3. I guess there is no clean way to avoid changing all consumers of ll_addr to not worry
about the padding after the length and before the LL address, but it
would be nice if
that were possible (eg via the ndisc_parse_options).

Quoted 7 lines

Also, Dave/Yoshifuji, can't ndisc_options have just nd_opt_array[5] instead of nd_opt_array[7],
with indices-1 being used to store/access the options ? In any case, I was expecting 6 rather
than 7 in the current code, since there are 5 options with the array[0] being unused.

I'll take care of both. Thank you!

--yoshfuji