Re: [PATCH v2 5/5] 6lowpan: Use netdev addr_len to determine lladdr len

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Alex,

On Fri, Feb 17, 2017 at 9:40 AM, Alexander Aring <aar@xxxxxxxxxxxxxx> 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 <luiz.von.dentz@xxxxxxxxx>
>>
>> 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 <luiz.von.dentz@xxxxxxxxx>
>> ---
>>  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
--
To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Bluez Devel]     [Linux Wireless Networking]     [Linux Wireless Personal Area Networking]     [Linux ATH6KL]     [Linux USB Devel]     [Linux Media Drivers]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Big List of Linux Books]

  Powered by Linux