Return-Path: MIME-Version: 1.0 In-Reply-To: References: <20170216171946.5105-1-luiz.dentz@gmail.com> <20170216171946.5105-6-luiz.dentz@gmail.com> From: Luiz Augusto von Dentz Date: Fri, 17 Feb 2017 11:32:21 +0200 Message-ID: Subject: Re: [PATCH v2 5/5] 6lowpan: Use netdev addr_len to determine lladdr len To: Alexander Aring Cc: "linux-bluetooth@vger.kernel.org" , Patrik Flykt , linux-wpan@vger.kernel.org Content-Type: text/plain; charset=UTF-8 Sender: linux-wpan-owner@vger.kernel.org List-ID: Hi Alex, On Fri, Feb 17, 2017 at 9:40 AM, Alexander Aring wrote: > > Hi Luiz, > > iphc code looks fine and better as before so far I see. Thanks! > > On 02/16/2017 06:19 PM, Luiz Augusto von Dentz wrote: >> From: Luiz Augusto von Dentz >> >> This allow technologies such as Bluetooth to use its native lladdr which >> is eui48 instead of eui64 which was expected by functions like >> lowpan_header_decompress and lowpan_header_compress. >> >> Signed-off-by: Luiz Augusto von Dentz >> --- >> include/net/6lowpan.h | 19 +++++++++++++++++++ >> net/6lowpan/iphc.c | 49 +++++++++++++++++++++++++++++++++++++------------ >> net/bluetooth/6lowpan.c | 43 ++++++++++--------------------------------- >> 3 files changed, 66 insertions(+), 45 deletions(-) >> >> diff --git a/include/net/6lowpan.h b/include/net/6lowpan.h >> index 5ab4c99..c5792cb 100644 >> --- a/include/net/6lowpan.h >> +++ b/include/net/6lowpan.h >> @@ -198,6 +198,25 @@ static inline void lowpan_iphc_uncompress_eui64_lladdr(struct in6_addr *ipaddr, >> ipaddr->s6_addr[8] ^= 0x02; >> } >> >> +static inline void lowpan_iphc_uncompress_eui48_lladdr(struct in6_addr *ipaddr, >> + const void *lladdr) >> +{ >> + /* fe:80::XXXX:XXff:feXX:XXXX >> + * \_________________/ >> + * hwaddr >> + */ >> + ipaddr->s6_addr[0] = 0xFE; >> + ipaddr->s6_addr[1] = 0x80; >> + memcpy(&ipaddr->s6_addr[8], lladdr, 3); >> + ipaddr->s6_addr[11] = 0xFF; >> + ipaddr->s6_addr[12] = 0xFE; >> + memcpy(&ipaddr->s6_addr[13], lladdr + 3, 3); >> + /* second bit-flip (Universe/Local) >> + * is done according RFC2464 >> + */ >> + ipaddr->s6_addr[8] ^= 0x02; >> +} >> + >> #ifdef DEBUG >> /* print data in line */ >> static inline void raw_dump_inline(const char *caller, char *msg, >> diff --git a/net/6lowpan/iphc.c b/net/6lowpan/iphc.c >> index fb5f6fa..ddbeeb7 100644 >> --- a/net/6lowpan/iphc.c >> +++ b/net/6lowpan/iphc.c >> @@ -278,6 +278,22 @@ lowpan_iphc_ctx_get_by_mcast_addr(const struct net_device *dev, >> return ret; >> } >> >> +static void lowpan_iphc_uncompress_lladdr(const struct net_device *dev, >> + struct in6_addr *ipaddr, >> + const void *lladdr) >> +{ >> + switch (dev->addr_len) { >> + case ETH_ALEN: >> + lowpan_iphc_uncompress_eui48_lladdr(ipaddr, lladdr); >> + break; >> + case EUI64_ADDR_LEN: >> + lowpan_iphc_uncompress_eui64_lladdr(ipaddr, lladdr); >> + break; >> + default: >> + WARN_ON_ONCE(1); >> + } >> +} >> + >> /* Uncompress address function for source and >> * destination address(non-multicast). >> * >> @@ -320,8 +336,7 @@ static int lowpan_iphc_uncompress_addr(struct sk_buff *skb, >> lowpan_iphc_uncompress_802154_lladdr(ipaddr, lladdr); >> break; >> default: >> - lowpan_iphc_uncompress_eui64_lladdr(ipaddr, lladdr); >> - break; > > why removing this break? > >> + lowpan_iphc_uncompress_lladdr(dev, ipaddr, lladdr); >> } >> break; >> default: >> @@ -381,7 +396,7 @@ static int lowpan_iphc_uncompress_ctx_addr(struct sk_buff *skb, >> lowpan_iphc_uncompress_802154_lladdr(ipaddr, lladdr); >> break; >> default: >> - lowpan_iphc_uncompress_eui64_lladdr(ipaddr, lladdr); >> + lowpan_iphc_uncompress_lladdr(dev, ipaddr, lladdr); >> break; >> } >> ipv6_addr_prefix_copy(ipaddr, &ctx->pfx, ctx->plen); >> @@ -810,6 +825,21 @@ lowpan_iphc_compress_ctx_802154_lladdr(const struct in6_addr *ipaddr, >> return lladdr_compress; >> } >> >> +static bool lowpan_iphc_addr_equal(const struct net_device *dev, >> + const struct lowpan_iphc_ctx *ctx, >> + const struct in6_addr *ipaddr, >> + const void *lladdr) >> +{ >> + struct in6_addr tmp = {}; >> + >> + lowpan_iphc_uncompress_lladdr(dev, &tmp, lladdr); >> + >> + if (ctx) >> + ipv6_addr_prefix_copy(&tmp, &ctx->pfx, ctx->plen); >> + >> + return ipv6_addr_equal(&tmp, ipaddr); >> +} >> + >> static u8 lowpan_compress_ctx_addr(u8 **hc_ptr, const struct net_device *dev, >> const struct in6_addr *ipaddr, >> const struct lowpan_iphc_ctx *ctx, >> @@ -827,13 +857,7 @@ static u8 lowpan_compress_ctx_addr(u8 **hc_ptr, const struct net_device *dev, >> } >> break; >> default: >> - /* check for SAM/DAM = 11 */ >> - memcpy(&tmp.s6_addr[8], lladdr, EUI64_ADDR_LEN); >> - /* second bit-flip (Universe/Local) is done according RFC2464 */ >> - tmp.s6_addr[8] ^= 0x02; >> - /* context information are always used */ >> - ipv6_addr_prefix_copy(&tmp, &ctx->pfx, ctx->plen); >> - if (ipv6_addr_equal(&tmp, ipaddr)) { >> + if (lowpan_iphc_addr_equal(dev, ctx, ipaddr, lladdr)) { >> dam = LOWPAN_IPHC_DAM_11; >> goto out; >> } >> @@ -929,11 +953,12 @@ static u8 lowpan_compress_addr_64(u8 **hc_ptr, const struct net_device *dev, >> } >> break; >> default: >> - if (is_addr_mac_addr_based(ipaddr, lladdr)) { >> - dam = LOWPAN_IPHC_DAM_11; /* 0-bits */ >> + if (lowpan_iphc_addr_equal(dev, NULL, ipaddr, lladdr)) { >> + dam = LOWPAN_IPHC_DAM_11; >> pr_debug("address compression 0 bits\n"); >> goto out; >> } >> + >> break; >> } >> >> diff --git a/net/bluetooth/6lowpan.c b/net/bluetooth/6lowpan.c >> index 1456b01..d2a1aa5 100644 >> --- a/net/bluetooth/6lowpan.c >> +++ b/net/bluetooth/6lowpan.c >> @@ -64,7 +64,7 @@ struct lowpan_peer { >> struct l2cap_chan *chan; >> >> /* peer addresses in various formats */ >> - unsigned char eui64_addr[EUI64_ADDR_LEN]; >> + unsigned char lladdr[ETH_ALEN]; >> struct in6_addr peer_addr; >> }; >> >> @@ -80,8 +80,6 @@ struct lowpan_btle_dev { >> struct delayed_work notify_peers; >> }; >> >> -static void set_addr(u8 *eui, u8 *addr, u8 addr_type); >> - >> static inline struct lowpan_btle_dev * >> lowpan_btle_dev(const struct net_device *netdev) >> { >> @@ -277,7 +275,6 @@ static int iphc_decompress(struct sk_buff *skb, struct net_device *netdev, >> const u8 *saddr; >> struct lowpan_btle_dev *dev; >> struct lowpan_peer *peer; >> - unsigned char eui64_daddr[EUI64_ADDR_LEN]; >> >> dev = lowpan_btle_dev(netdev); >> >> @@ -287,10 +284,9 @@ static int iphc_decompress(struct sk_buff *skb, struct net_device *netdev, >> if (!peer) >> return -EINVAL; >> >> - saddr = peer->eui64_addr; >> - set_addr(&eui64_daddr[0], chan->src.b, chan->src_type); >> + saddr = peer->lladdr; >> >> - return lowpan_header_decompress(skb, netdev, &eui64_daddr, saddr); >> + return lowpan_header_decompress(skb, netdev, netdev->dev_addr, saddr); >> } >> > > This is something which I don't understand. > > protoype of this function: > > int lowpan_header_decompress(struct sk_buff *skb, const struct net_device *dev, > const void *daddr, const void *saddr) > > your daddr pointer is "netdev->dev_addr", but this contains normally the L2 source > address of your transceiver. > You don't set the destination L2 address in your dev->dev_addr. (I hope) > > For me it looks like daddr == saddr, or should saddr be daddr, but then > saddr (meaning here the saddr which should named daddr then, if it is daddr) > should be as 3. parameter. This is for decompressing a received packet and it actually don't change the order of the parameters, the dev_addr is in fact the chan->src byte swapped to be used as a MAC address if the interface, there is nothing new in this regard. The only change that caused by this is that we no longer convert the MAC into an IID. > >> static void ifup(struct net_device *netdev) >> { >> int err; >> @@ -762,14 +737,16 @@ static struct l2cap_chan *add_peer_chan(struct l2cap_chan *chan, >> peer->chan = chan; >> memset(&peer->peer_addr, 0, sizeof(struct in6_addr)); >> >> + baswap((void *)peer->lladdr, &chan->dst); >> + >> /* RFC 2464 ch. 5 */ >> peer->peer_addr.s6_addr[0] = 0xFE; >> peer->peer_addr.s6_addr[1] = 0x80; >> - set_addr((u8 *)&peer->peer_addr.s6_addr + 8, chan->dst.b, >> - chan->dst_type); >> >> - memcpy(&peer->eui64_addr, (u8 *)&peer->peer_addr.s6_addr + 8, >> - EUI64_ADDR_LEN); >> + memcpy(&peer->peer_addr.s6_addr[8], peer->lladdr, 3); >> + peer->peer_addr.s6_addr[11] = 0xFF; >> + peer->peer_addr.s6_addr[12] = 0xFE; >> + memcpy(&peer->peer_addr.s6_addr[13], peer->lladdr + 3, 3); >> > > You need to explain me why you add FFFE here again, why you need to > generate an address here on ifup? This is just maintaining the same logic as there was in set_addr since we no longer have the address as 8 bytes it needed to introduce the filler bytes, I can actually replace this with a call to lowpan_iphc_uncompress_eui48_lladdr which does contain the exact same code. > Normally this should not be needed inside 6LoWPAN L2 implementation... > but I suppose this is a next step to fixing BTLE 6LoWPAN. It is perhaps not necessary to generate an IP address here, but I did not want to add unrelated changes, this might actually come in a following patch if we deem necessary. -- Luiz Augusto von Dentz