[PATCH linux-wpan v4] ieee802154: 6lowpan: ensure header compression does not corrupt ipv6 header

[PATCH linux-wpan v4] ieee802154: 6lowpan: ensure header compression does not corrupt ipv6 header

[Simon Vincent]

The 6lowpan ipv6 header compression was causing problems for other interfaces
that expected a ipv6 header to still be in place, as we were replacing the
ipv6 header with a compressed version. This happened if you sent a packet to a
multicast address as the packet would be output on 802.15.4, ethernet, and also
be sent to the loopback interface. The skb data was shared between these
interfaces so all interfaces ended up with a compressed ipv6 header.
The solution is to ensure that before we do any header compression we are not
sharing the skb or skb data with any other interface. If we are then we must
take a copy of the skb and skb data before modifying the ipv6 header.
The only place we can copy the skb is inside the xmit function so we don't
leave dangling references to skb.
This patch moves all the header compression to inside the xmit function. Very
little code has been changed it has mostly been moved from lowpan_header_create
to lowpan_xmit. At the top of the xmit function we now check if the skb is
shared and if so copy it. In lowpan_header_create all we do now is store the
source and destination addresses for use later when we compress the header.

Signed-off-by: Simon Vincent <simon.vincent@xsilon.com>
---

v4 - fixed patch style checks


 net/ieee802154/6lowpan_rtnl.c | 125 ++++++++++++++++++++++++++++++------------
 1 file changed, 89 insertions(+), 36 deletions(-)

diff --git a/net/ieee802154/6lowpan_rtnl.c b/net/ieee802154/6lowpan_rtnl.c
index 6591d27..ca7ef0e 100644
--- a/net/ieee802154/6lowpan_rtnl.c
+++ b/net/ieee802154/6lowpan_rtnl.c
@@ -71,6 +71,21 @@ struct lowpan_dev_record {
 	struct list_head list;
 };
 
+/* don't save pan id, it's intra pan */
+struct lowpan_addr {
+	__le16 mode;
+	union lowpan_addr_u {
+		/* IPv6 needs big endian here */
+		__be64 extended_addr;
+		__be16 short_addr;
+	} u;
+};
+
+struct lowpan_addr_info {
+	struct lowpan_addr daddr;
+	struct lowpan_addr saddr;
+};
+
 static inline struct
 lowpan_dev_info *lowpan_dev_info(const struct net_device *dev)
 {
@@ -85,14 +100,21 @@ static inline void lowpan_address_flip(u8 *src, u8 *dest)
 		(dest)[IEEE802154_ADDR_LEN - i - 1] = (src)[i];
 }
 
+static inline struct
+lowpan_addr_info *lowpan_skb_priv(const struct sk_buff *skb)
+{
+	WARN_ON_ONCE(skb_headroom(skb) < sizeof(struct lowpan_addr_info));
+	return (struct lowpan_addr_info *)(skb->data -
+			sizeof(struct lowpan_addr_info));
+}
+
 static int lowpan_header_create(struct sk_buff *skb, struct net_device *dev,
 				unsigned short type, const void *_daddr,
 				const void *_saddr, unsigned int len)
 {
 	const u8 *saddr = _saddr;
 	const u8 *daddr = _daddr;
-	struct ieee802154_addr sa, da;
-	struct ieee802154_mac_cb *cb = mac_cb_init(skb);
+	struct lowpan_addr_info *info;
 
 	/* TODO:
 	 * if this package isn't ipv6 one, where should it be routed?
@@ -106,41 +128,17 @@ static int lowpan_header_create(struct sk_buff *skb, struct net_device *dev,
 	raw_dump_inline(__func__, "saddr", (unsigned char *)saddr, 8);
 	raw_dump_inline(__func__, "daddr", (unsigned char *)daddr, 8);
 
-	lowpan_header_compress(skb, dev, type, daddr, saddr, len);
-
-	/* NOTE1: I'm still unsure about the fact that compression and WPAN
-	 * header are created here and not later in the xmit. So wait for
-	 * an opinion of net maintainers.
-	 */
-	/* NOTE2: to be absolutely correct, we must derive PANid information
-	 * from MAC subif of the 'dev' and 'real_dev' network devices, but
-	 * this isn't implemented in mainline yet, so currently we assign 0xff
-	 */
-	cb->type = IEEE802154_FC_TYPE_DATA;
-
-	/* prepare wpan address data */
-	sa.mode = IEEE802154_ADDR_LONG;
-	sa.pan_id = ieee802154_mlme_ops(dev)->get_pan_id(dev);
-	sa.extended_addr = ieee802154_devaddr_from_raw(saddr);
-
-	/* intra-PAN communications */
-	da.pan_id = sa.pan_id;
-
-	/* if the destination address is the broadcast address, use the
-	 * corresponding short address
-	 */
-	if (lowpan_is_addr_broadcast(daddr)) {
-		da.mode = IEEE802154_ADDR_SHORT;
-		da.short_addr = cpu_to_le16(IEEE802154_ADDR_BROADCAST);
-	} else {
-		da.mode = IEEE802154_ADDR_LONG;
-		da.extended_addr = ieee802154_devaddr_from_raw(daddr);
-	}
+	info = lowpan_skb_priv(skb);
 
-	cb->ackreq = !lowpan_is_addr_broadcast(daddr);
+	/* TODO: Currently we only support extended_addr */
+	info->daddr.mode = IEEE802154_ADDR_LONG;
+	memcpy(&info->daddr.u.extended_addr, daddr,
+	       sizeof(info->daddr.u.extended_addr));
+	info->saddr.mode = IEEE802154_ADDR_LONG;
+	memcpy(&info->saddr.u.extended_addr, saddr,
+	       sizeof(info->daddr.u.extended_addr));
 
-	return dev_hard_header(skb, lowpan_dev_info(dev)->real_dev,
-			type, (void *)&da, (void *)&sa, 0);
+	return 0;
 }
 
 static int lowpan_give_skb_to_devices(struct sk_buff *skb,
@@ -338,13 +336,68 @@ err:
 	return rc;
 }
 
+static int lowpan_header(struct sk_buff *skb, struct net_device *dev)
+{
+	struct ieee802154_addr sa, da;
+	struct ieee802154_mac_cb *cb = mac_cb_init(skb);
+	struct lowpan_addr_info info;
+	void *daddr, *saddr;
+
+	memcpy(&info, lowpan_skb_priv(skb), sizeof(info));
+
+	/* TODO: Currently we only support extended_addr */
+	daddr = &info.daddr.u.extended_addr;
+	saddr = &info.saddr.u.extended_addr;
+
+	lowpan_header_compress(skb, dev, ETH_P_IPV6, daddr, saddr, skb->len);
+
+	cb->type = IEEE802154_FC_TYPE_DATA;
+
+	/* prepare wpan address data */
+	sa.mode = IEEE802154_ADDR_LONG;
+	sa.pan_id = ieee802154_mlme_ops(dev)->get_pan_id(dev);
+	sa.extended_addr = ieee802154_devaddr_from_raw(saddr);
+
+	/* intra-PAN communications */
+	da.pan_id = sa.pan_id;
+
+	/* if the destination address is the broadcast address, use the
+	 * corresponding short address
+	 */
+	if (lowpan_is_addr_broadcast((const u8 *)daddr)) {
+		da.mode = IEEE802154_ADDR_SHORT;
+		da.short_addr = cpu_to_le16(IEEE802154_ADDR_BROADCAST);
+		cb->ackreq = false;
+	} else {
+		da.mode = IEEE802154_ADDR_LONG;
+		da.extended_addr = ieee802154_devaddr_from_raw(daddr);
+		cb->ackreq = true;
+	}
+
+	return dev_hard_header(skb, lowpan_dev_info(dev)->real_dev,
+			ETH_P_IPV6, (void *)&da, (void *)&sa, 0);
+}
+
 static netdev_tx_t lowpan_xmit(struct sk_buff *skb, struct net_device *dev)
 {
 	struct ieee802154_hdr wpan_hdr;
-	int max_single;
+	int max_single, ret;
 
 	pr_debug("package xmit\n");
 
+	/* We must take a copy of the skb before we modify/replace the ipv6
+	 * header as the header could be used elsewhere
+	 */
+	skb = skb_unshare(skb, GFP_ATOMIC);
+	if (!skb)
+		return NET_XMIT_DROP;
+
+	ret = lowpan_header(skb, dev);
+	if (ret < 0) {
+		kfree_skb(skb);
+		return NET_XMIT_DROP;
+	}
+
 	if (ieee802154_hdr_peek(skb, &wpan_hdr) < 0) {
 		kfree_skb(skb);
 		return NET_XMIT_DROP;
-- 
1.9.1

Re: [PATCH linux-wpan v4] ieee802154: 6lowpan: ensure header compression does not corrupt ipv6 header

[Alexander Aring]

Hi Simon,

grml, I overlooked something the last version. But I will fix it while
applying if all seems to be okay for all other guys/girls.

On Mon, Sep 22, 2014 at 04:52:03PM +0100, Simon Vincent wrote:
> The 6lowpan ipv6 header compression was causing problems for other interfaces
> that expected a ipv6 header to still be in place, as we were replacing the
> ipv6 header with a compressed version. This happened if you sent a packet to a
> multicast address as the packet would be output on 802.15.4, ethernet, and also
> be sent to the loopback interface. The skb data was shared between these
> interfaces so all interfaces ended up with a compressed ipv6 header.
> The solution is to ensure that before we do any header compression we are not
> sharing the skb or skb data with any other interface. If we are then we must
> take a copy of the skb and skb data before modifying the ipv6 header.
> The only place we can copy the skb is inside the xmit function so we don't
> leave dangling references to skb.
> This patch moves all the header compression to inside the xmit function. Very
> little code has been changed it has mostly been moved from lowpan_header_create
> to lowpan_xmit. At the top of the xmit function we now check if the skb is
> shared and if so copy it. In lowpan_header_create all we do now is store the
> source and destination addresses for use later when we compress the header.
> 
> Signed-off-by: Simon Vincent <simon.vincent@xsilon.com>
> ---
> 
> v4 - fixed patch style checks
> 
> 
>  net/ieee802154/6lowpan_rtnl.c | 125 ++++++++++++++++++++++++++++++------------
>  1 file changed, 89 insertions(+), 36 deletions(-)
> 
> diff --git a/net/ieee802154/6lowpan_rtnl.c b/net/ieee802154/6lowpan_rtnl.c
> index 6591d27..ca7ef0e 100644
> --- a/net/ieee802154/6lowpan_rtnl.c
> +++ b/net/ieee802154/6lowpan_rtnl.c
> @@ -71,6 +71,21 @@ struct lowpan_dev_record {
>  	struct list_head list;
>  };
>  
> +/* don't save pan id, it's intra pan */
> +struct lowpan_addr {
> +	__le16 mode;

In the rework I made this __le16 because this value comes directly from
mac header and contain the masked frame control fields for the
destination address mode and source address mode and 802.15.4 header
is little byteorder. I know it's very confusing because 802.15.4 is
little endian and IPv6 is big endian, oh yes that was a very good idea
to make it little endian. ;-)

We should never ever convert this value into some host byteorder, this
cost time and netdev people doesn't like that (I already saw some
patches which came not mainline because that).

performance example:

if (__le16_mode == cpu_to_le16(IEEE802154_FCTL_DADDR_EXTENDED))

the byte ordering is determined at compile time.

Doing this like:

if (le16_to_cpu(__le16_mode) == IEEE802154_FCTL_DADDR_EXTENDED))

is determined at runtime. <-- cost time, also compiler can't detect it.
Also this is shift operation, if we do compile time determined then we
only have some compare and "maybe" mask operators. Shift operation cost
some time on several architectures which have shifts as pseudo
instruction (like mips). (converting byte order is some mixing shift and
masking operations).

And the netdev people doesn't like that because, I don't know it's
usually to keep the byteorder from protocol to avoid confusing.

Nevertheless:
this should in this case u8 because somebody converted it into some cpu
understandable value. <--- this we should not do on parsing. I
will remove the converting to any host byteorder in the rework.

See below for the example.

> +	union lowpan_addr_u {
> +		/* IPv6 needs big endian here */
> +		__be64 extended_addr;
> +		__be16 short_addr;
> +	} u;
> +};
> +
> +struct lowpan_addr_info {
> +	struct lowpan_addr daddr;
> +	struct lowpan_addr saddr;
> +};
> +
>  static inline struct
>  lowpan_dev_info *lowpan_dev_info(const struct net_device *dev)
>  {
> @@ -85,14 +100,21 @@ static inline void lowpan_address_flip(u8 *src, u8 *dest)
>  		(dest)[IEEE802154_ADDR_LEN - i - 1] = (src)[i];
>  }
>  
> +static inline struct
> +lowpan_addr_info *lowpan_skb_priv(const struct sk_buff *skb)
> +{
> +	WARN_ON_ONCE(skb_headroom(skb) < sizeof(struct lowpan_addr_info));
> +	return (struct lowpan_addr_info *)(skb->data -
> +			sizeof(struct lowpan_addr_info));
> +}
> +
>  static int lowpan_header_create(struct sk_buff *skb, struct net_device *dev,
>  				unsigned short type, const void *_daddr,
>  				const void *_saddr, unsigned int len)
>  {
>  	const u8 *saddr = _saddr;
>  	const u8 *daddr = _daddr;
> -	struct ieee802154_addr sa, da;
> -	struct ieee802154_mac_cb *cb = mac_cb_init(skb);
> +	struct lowpan_addr_info *info;
>  
>  	/* TODO:
>  	 * if this package isn't ipv6 one, where should it be routed?
> @@ -106,41 +128,17 @@ static int lowpan_header_create(struct sk_buff *skb, struct net_device *dev,
>  	raw_dump_inline(__func__, "saddr", (unsigned char *)saddr, 8);
>  	raw_dump_inline(__func__, "daddr", (unsigned char *)daddr, 8);
>  
> -	lowpan_header_compress(skb, dev, type, daddr, saddr, len);
> -
> -	/* NOTE1: I'm still unsure about the fact that compression and WPAN
> -	 * header are created here and not later in the xmit. So wait for
> -	 * an opinion of net maintainers.
> -	 */
> -	/* NOTE2: to be absolutely correct, we must derive PANid information
> -	 * from MAC subif of the 'dev' and 'real_dev' network devices, but
> -	 * this isn't implemented in mainline yet, so currently we assign 0xff
> -	 */
> -	cb->type = IEEE802154_FC_TYPE_DATA;
> -
> -	/* prepare wpan address data */
> -	sa.mode = IEEE802154_ADDR_LONG;
> -	sa.pan_id = ieee802154_mlme_ops(dev)->get_pan_id(dev);
> -	sa.extended_addr = ieee802154_devaddr_from_raw(saddr);
> -
> -	/* intra-PAN communications */
> -	da.pan_id = sa.pan_id;
> -
> -	/* if the destination address is the broadcast address, use the
> -	 * corresponding short address
> -	 */
> -	if (lowpan_is_addr_broadcast(daddr)) {
> -		da.mode = IEEE802154_ADDR_SHORT;
> -		da.short_addr = cpu_to_le16(IEEE802154_ADDR_BROADCAST);
> -	} else {
> -		da.mode = IEEE802154_ADDR_LONG;
> -		da.extended_addr = ieee802154_devaddr_from_raw(daddr);
> -	}
> +	info = lowpan_skb_priv(skb);
>  
> -	cb->ackreq = !lowpan_is_addr_broadcast(daddr);
> +	/* TODO: Currently we only support extended_addr */
> +	info->daddr.mode = IEEE802154_ADDR_LONG;

now IEEE802154_ADDR_LONG is no little endian value copied from mac
header. Somewhere else converted the little endian value to an host understandable
enum/define number. For the rework I will remove this value. We should never
ever convert this value for parsing.

We use it only for af_802154 and userspace specification what addresses
should be used. But the above one to make this __le16 is incorrect. For
userspace we should create some enums for declaring different address
types. Then we can use this for 802.15.4 sockets.

I will fix it while applying when all things are okay, it's only to
replace the __le16 to u8.


And I also know that 6LoWPAN GENERIC need this value for determined is EUI64
or short. I don't forget this one, wasn't full finished at rework
branch.

- Alex

Re: [PATCH linux-wpan v4] ieee802154: 6lowpan: ensure header compression does not corrupt ipv6 header

[Alexander Aring]

On Mon, Sep 22, 2014 at 06:21:48PM +0200, Alexander Aring wrote:
> Hi Simon,
> 
> grml, I overlooked something the last version. But I will fix it while
> applying if all seems to be okay for all other guys/girls.
> 
> On Mon, Sep 22, 2014 at 04:52:03PM +0100, Simon Vincent wrote:
> > The 6lowpan ipv6 header compression was causing problems for other interfaces
> > that expected a ipv6 header to still be in place, as we were replacing the
> > ipv6 header with a compressed version. This happened if you sent a packet to a
> > multicast address as the packet would be output on 802.15.4, ethernet, and also
> > be sent to the loopback interface. The skb data was shared between these
> > interfaces so all interfaces ended up with a compressed ipv6 header.
> > The solution is to ensure that before we do any header compression we are not
> > sharing the skb or skb data with any other interface. If we are then we must
> > take a copy of the skb and skb data before modifying the ipv6 header.
> > The only place we can copy the skb is inside the xmit function so we don't
> > leave dangling references to skb.
> > This patch moves all the header compression to inside the xmit function. Very
> > little code has been changed it has mostly been moved from lowpan_header_create
> > to lowpan_xmit. At the top of the xmit function we now check if the skb is
> > shared and if so copy it. In lowpan_header_create all we do now is store the
> > source and destination addresses for use later when we compress the header.
> > 
> > Signed-off-by: Simon Vincent <simon.vincent@xsilon.com>
> > ---
> > 
> > v4 - fixed patch style checks
> > 
> > 
> >  net/ieee802154/6lowpan_rtnl.c | 125 ++++++++++++++++++++++++++++++------------
> >  1 file changed, 89 insertions(+), 36 deletions(-)
> > 
> > diff --git a/net/ieee802154/6lowpan_rtnl.c b/net/ieee802154/6lowpan_rtnl.c
> > index 6591d27..ca7ef0e 100644
> > --- a/net/ieee802154/6lowpan_rtnl.c
> > +++ b/net/ieee802154/6lowpan_rtnl.c
> > @@ -71,6 +71,21 @@ struct lowpan_dev_record {
> >  	struct list_head list;
> >  };
> >  
> > +/* don't save pan id, it's intra pan */
> > +struct lowpan_addr {
> > +	__le16 mode;
> 
> In the rework I made this __le16 because this value comes directly from
> mac header and contain the masked frame control fields for the

sorry, this is sending part. So correct would no parsing, just set the
value at begin of generation of mac header to the frame control field.
So we never need to convert this value again.

This is also for parsing at receiving side, then we just copy this value.

- Alex

Re: [PATCH linux-wpan v4] ieee802154: 6lowpan: ensure header compression does not corrupt ipv6 header

[Alexander Aring]

and just another hints to explain what exactly going on there.

On Mon, Sep 22, 2014 at 04:52:03PM +0100, Simon Vincent wrote:
> The 6lowpan ipv6 header compression was causing problems for other interfaces
> that expected a ipv6 header to still be in place, as we were replacing the
> ipv6 header with a compressed version. This happened if you sent a packet to a
> multicast address as the packet would be output on 802.15.4, ethernet, and also
> be sent to the loopback interface. The skb data was shared between these
> interfaces so all interfaces ended up with a compressed ipv6 header.
> The solution is to ensure that before we do any header compression we are not
> sharing the skb or skb data with any other interface. If we are then we must
> take a copy of the skb and skb data before modifying the ipv6 header.
> The only place we can copy the skb is inside the xmit function so we don't
> leave dangling references to skb.
> This patch moves all the header compression to inside the xmit function. Very
> little code has been changed it has mostly been moved from lowpan_header_create
> to lowpan_xmit. At the top of the xmit function we now check if the skb is
> shared and if so copy it. In lowpan_header_create all we do now is store the
> source and destination addresses for use later when we compress the header.
> 
> Signed-off-by: Simon Vincent <simon.vincent@xsilon.com>
> ---
> 
> v4 - fixed patch style checks
> 
> 
>  net/ieee802154/6lowpan_rtnl.c | 125 ++++++++++++++++++++++++++++++------------
>  1 file changed, 89 insertions(+), 36 deletions(-)
> 
> diff --git a/net/ieee802154/6lowpan_rtnl.c b/net/ieee802154/6lowpan_rtnl.c
> index 6591d27..ca7ef0e 100644
> --- a/net/ieee802154/6lowpan_rtnl.c
> +++ b/net/ieee802154/6lowpan_rtnl.c
> @@ -71,6 +71,21 @@ struct lowpan_dev_record {
>  	struct list_head list;
>  };
>  
> +/* don't save pan id, it's intra pan */
> +struct lowpan_addr {
> +	__le16 mode;
> +	union lowpan_addr_u {
> +		/* IPv6 needs big endian here */
> +		__be64 extended_addr;
> +		__be16 short_addr;
> +	} u;
> +};
> +
> +struct lowpan_addr_info {
> +	struct lowpan_addr daddr;
> +	struct lowpan_addr saddr;
> +};
> +
>  static inline struct
>  lowpan_dev_info *lowpan_dev_info(const struct net_device *dev)
>  {
> @@ -85,14 +100,21 @@ static inline void lowpan_address_flip(u8 *src, u8 *dest)
>  		(dest)[IEEE802154_ADDR_LEN - i - 1] = (src)[i];
>  }
>  
> +static inline struct
> +lowpan_addr_info *lowpan_skb_priv(const struct sk_buff *skb)
> +{
> +	WARN_ON_ONCE(skb_headroom(skb) < sizeof(struct lowpan_addr_info));
> +	return (struct lowpan_addr_info *)(skb->data -
> +			sizeof(struct lowpan_addr_info));
> +}
> +
>  static int lowpan_header_create(struct sk_buff *skb, struct net_device *dev,
>  				unsigned short type, const void *_daddr,
>  				const void *_saddr, unsigned int len)
>  {
>  	const u8 *saddr = _saddr;
>  	const u8 *daddr = _daddr;
> -	struct ieee802154_addr sa, da;
> -	struct ieee802154_mac_cb *cb = mac_cb_init(skb);
> +	struct lowpan_addr_info *info;
>  
>  	/* TODO:
>  	 * if this package isn't ipv6 one, where should it be routed?
> @@ -106,41 +128,17 @@ static int lowpan_header_create(struct sk_buff *skb, struct net_device *dev,
>  	raw_dump_inline(__func__, "saddr", (unsigned char *)saddr, 8);
>  	raw_dump_inline(__func__, "daddr", (unsigned char *)daddr, 8);
>  
> -	lowpan_header_compress(skb, dev, type, daddr, saddr, len);
> -
> -	/* NOTE1: I'm still unsure about the fact that compression and WPAN
> -	 * header are created here and not later in the xmit. So wait for
> -	 * an opinion of net maintainers.
> -	 */
> -	/* NOTE2: to be absolutely correct, we must derive PANid information
> -	 * from MAC subif of the 'dev' and 'real_dev' network devices, but
> -	 * this isn't implemented in mainline yet, so currently we assign 0xff
> -	 */
> -	cb->type = IEEE802154_FC_TYPE_DATA;
> -
> -	/* prepare wpan address data */
> -	sa.mode = IEEE802154_ADDR_LONG;
> -	sa.pan_id = ieee802154_mlme_ops(dev)->get_pan_id(dev);
> -	sa.extended_addr = ieee802154_devaddr_from_raw(saddr);
> -
> -	/* intra-PAN communications */
> -	da.pan_id = sa.pan_id;
> -
> -	/* if the destination address is the broadcast address, use the
> -	 * corresponding short address
> -	 */
> -	if (lowpan_is_addr_broadcast(daddr)) {
> -		da.mode = IEEE802154_ADDR_SHORT;
> -		da.short_addr = cpu_to_le16(IEEE802154_ADDR_BROADCAST);
> -	} else {
> -		da.mode = IEEE802154_ADDR_LONG;
> -		da.extended_addr = ieee802154_devaddr_from_raw(daddr);
> -	}
> +	info = lowpan_skb_priv(skb);
>  
> -	cb->ackreq = !lowpan_is_addr_broadcast(daddr);
> +	/* TODO: Currently we only support extended_addr */
> +	info->daddr.mode = IEEE802154_ADDR_LONG;
> +	memcpy(&info->daddr.u.extended_addr, daddr,
> +	       sizeof(info->daddr.u.extended_addr));

here daddr is __be64 because it comes from IPv6 so __be64 on
lowpan_addr_info is correct.

> +	info->saddr.mode = IEEE802154_ADDR_LONG;
> +	memcpy(&info->saddr.u.extended_addr, saddr,
> +	       sizeof(info->daddr.u.extended_addr));
>  

same for saddr.

> -	return dev_hard_header(skb, lowpan_dev_info(dev)->real_dev,
> -			type, (void *)&da, (void *)&sa, 0);
> +	return 0;
>  }
>  
>  static int lowpan_give_skb_to_devices(struct sk_buff *skb,
> @@ -338,13 +336,68 @@ err:
>  	return rc;
>  }
>  
> +static int lowpan_header(struct sk_buff *skb, struct net_device *dev)
> +{
> +	struct ieee802154_addr sa, da;
> +	struct ieee802154_mac_cb *cb = mac_cb_init(skb);
> +	struct lowpan_addr_info info;
> +	void *daddr, *saddr;
> +
> +	memcpy(&info, lowpan_skb_priv(skb), sizeof(info));
> +
> +	/* TODO: Currently we only support extended_addr */
> +	daddr = &info.daddr.u.extended_addr;
> +	saddr = &info.saddr.u.extended_addr;
> +
> +	lowpan_header_compress(skb, dev, ETH_P_IPV6, daddr, saddr, skb->len);
> +
> +	cb->type = IEEE802154_FC_TYPE_DATA;
> +
> +	/* prepare wpan address data */
> +	sa.mode = IEEE802154_ADDR_LONG;
> +	sa.pan_id = ieee802154_mlme_ops(dev)->get_pan_id(dev);
> +	sa.extended_addr = ieee802154_devaddr_from_raw(saddr);

same like daddr, see below.

> +
> +	/* intra-PAN communications */
> +	da.pan_id = sa.pan_id;
> +
> +	/* if the destination address is the broadcast address, use the
> +	 * corresponding short address
> +	 */
> +	if (lowpan_is_addr_broadcast((const u8 *)daddr)) {
> +		da.mode = IEEE802154_ADDR_SHORT;
> +		da.short_addr = cpu_to_le16(IEEE802154_ADDR_BROADCAST);
> +		cb->ackreq = false;
> +	} else {
> +		da.mode = IEEE802154_ADDR_LONG;
> +		da.extended_addr = ieee802154_devaddr_from_raw(daddr);

here we need little_endian ieee802154_devaddr_from_raw makes some ugly
conversion with swab64 to convert it from __be64 to __le64.

> +		cb->ackreq = true;
> +	}
> +
> +	return dev_hard_header(skb, lowpan_dev_info(dev)->real_dev,
> +			ETH_P_IPV6, (void *)&da, (void *)&sa, 0);

generating mac header, require __le64 addresses.


so it's always necessary to use the right __leFOO and __beFOO types,
otherwise you get really fast confusing.

- Alex

Re: [PATCH linux-wpan v4] ieee802154: 6lowpan: ensure header compression does not corrupt ipv6 header

[Jukka Rissanen]

Hi Alex,

On ma, 2014-09-22 at 18:21 +0200, Alexander Aring wrote:

> grml, I overlooked something the last version. But I will fix it while
> >  
> > -	cb->ackreq = !lowpan_is_addr_broadcast(daddr);
> > +	/* TODO: Currently we only support extended_addr */
> > +	info->daddr.mode = IEEE802154_ADDR_LONG;
> 
> now IEEE802154_ADDR_LONG is no little endian value copied from mac
> header. Somewhere else converted the little endian value to an host understandable
> enum/define number. For the rework I will remove this value. We should never
> ever convert this value for parsing.
> 
> We use it only for af_802154 and userspace specification what addresses
> should be used. But the above one to make this __le16 is incorrect. For
> userspace we should create some enums for declaring different address
> types. Then we can use this for 802.15.4 sockets.

BTW, at some point we might want to change the IEEE802154_ADDR_LONG in
net/6lowpan/iphc.c to more generic. Now this IEEE 802154 specific enum
needs to be mentioned in net/bluetooth/6lowpan.c which looks quite
confusing.

> 
> I will fix it while applying when all things are okay, it's only to
> replace the __le16 to u8.
> 
> 
> And I also know that 6LoWPAN GENERIC need this value for determined is EUI64
> or short. I don't forget this one, wasn't full finished at rework
> branch.
> 
> - Alex


Cheers,
Jukka

Re: [PATCH linux-wpan v4] ieee802154: 6lowpan: ensure header compression does not corrupt ipv6 header

[Alexander Aring]

On Tue, Sep 23, 2014 at 12:13:54PM +0300, Jukka Rissanen wrote:
> Hi Alex,
> 
> On ma, 2014-09-22 at 18:21 +0200, Alexander Aring wrote:
> 
> > grml, I overlooked something the last version. But I will fix it while
> > >  
> > > -	cb->ackreq = !lowpan_is_addr_broadcast(daddr);
> > > +	/* TODO: Currently we only support extended_addr */
> > > +	info->daddr.mode = IEEE802154_ADDR_LONG;
> > 
> > now IEEE802154_ADDR_LONG is no little endian value copied from mac
> > header. Somewhere else converted the little endian value to an host understandable
> > enum/define number. For the rework I will remove this value. We should never
> > ever convert this value for parsing.
> > 
> > We use it only for af_802154 and userspace specification what addresses
> > should be used. But the above one to make this __le16 is incorrect. For
> > userspace we should create some enums for declaring different address
> > types. Then we can use this for 802.15.4 sockets.
> 
> BTW, at some point we might want to change the IEEE802154_ADDR_LONG in
> net/6lowpan/iphc.c to more generic. Now this IEEE 802154 specific enum
> needs to be mentioned in net/bluetooth/6lowpan.c which looks quite
> confusing.
> 

Yep, the generic name for such address is EUI-64. Forget the hyphen
there. There is alot of 802.15.4 specific 6lowpan defines in iphc.c.
They are defined in include/net/6lowpan.h, which should moved into an
internal header of "net/ieee802154/...".

And also we could do this maybe not as address type, more a address
length. EUI64 is 8 and the SHORT address is 2 bytes long. No special
handling about some address types, only handling about different lengt,
only handling about different lengths.

so we have some:

switch (address_length) {
case IEEE_EUI64_ADDRLEN:
case IEEE802154_SHORT_ADDRLEN:
default:
	/* not supported */
}

the second case is only for IEEE802154 handling, maybe we could also
check the netdev type, but that's not necessary. bluetooth should never
set IEEE802154_SHORT_ADDRLEN by calling lowpan_header_create.

Also we don't support the case IEEE802154_SHORT_ADDRLEN right now. :-)
That's currently dead code, it's a complicated issue to support that...
but in near future we will support it, otherwise our stack is in an
unusable state.

I mean IEEE_EUI64_ADDRLEN, is in 802154 the extended address. In
bluetooth you need to generate the EUI64 with filling ff:fe pattern.
Like behaviour in ethernet. And drop the special type handling.

Is this okay?

- Alex

Re: [PATCH linux-wpan v4] ieee802154: 6lowpan: ensure header compression does not corrupt ipv6 header

[Alexander Aring]

Jukka,

I/Simon mainly add you here in cc because this bug what Simon fixed here
is also inside of bluetooth 6lowpan.

I see now [0]. And it seems to me that this also occurs at bluetooth
branch. Do you have any questions on how the fix now is? Sorry, but I
need to clarify that you understood this issue and you/others prepare a
patch for this.

- Alex

[0] http://lxr.free-electrons.com/source/net/bluetooth/6lowpan.c#L509

Re: [PATCH linux-wpan v4] ieee802154: 6lowpan: ensure header compression does not corrupt ipv6 header

[Jukka Rissanen]

Hi Alex,

On ti, 2014-09-23 at 11:35 +0200, Alexander Aring wrote:
> Jukka,
> 
> I/Simon mainly add you here in cc because this bug what Simon fixed here
> is also inside of bluetooth 6lowpan.

BTW, the v4 patch does not apply cleanly to bluetooth-next. The
modifications were very small thou.

> 
> I see now [0]. And it seems to me that this also occurs at bluetooth
> branch. Do you have any questions on how the fix now is? Sorry, but I
> need to clarify that you understood this issue and you/others prepare a
> patch for this.

Yes, I am working on it. This v4 patch and the bluetooth fixes should go
in about the same time.

> 
> - Alex
> 
> [0] http://lxr.free-electrons.com/source/net/bluetooth/6lowpan.c#L509


Cheers,
Jukka

Re: [PATCH linux-wpan v4] ieee802154: 6lowpan: ensure header compression does not corrupt ipv6 header

[Jukka Rissanen]

Hi Alex,

On ti, 2014-09-23 at 11:26 +0200, Alexander Aring wrote:
> On Tue, Sep 23, 2014 at 12:13:54PM +0300, Jukka Rissanen wrote:
> > Hi Alex,
> > 
> > On ma, 2014-09-22 at 18:21 +0200, Alexander Aring wrote:
> > 
> > > grml, I overlooked something the last version. But I will fix it while
> > > >  
> > > > -	cb->ackreq = !lowpan_is_addr_broadcast(daddr);
> > > > +	/* TODO: Currently we only support extended_addr */
> > > > +	info->daddr.mode = IEEE802154_ADDR_LONG;
> > > 
> > > now IEEE802154_ADDR_LONG is no little endian value copied from mac
> > > header. Somewhere else converted the little endian value to an host understandable
> > > enum/define number. For the rework I will remove this value. We should never
> > > ever convert this value for parsing.
> > > 
> > > We use it only for af_802154 and userspace specification what addresses
> > > should be used. But the above one to make this __le16 is incorrect. For
> > > userspace we should create some enums for declaring different address
> > > types. Then we can use this for 802.15.4 sockets.
> > 
> > BTW, at some point we might want to change the IEEE802154_ADDR_LONG in
> > net/6lowpan/iphc.c to more generic. Now this IEEE 802154 specific enum
> > needs to be mentioned in net/bluetooth/6lowpan.c which looks quite
> > confusing.
> > 
> 
> Yep, the generic name for such address is EUI-64. Forget the hyphen
> there. There is alot of 802.15.4 specific 6lowpan defines in iphc.c.
> They are defined in include/net/6lowpan.h, which should moved into an
> internal header of "net/ieee802154/...".
> 
> And also we could do this maybe not as address type, more a address
> length. EUI64 is 8 and the SHORT address is 2 bytes long. No special
> handling about some address types, only handling about different lengt,
> only handling about different lengths.

Agreed, the address length would be better.

> 
> so we have some:
> 
> switch (address_length) {
> case IEEE_EUI64_ADDRLEN:
> case IEEE802154_SHORT_ADDRLEN:
> default:
> 	/* not supported */
> }
> 
> the second case is only for IEEE802154 handling, maybe we could also
> check the netdev type, but that's not necessary. bluetooth should never
> set IEEE802154_SHORT_ADDRLEN by calling lowpan_header_create.
> 
> Also we don't support the case IEEE802154_SHORT_ADDRLEN right now. :-)
> That's currently dead code, it's a complicated issue to support that...
> but in near future we will support it, otherwise our stack is in an
> unusable state.
> 
> I mean IEEE_EUI64_ADDRLEN, is in 802154 the extended address. In
> bluetooth you need to generate the EUI64 with filling ff:fe pattern.
> Like behaviour in ethernet. And drop the special type handling.
> 
> Is this okay?

Yes, that sounds like a good plan.


Cheers,
Jukka

Re: [PATCH linux-wpan v4] ieee802154: 6lowpan: ensure header compression does not corrupt ipv6 header

[Alexander Aring]

On Tue, Sep 23, 2014 at 01:14:06PM +0300, Jukka Rissanen wrote:
> Hi Alex,
> 
> On ti, 2014-09-23 at 11:35 +0200, Alexander Aring wrote:
> > Jukka,
> > 
> > I/Simon mainly add you here in cc because this bug what Simon fixed here
> > is also inside of bluetooth 6lowpan.
> 
> BTW, the v4 patch does not apply cleanly to bluetooth-next. The
> modifications were very small thou.
> 

It's a bug fix and should be go to wpan, then wpan goes to bluetooth. I
know there is some easy solveable merge conflict... but maintainers need to be
deal with it then. ;-)

When this is an easy conflict it doesn't matter to wait on it. I hope
that's okay for all maintainers, I am new in this handling. Maintainers
isn't just "apply patches", making the right workflow and solve merge
conflicts, I think. Otherwise some maintainer will tell me "don't making merge
conflicts!", but I heard nothing something like that now.

Or we wait when these changes are in bluetooth repo, which means also
wpan repo.


Also there is an option to send it to -stable, but for me that doesn't
matter currently, there are still other unsolved issues in 802.15.4
branch. When we know some "real" users and some of them wants support
for v3.16 or something else then we could try to make a backport for
this. At the moment we don't have a "real" -stable strategy.

And stable != bluetooth/wpan, okay the stable maintainers sometimes look
into bluetooth/wpan/net/wireless (the without -next) to grab some stable
patches from it. But to make a real stable patch you need to read:

Documentation/stable_kernel_rules.txt

- Alex

Re: [PATCH linux-wpan v4] ieee802154: 6lowpan: ensure header compression does not corrupt ipv6 header

[Jukka Rissanen]

On ti, 2014-09-23 at 12:33 +0200, Alexander Aring wrote:
> On Tue, Sep 23, 2014 at 01:14:06PM +0300, Jukka Rissanen wrote:
> > Hi Alex,
> > 
> > On ti, 2014-09-23 at 11:35 +0200, Alexander Aring wrote:
> > > Jukka,
> > > 
> > > I/Simon mainly add you here in cc because this bug what Simon fixed here
> > > is also inside of bluetooth 6lowpan.
> > 
> > BTW, the v4 patch does not apply cleanly to bluetooth-next. The
> > modifications were very small thou.
> > 
> 
> It's a bug fix and should be go to wpan, then wpan goes to bluetooth. I
> know there is some easy solveable merge conflict... but maintainers need to be
> deal with it then. ;-)
> 
> When this is an easy conflict it doesn't matter to wait on it. I hope
> that's okay for all maintainers, I am new in this handling. Maintainers
> isn't just "apply patches", making the right workflow and solve merge
> conflicts, I think. Otherwise some maintainer will tell me "don't making merge
> conflicts!", but I heard nothing something like that now.
> 
> Or we wait when these changes are in bluetooth repo, which means also
> wpan repo.

Actually, the v4 patch only touches net/ieee802154/6lowpan_rtnl.c which
is not used by bluetooth so no worries here. 


Cheers,
Jukka

Re: [PATCH linux-wpan v4] ieee802154: 6lowpan: ensure header compression does not corrupt ipv6 header

[Jukka Rissanen]

Hi Alex,

On ti, 2014-09-23 at 11:35 +0200, Alexander Aring wrote:
> Jukka,
> 
> I/Simon mainly add you here in cc because this bug what Simon fixed here
> is also inside of bluetooth 6lowpan.
> 
> I see now [0]. And it seems to me that this also occurs at bluetooth
> branch. Do you have any questions on how the fix now is? Sorry, but I
> need to clarify that you understood this issue and you/others prepare a
> patch for this.

I was trying to test this issue and send multicast packets to Bluetooth
6lowpan until I realized that the interface does not have IFF_MULTICAST
enabled :) So this issue is not valid atm for Bluetooth.

Thanks for pointing this out.


Cheers,
Jukka

Re: [PATCH linux-wpan v4] ieee802154: 6lowpan: ensure header compression does not corrupt ipv6 header

[Alexander Aring]

Hi Jukka,

On Tue, Sep 23, 2014 at 05:20:42PM +0300, Jukka Rissanen wrote:
> Hi Alex,
> 
> On ti, 2014-09-23 at 11:35 +0200, Alexander Aring wrote:
> > Jukka,
> > 
> > I/Simon mainly add you here in cc because this bug what Simon fixed here
> > is also inside of bluetooth 6lowpan.
> > 
> > I see now [0]. And it seems to me that this also occurs at bluetooth
> > branch. Do you have any questions on how the fix now is? Sorry, but I
> > need to clarify that you understood this issue and you/others prepare a
> > patch for this.
> 
> I was trying to test this issue and send multicast packets to Bluetooth
> 6lowpan until I realized that the interface does not have IFF_MULTICAST
> enabled :) So this issue is not valid atm for Bluetooth.
> 

Sorry that confusing me now completely. I don't know how 6LoWPAN
Bluetooth works. But sending MULTICAST packets is part of IPv6 so
6LoWPAN need the possibility to send MULTICAST packets at L3.

On L2 (that's 802.15.4) we don't support MULTICAST but we sending
BROADCAST frames out. So we have a LOGICAL MULTICAST over BROADCAST.

I don't know how bluetooth works, if they have some MULTICAST support,
we don't have it. Ethernet has MULTICAST support (as L2 example).

We already talked about RFC6775 that's mainly an optimzation for
neighbor discovery. The ethernet neighbor discovery use's many MULTICAST
frames, because ethernet have PHYSICAL MULTICAST support. RFC6775 have
different strategy to use LOGICAL MULTICAST which have a PHYSICAL
BROADCAST to have less traffic afterwards. That's one of the big
benefits to use RFC6775.

Sorry, that 6LoWPAN Bluetooth doesn't support MULTICAST sounds for me
wrong. I mean the underlaying device supports BROADCAST, but then you
have a LOGICAL MULTICAST over a BROADCAST (that's how 802.15.4 deal with
that). How can neighbor discovery deal with that? I don't think that the
kernel implementation checks on IFF_MULTICAST flag. And now you can't
send no MULTICAST 6LoWPAN packets on an IPv6 socket? Don't know but
6LoWPAN is an adaptation layer, it should be support all things what's
also supported by IPv6.



To the fix:
But this issue "could" also occur on non multicast packets, it's better
to be sure that the buffer isn't shared when replacing IPv6 with 6LoWPAN
header.

- Alex

Re: [PATCH linux-wpan v4] ieee802154: 6lowpan: ensure header compression does not corrupt ipv6 header

[Alexander Aring]

On Tue, Sep 23, 2014 at 04:43:15PM +0200, Alexander Aring wrote:
> Hi Jukka,
> 
> On Tue, Sep 23, 2014 at 05:20:42PM +0300, Jukka Rissanen wrote:
> > Hi Alex,
> > 
> > On ti, 2014-09-23 at 11:35 +0200, Alexander Aring wrote:
> > > Jukka,
> > > 
> > > I/Simon mainly add you here in cc because this bug what Simon fixed here
> > > is also inside of bluetooth 6lowpan.
> > > 
> > > I see now [0]. And it seems to me that this also occurs at bluetooth
> > > branch. Do you have any questions on how the fix now is? Sorry, but I
> > > need to clarify that you understood this issue and you/others prepare a
> > > patch for this.
> > 
> > I was trying to test this issue and send multicast packets to Bluetooth
> > 6lowpan until I realized that the interface does not have IFF_MULTICAST
> > enabled :) So this issue is not valid atm for Bluetooth.
> > 
> 
> Sorry that confusing me now completely. I don't know how 6LoWPAN
> Bluetooth works. But sending MULTICAST packets is part of IPv6 so
> 6LoWPAN need the possibility to send MULTICAST packets at L3.
> 
> On L2 (that's 802.15.4) we don't support MULTICAST but we sending
> BROADCAST frames out. So we have a LOGICAL MULTICAST over BROADCAST.
> 

See [0]. That's also what the neighbor discovery do when no special
multicast address map generation is implemented.

So 802.15.4 and bluetooth running at the moment into the default branch
which is always the BROADCAST address -> then this is LOGICAL MULTICAST
over BROADCAST.


Sorry for the upper case words. :-)

- Alex

[0] http://lxr.free-electrons.com/source/net/ipv6/ndisc.c#L263

Re: [PATCH linux-wpan v4] ieee802154: 6lowpan: ensure header compression does not corrupt ipv6 header

[Marcel Holtmann]

Hi Alex,

>>> I/Simon mainly add you here in cc because this bug what Simon fixed here
>>> is also inside of bluetooth 6lowpan.
>> 
>> BTW, the v4 patch does not apply cleanly to bluetooth-next. The
>> modifications were very small thou.
>> 
> 
> It's a bug fix and should be go to wpan, then wpan goes to bluetooth. I
> know there is some easy solveable merge conflict... but maintainers need to be
> deal with it then. ;-)
> 
> When this is an easy conflict it doesn't matter to wait on it. I hope
> that's okay for all maintainers, I am new in this handling. Maintainers
> isn't just "apply patches", making the right workflow and solve merge
> conflicts, I think. Otherwise some maintainer will tell me "don't making merge
> conflicts!", but I heard nothing something like that now.
> 
> Or we wait when these changes are in bluetooth repo, which means also
> wpan repo.

since 3.17 is basically around the corner, adding it to bluetooth tree is not getting us anywhere right now. So what I would do is include it in bluetooth-next and add a CC: stable tag to it.

Regards

Marcel

Re: [PATCH linux-wpan v4] ieee802154: 6lowpan: ensure header compression does not corrupt ipv6 header

[Alexander Aring]

Hi Marcel,

On Tue, Sep 23, 2014 at 05:23:19PM +0200, Marcel Holtmann wrote:
> Hi Alex,
> 
> >>> I/Simon mainly add you here in cc because this bug what Simon fixed here
> >>> is also inside of bluetooth 6lowpan.
> >> 
> >> BTW, the v4 patch does not apply cleanly to bluetooth-next. The
> >> modifications were very small thou.
> >> 
> > 
> > It's a bug fix and should be go to wpan, then wpan goes to bluetooth. I
> > know there is some easy solveable merge conflict... but maintainers need to be
> > deal with it then. ;-)
> > 
> > When this is an easy conflict it doesn't matter to wait on it. I hope
> > that's okay for all maintainers, I am new in this handling. Maintainers
> > isn't just "apply patches", making the right workflow and solve merge
> > conflicts, I think. Otherwise some maintainer will tell me "don't making merge
> > conflicts!", but I heard nothing something like that now.
> > 
> > Or we wait when these changes are in bluetooth repo, which means also
> > wpan repo.
> 
> since 3.17 is basically around the corner, adding it to bluetooth tree is not getting us anywhere right now. So what I would do is include it in bluetooth-next and add a CC: stable tag to it.
> 

you are right. I will try that to CC: stable on this patch. Please don't
be angry that I will fail there. The last time when I tried it I got a
formletter back. :-)

- Alex

Re: [PATCH linux-wpan v4] ieee802154: 6lowpan: ensure header compression does not corrupt ipv6 header

[Marcel Holtmann]

Hi Alex,

>>>>> I/Simon mainly add you here in cc because this bug what Simon fixed here
>>>>> is also inside of bluetooth 6lowpan.
>>>> 
>>>> BTW, the v4 patch does not apply cleanly to bluetooth-next. The
>>>> modifications were very small thou.
>>>> 
>>> 
>>> It's a bug fix and should be go to wpan, then wpan goes to bluetooth. I
>>> know there is some easy solveable merge conflict... but maintainers need to be
>>> deal with it then. ;-)
>>> 
>>> When this is an easy conflict it doesn't matter to wait on it. I hope
>>> that's okay for all maintainers, I am new in this handling. Maintainers
>>> isn't just "apply patches", making the right workflow and solve merge
>>> conflicts, I think. Otherwise some maintainer will tell me "don't making merge
>>> conflicts!", but I heard nothing something like that now.
>>> 
>>> Or we wait when these changes are in bluetooth repo, which means also
>>> wpan repo.
>> 
>> since 3.17 is basically around the corner, adding it to bluetooth tree is not getting us anywhere right now. So what I would do is include it in bluetooth-next and add a CC: stable tag to it.
>> 
> 
> you are right. I will try that to CC: stable on this patch. Please don't
> be angry that I will fail there. The last time when I tried it I got a
> formletter back. :-)

check if the patch would actually apply against 3.16.x and if it does just included CC: stable@vger.kernel.org under the signed-off-by line. If it doesn't, then we need to get the fix included first. And after the merge window we reference it by a version that applies to 3.16.x and send it to stable tree manually. However in all cases patch needs to hit Linus' tree first.

Regards

Marcel

Re: [PATCH linux-wpan v4] ieee802154: 6lowpan: ensure header compression does not corrupt ipv6 header

[Jukka Rissanen]

Hi Alex,

On ti, 2014-09-23 at 16:43 +0200, Alexander Aring wrote:
> Hi Jukka,
> 
> On Tue, Sep 23, 2014 at 05:20:42PM +0300, Jukka Rissanen wrote:
> > Hi Alex,
> > 
> > On ti, 2014-09-23 at 11:35 +0200, Alexander Aring wrote:
> > > Jukka,
> > > 
> > > I/Simon mainly add you here in cc because this bug what Simon fixed here
> > > is also inside of bluetooth 6lowpan.
> > > 
> > > I see now [0]. And it seems to me that this also occurs at bluetooth
> > > branch. Do you have any questions on how the fix now is? Sorry, but I
> > > need to clarify that you understood this issue and you/others prepare a
> > > patch for this.
> > 
> > I was trying to test this issue and send multicast packets to Bluetooth
> > 6lowpan until I realized that the interface does not have IFF_MULTICAST
> > enabled :) So this issue is not valid atm for Bluetooth.
> > 
> 
> Sorry that confusing me now completely. I don't know how 6LoWPAN
> Bluetooth works. But sending MULTICAST packets is part of IPv6 so
> 6LoWPAN need the possibility to send MULTICAST packets at L3.

Yes, I know. I was just saying that IFF_MULTICAST is not set for the
interface in current Bluetooth 6lowpan implementation. This will be
fixed for sure.


Cheers,
Jukka

Re: [PATCH linux-wpan v4] ieee802154: 6lowpan: ensure header compression does not corrupt ipv6 header

[Alexander Aring]

On Wed, Sep 24, 2014 at 10:10:01AM +0300, Jukka Rissanen wrote:
> Hi Alex,
> 
> On ti, 2014-09-23 at 16:43 +0200, Alexander Aring wrote:
> > Hi Jukka,
> > 
> > On Tue, Sep 23, 2014 at 05:20:42PM +0300, Jukka Rissanen wrote:
> > > Hi Alex,
> > > 
> > > On ti, 2014-09-23 at 11:35 +0200, Alexander Aring wrote:
> > > > Jukka,
> > > > 
> > > > I/Simon mainly add you here in cc because this bug what Simon fixed here
> > > > is also inside of bluetooth 6lowpan.
> > > > 
> > > > I see now [0]. And it seems to me that this also occurs at bluetooth
> > > > branch. Do you have any questions on how the fix now is? Sorry, but I
> > > > need to clarify that you understood this issue and you/others prepare a
> > > > patch for this.
> > > 
> > > I was trying to test this issue and send multicast packets to Bluetooth
> > > 6lowpan until I realized that the interface does not have IFF_MULTICAST
> > > enabled :) So this issue is not valid atm for Bluetooth.
> > > 
> > 
> > Sorry that confusing me now completely. I don't know how 6LoWPAN
> > Bluetooth works. But sending MULTICAST packets is part of IPv6 so
> > 6LoWPAN need the possibility to send MULTICAST packets at L3.
> 
> Yes, I know. I was just saying that IFF_MULTICAST is not set for the
> interface in current Bluetooth 6lowpan implementation. This will be
> fixed for sure.
> 

Okay. :-)

- Alex

Re: [PATCH linux-wpan v4] ieee802154: 6lowpan: ensure header compression does not corrupt ipv6 header

[Alexander Aring]

Hi Marcel,

thank you for your help.

On Tue, Sep 23, 2014 at 06:19:59PM +0200, Marcel Holtmann wrote:
> Hi Alex,
> 
> >>>>> I/Simon mainly add you here in cc because this bug what Simon fixed here
> >>>>> is also inside of bluetooth 6lowpan.
> >>>> 
> >>>> BTW, the v4 patch does not apply cleanly to bluetooth-next. The
> >>>> modifications were very small thou.
> >>>> 
> >>> 
> >>> It's a bug fix and should be go to wpan, then wpan goes to bluetooth. I
> >>> know there is some easy solveable merge conflict... but maintainers need to be
> >>> deal with it then. ;-)
> >>> 
> >>> When this is an easy conflict it doesn't matter to wait on it. I hope
> >>> that's okay for all maintainers, I am new in this handling. Maintainers
> >>> isn't just "apply patches", making the right workflow and solve merge
> >>> conflicts, I think. Otherwise some maintainer will tell me "don't making merge
> >>> conflicts!", but I heard nothing something like that now.
> >>> 
> >>> Or we wait when these changes are in bluetooth repo, which means also
> >>> wpan repo.
> >> 
> >> since 3.17 is basically around the corner, adding it to bluetooth tree is not getting us anywhere right now. So what I would do is include it in bluetooth-next and add a CC: stable tag to it.
> >> 
> > 
> > you are right. I will try that to CC: stable on this patch. Please don't
> > be angry that I will fail there. The last time when I tried it I got a
> > formletter back. :-)
> 
> check if the patch would actually apply against 3.16.x and if it does just included CC: stable@vger.kernel.org under the signed-off-by line. If it doesn't, then we need to get the fix included first. And after the merge window we reference it by a version that applies to 3.16.x and send it to stable tree manually. However in all cases patch needs to hit Linus' tree first.
> 

doesn't apply against "3.16.x". I will apply it to my wpan-next testing
branch and in the next days I will send it to your bluetooth-next with a
couple of other patches.

My understanding is that we are close of releasing 3.17 it makes no
sense to send it to bluetooth now. When the patch hit's "linus" tree we
can make a different patch for 3.16.x and send it to -stable.

- Alex

Re: [PATCH linux-wpan v4] ieee802154: 6lowpan: ensure header compression does not corrupt ipv6 header

[Alexander Aring]

Hi Simon,

On Mon, Sep 22, 2014 at 04:52:03PM +0100, Simon Vincent wrote:
> The 6lowpan ipv6 header compression was causing problems for other interfaces
> that expected a ipv6 header to still be in place, as we were replacing the
> ipv6 header with a compressed version. This happened if you sent a packet to a
> multicast address as the packet would be output on 802.15.4, ethernet, and also
> be sent to the loopback interface. The skb data was shared between these
> interfaces so all interfaces ended up with a compressed ipv6 header.
> The solution is to ensure that before we do any header compression we are not
> sharing the skb or skb data with any other interface. If we are then we must
> take a copy of the skb and skb data before modifying the ipv6 header.
> The only place we can copy the skb is inside the xmit function so we don't
> leave dangling references to skb.
> This patch moves all the header compression to inside the xmit function. Very
> little code has been changed it has mostly been moved from lowpan_header_create
> to lowpan_xmit. At the top of the xmit function we now check if the skb is
> shared and if so copy it. In lowpan_header_create all we do now is store the
> source and destination addresses for use later when we compress the header.
> 
> Signed-off-by: Simon Vincent <simon.vincent@xsilon.com>

patch has been applied to linux-wpan-next/testing.

Minor changes "__le16 mode" to "u8 mode".

Thanks for your effort to pointing this issue out.

- Alex

Re: [PATCH linux-wpan v4] ieee802154: 6lowpan: ensure header compression does not corrupt ipv6 header

[Marcel Holtmann]

Hi Alex,

>>>>>>> I/Simon mainly add you here in cc because this bug what Simon fixed here
>>>>>>> is also inside of bluetooth 6lowpan.
>>>>>> 
>>>>>> BTW, the v4 patch does not apply cleanly to bluetooth-next. The
>>>>>> modifications were very small thou.
>>>>>> 
>>>>> 
>>>>> It's a bug fix and should be go to wpan, then wpan goes to bluetooth. I
>>>>> know there is some easy solveable merge conflict... but maintainers need to be
>>>>> deal with it then. ;-)
>>>>> 
>>>>> When this is an easy conflict it doesn't matter to wait on it. I hope
>>>>> that's okay for all maintainers, I am new in this handling. Maintainers
>>>>> isn't just "apply patches", making the right workflow and solve merge
>>>>> conflicts, I think. Otherwise some maintainer will tell me "don't making merge
>>>>> conflicts!", but I heard nothing something like that now.
>>>>> 
>>>>> Or we wait when these changes are in bluetooth repo, which means also
>>>>> wpan repo.
>>>> 
>>>> since 3.17 is basically around the corner, adding it to bluetooth tree is not getting us anywhere right now. So what I would do is include it in bluetooth-next and add a CC: stable tag to it.
>>>> 
>>> 
>>> you are right. I will try that to CC: stable on this patch. Please don't
>>> be angry that I will fail there. The last time when I tried it I got a
>>> formletter back. :-)
>> 
>> check if the patch would actually apply against 3.16.x and if it does just included CC: stable@vger.kernel.org under the signed-off-by line. If it doesn't, then we need to get the fix included first. And after the merge window we reference it by a version that applies to 3.16.x and send it to stable tree manually. However in all cases patch needs to hit Linus' tree first.
>> 
> 
> doesn't apply against "3.16.x". I will apply it to my wpan-next testing
> branch and in the next days I will send it to your bluetooth-next with a
> couple of other patches.

you might want to hurry up here since if Linus releases 3.17 on Sunday, then this means the merge window has opened. So you are cutting it extremely close. I say unless we get the patches by end of today, it becomes really hard to justify pushing these into 3.18 merge window.

> My understanding is that we are close of releasing 3.17 it makes no
> sense to send it to bluetooth now. When the patch hit's "linus" tree we
> can make a different patch for 3.16.x and send it to -stable.

Yes. If we fixed it in Linus' tree and decide back porting it to older kernels is worth while doing, then yes, we can send a modified patch that applies to 3.17 first of all. I say 3.17 since that will be the first stable kernel it should apply to and then see what needs to be done for 3.16 and if it makes sense.

For example some Bluetooth patches we have not send for stable since the effort is too much. We send them to the stable kernel they still apply to and once they stop applying, we ignored them until real bugs are have been filed. Remember that not every bug is something that needs fixing in stable kernel.

Regards

Marcel

Re: [PATCH linux-wpan v4] ieee802154: 6lowpan: ensure header compression does not corrupt ipv6 header

[Alexander Aring]

Hi Marcel,

On Wed, Sep 24, 2014 at 11:55:40AM +0200, Marcel Holtmann wrote:
> Hi Alex,
> 
> >>>>>>> I/Simon mainly add you here in cc because this bug what Simon fixed here
> >>>>>>> is also inside of bluetooth 6lowpan.
> >>>>>> 
> >>>>>> BTW, the v4 patch does not apply cleanly to bluetooth-next. The
> >>>>>> modifications were very small thou.
> >>>>>> 
> >>>>> 
> >>>>> It's a bug fix and should be go to wpan, then wpan goes to bluetooth. I
> >>>>> know there is some easy solveable merge conflict... but maintainers need to be
> >>>>> deal with it then. ;-)
> >>>>> 
> >>>>> When this is an easy conflict it doesn't matter to wait on it. I hope
> >>>>> that's okay for all maintainers, I am new in this handling. Maintainers
> >>>>> isn't just "apply patches", making the right workflow and solve merge
> >>>>> conflicts, I think. Otherwise some maintainer will tell me "don't making merge
> >>>>> conflicts!", but I heard nothing something like that now.
> >>>>> 
> >>>>> Or we wait when these changes are in bluetooth repo, which means also
> >>>>> wpan repo.
> >>>> 
> >>>> since 3.17 is basically around the corner, adding it to bluetooth tree is not getting us anywhere right now. So what I would do is include it in bluetooth-next and add a CC: stable tag to it.
> >>>> 
> >>> 
> >>> you are right. I will try that to CC: stable on this patch. Please don't
> >>> be angry that I will fail there. The last time when I tried it I got a
> >>> formletter back. :-)
> >> 
> >> check if the patch would actually apply against 3.16.x and if it does just included CC: stable@vger.kernel.org under the signed-off-by line. If it doesn't, then we need to get the fix included first. And after the merge window we reference it by a version that applies to 3.16.x and send it to stable tree manually. However in all cases patch needs to hit Linus' tree first.
> >> 
> > 
> > doesn't apply against "3.16.x". I will apply it to my wpan-next testing
> > branch and in the next days I will send it to your bluetooth-next with a
> > couple of other patches.
> 
> you might want to hurry up here since if Linus releases 3.17 on Sunday, then this means the merge window has opened. So you are cutting it extremely close. I say unless we get the patches by end of today, it becomes really hard to justify pushing these into 3.18 merge window.
> 

ok, ok. I will send you what I have now. I know the "3 hops problematic"
patch traceroute looks like: bluetooth -> wireless -> net -> linus

I/We have a huge latency here.

> > My understanding is that we are close of releasing 3.17 it makes no
> > sense to send it to bluetooth now. When the patch hit's "linus" tree we
> > can make a different patch for 3.16.x and send it to -stable.
> 
> Yes. If we fixed it in Linus' tree and decide back porting it to older kernels is worth while doing, then yes, we can send a modified patch that applies to 3.17 first of all. I say 3.17 since that will be the first stable kernel it should apply to and then see what needs to be done for 3.16 and if it makes sense.
> 
> For example some Bluetooth patches we have not send for stable since the effort is too much. We send them to the stable kernel they still apply to and once they stop applying, we ignored them until real bugs are have been filed. Remember that not every bug is something that needs fixing in stable kernel.
> 

ehm, yes effort is sometimes also too much here...

I mean, if somebody wants this in stable version x.y.z I am happy that
somebody do the effort and porting this back. But I don't want to
backport all fixes into -stable and for each stable branch. Sometimes
that's also not possible.

- Alex

Re: [PATCH linux-wpan v4] ieee802154: 6lowpan: ensure header compression does not corrupt ipv6 header

[Marcel Holtmann]

Hi Alex,

>>>>>>>>> I/Simon mainly add you here in cc because this bug what Simon fixed here
>>>>>>>>> is also inside of bluetooth 6lowpan.
>>>>>>>> 
>>>>>>>> BTW, the v4 patch does not apply cleanly to bluetooth-next. The
>>>>>>>> modifications were very small thou.
>>>>>>>> 
>>>>>>> 
>>>>>>> It's a bug fix and should be go to wpan, then wpan goes to bluetooth. I
>>>>>>> know there is some easy solveable merge conflict... but maintainers need to be
>>>>>>> deal with it then. ;-)
>>>>>>> 
>>>>>>> When this is an easy conflict it doesn't matter to wait on it. I hope
>>>>>>> that's okay for all maintainers, I am new in this handling. Maintainers
>>>>>>> isn't just "apply patches", making the right workflow and solve merge
>>>>>>> conflicts, I think. Otherwise some maintainer will tell me "don't making merge
>>>>>>> conflicts!", but I heard nothing something like that now.
>>>>>>> 
>>>>>>> Or we wait when these changes are in bluetooth repo, which means also
>>>>>>> wpan repo.
>>>>>> 
>>>>>> since 3.17 is basically around the corner, adding it to bluetooth tree is not getting us anywhere right now. So what I would do is include it in bluetooth-next and add a CC: stable tag to it.
>>>>>> 
>>>>> 
>>>>> you are right. I will try that to CC: stable on this patch. Please don't
>>>>> be angry that I will fail there. The last time when I tried it I got a
>>>>> formletter back. :-)
>>>> 
>>>> check if the patch would actually apply against 3.16.x and if it does just included CC: stable@vger.kernel.org under the signed-off-by line. If it doesn't, then we need to get the fix included first. And after the merge window we reference it by a version that applies to 3.16.x and send it to stable tree manually. However in all cases patch needs to hit Linus' tree first.
>>>> 
>>> 
>>> doesn't apply against "3.16.x". I will apply it to my wpan-next testing
>>> branch and in the next days I will send it to your bluetooth-next with a
>>> couple of other patches.
>> 
>> you might want to hurry up here since if Linus releases 3.17 on Sunday, then this means the merge window has opened. So you are cutting it extremely close. I say unless we get the patches by end of today, it becomes really hard to justify pushing these into 3.18 merge window.
>> 
> 
> ok, ok. I will send you what I have now. I know the "3 hops problematic"
> patch traceroute looks like: bluetooth -> wireless -> net -> linus
> 
> I/We have a huge latency here.

if the merge window is around the corner, you will be in a hurry no matter what. So please do not blame the different trees a patch has to go through. This is what it is. The earlier patches are ready the less this will become a problem. We have been pushing bluetooth-next into wireless-next on a really regular basis and John has been quick in pulling stuff. So there is really not bottleneck here.

>>> My understanding is that we are close of releasing 3.17 it makes no
>>> sense to send it to bluetooth now. When the patch hit's "linus" tree we
>>> can make a different patch for 3.16.x and send it to -stable.
>> 
>> Yes. If we fixed it in Linus' tree and decide back porting it to older kernels is worth while doing, then yes, we can send a modified patch that applies to 3.17 first of all. I say 3.17 since that will be the first stable kernel it should apply to and then see what needs to be done for 3.16 and if it makes sense.
>> 
>> For example some Bluetooth patches we have not send for stable since the effort is too much. We send them to the stable kernel they still apply to and once they stop applying, we ignored them until real bugs are have been filed. Remember that not every bug is something that needs fixing in stable kernel.
>> 
> 
> ehm, yes effort is sometimes also too much here...
> 
> I mean, if somebody wants this in stable version x.y.z I am happy that
> somebody do the effort and porting this back. But I don't want to
> backport all fixes into -stable and for each stable branch. Sometimes
> that's also not possible.

My advise is just don't. Unless it is trivial and an obvious fix. Hurrying things into stable is wrong in my opinion. I only tag for stable when it is a real issue that causes long term issues. That is why I hate if individual patch submitters tag things for stable. Your own patch is always the most important one, but in the big picture it might not be. So I would not worry too much about stable. Getting patches quickly into next trees is more important in my opinion.

Regards

Marcel

Re: [PATCH linux-wpan v4] ieee802154: 6lowpan: ensure header compression does not corrupt ipv6 header

[Alexander Aring]

Hi Marcel,

On Wed, Sep 24, 2014 at 02:24:44PM +0200, Marcel Holtmann wrote:
> Hi Alex,
> 
> >>>>>>>>> I/Simon mainly add you here in cc because this bug what Simon fixed here
> >>>>>>>>> is also inside of bluetooth 6lowpan.
> >>>>>>>> 
> >>>>>>>> BTW, the v4 patch does not apply cleanly to bluetooth-next. The
> >>>>>>>> modifications were very small thou.
> >>>>>>>> 
> >>>>>>> 
> >>>>>>> It's a bug fix and should be go to wpan, then wpan goes to bluetooth. I
> >>>>>>> know there is some easy solveable merge conflict... but maintainers need to be
> >>>>>>> deal with it then. ;-)
> >>>>>>> 
> >>>>>>> When this is an easy conflict it doesn't matter to wait on it. I hope
> >>>>>>> that's okay for all maintainers, I am new in this handling. Maintainers
> >>>>>>> isn't just "apply patches", making the right workflow and solve merge
> >>>>>>> conflicts, I think. Otherwise some maintainer will tell me "don't making merge
> >>>>>>> conflicts!", but I heard nothing something like that now.
> >>>>>>> 
> >>>>>>> Or we wait when these changes are in bluetooth repo, which means also
> >>>>>>> wpan repo.
> >>>>>> 
> >>>>>> since 3.17 is basically around the corner, adding it to bluetooth tree is not getting us anywhere right now. So what I would do is include it in bluetooth-next and add a CC: stable tag to it.
> >>>>>> 
> >>>>> 
> >>>>> you are right. I will try that to CC: stable on this patch. Please don't
> >>>>> be angry that I will fail there. The last time when I tried it I got a
> >>>>> formletter back. :-)
> >>>> 
> >>>> check if the patch would actually apply against 3.16.x and if it does just included CC: stable@vger.kernel.org under the signed-off-by line. If it doesn't, then we need to get the fix included first. And after the merge window we reference it by a version that applies to 3.16.x and send it to stable tree manually. However in all cases patch needs to hit Linus' tree first.
> >>>> 
> >>> 
> >>> doesn't apply against "3.16.x". I will apply it to my wpan-next testing
> >>> branch and in the next days I will send it to your bluetooth-next with a
> >>> couple of other patches.
> >> 
> >> you might want to hurry up here since if Linus releases 3.17 on Sunday, then this means the merge window has opened. So you are cutting it extremely close. I say unless we get the patches by end of today, it becomes really hard to justify pushing these into 3.18 merge window.
> >> 
> > 
> > ok, ok. I will send you what I have now. I know the "3 hops problematic"
> > patch traceroute looks like: bluetooth -> wireless -> net -> linus
> > 
> > I/We have a huge latency here.
> 
> if the merge window is around the corner, you will be in a hurry no matter what. So please do not blame the different trees a patch has to go through. This is what it is. The earlier patches are ready the less this will become a problem. We have been pushing bluetooth-next into wireless-next on a really regular basis and John has been quick in pulling stuff. So there is really not bottleneck here.

Okay, when this is no bottleneck. Sorry that I blame the different trees
in this case. Was just a mind that this could be a bottleneck. Sorry.

Next time I will send patches faster, but these 4 patches came up all
this week at linux-wpan, so there was no much patches in this period.

Other solution would be simple send all patches also 802.15.4 subsystem
directly to bluetooth. But now we have a better reviewing step before
we send it to your bluetooth tree's.

> 
> >>> My understanding is that we are close of releasing 3.17 it makes no
> >>> sense to send it to bluetooth now. When the patch hit's "linus" tree we
> >>> can make a different patch for 3.16.x and send it to -stable.
> >> 
> >> Yes. If we fixed it in Linus' tree and decide back porting it to older kernels is worth while doing, then yes, we can send a modified patch that applies to 3.17 first of all. I say 3.17 since that will be the first stable kernel it should apply to and then see what needs to be done for 3.16 and if it makes sense.
> >> 
> >> For example some Bluetooth patches we have not send for stable since the effort is too much. We send them to the stable kernel they still apply to and once they stop applying, we ignored them until real bugs are have been filed. Remember that not every bug is something that needs fixing in stable kernel.
> >> 
> > 
> > ehm, yes effort is sometimes also too much here...
> > 
> > I mean, if somebody wants this in stable version x.y.z I am happy that
> > somebody do the effort and porting this back. But I don't want to
> > backport all fixes into -stable and for each stable branch. Sometimes
> > that's also not possible.
> 
> My advise is just don't. Unless it is trivial and an obvious fix. Hurrying things into stable is wrong in my opinion. I only tag for stable when it is a real issue that causes long term issues. That is why I hate if individual patch submitters tag things for stable. Your own patch is always the most important one, but in the big picture it might not be. So I would not worry too much about stable. Getting patches quickly into next trees is more important in my opinion.
> 

Okay, I already told people that they should not use any stable kernel
for the 802.15.4 subsystem. Then I will not care about -stable branch,
it's okay for me. Otherwise I will getting crazy to care that everything
will work in -stable tree's.

- Alex