[PATCH 1/3] mac802154: don't warn on unsupported frames

[PATCH 1/3] mac802154: don't warn on unsupported frames

[Aristeu Rozanski]

Just because we don't support certain types of frames yet doesn't mean
we have to flood the message log with warnings about "invalid" frames.

Signed-off-by: Aristeu Rozanski <arozansk@redhat.com>
---
 net/mac802154/rx.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/net/mac802154/rx.c b/net/mac802154/rx.c
index 446e130..d388bf2 100644
--- a/net/mac802154/rx.c
+++ b/net/mac802154/rx.c
@@ -82,6 +82,7 @@ ieee802154_subif_frame(struct ieee802154_sub_if_data *sdata,
 		break;
 	default:
 		pr_debug("invalid dest mode\n");
+		sdata->dev->stats.rx_frame_errors++;
 		goto fail;
 	}
 
@@ -97,15 +98,22 @@ ieee802154_subif_frame(struct ieee802154_sub_if_data *sdata,
 		goto fail;
 	}
 
-	sdata->dev->stats.rx_packets++;
-	sdata->dev->stats.rx_bytes += skb->len;
-
 	switch (mac_cb(skb)->type) {
+	case IEEE802154_FC_TYPE_BEACON:
+	case IEEE802154_FC_TYPE_ACK:
+	case IEEE802154_FC_TYPE_MAC_CMD:
+		sdata->dev->stats.rx_dropped++;
+		goto fail;
+
 	case IEEE802154_FC_TYPE_DATA:
+		sdata->dev->stats.rx_bytes += skb->len;
+		sdata->dev->stats.rx_packets++;
 		return ieee802154_deliver_skb(skb);
+
 	default:
 		pr_warn("ieee802154: bad frame received (type = %d)\n",
 			mac_cb(skb)->type);
+		sdata->dev->stats.rx_frame_errors++;
 		goto fail;
 	}
 
-- 
1.8.3.1

[PATCH 2/3] mac802154: use rate limited warnings for malformed frames

[Aristeu Rozanski]

Signed-off-by: Aristeu Rozanski <arozansk@redhat.com>
---
 net/mac802154/rx.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/mac802154/rx.c b/net/mac802154/rx.c
index d388bf2..11dac04 100644
--- a/net/mac802154/rx.c
+++ b/net/mac802154/rx.c
@@ -111,8 +111,8 @@ ieee802154_subif_frame(struct ieee802154_sub_if_data *sdata,
 		return ieee802154_deliver_skb(skb);
 
 	default:
-		pr_warn("ieee802154: bad frame received (type = %d)\n",
-			mac_cb(skb)->type);
+		pr_warn_ratelimited("ieee802154: bad frame received "
+				    "(type = %d)\n", mac_cb(skb)->type);
 		sdata->dev->stats.rx_frame_errors++;
 		goto fail;
 	}
-- 
1.8.3.1

Re: [PATCH 2/3] mac802154: use rate limited warnings for malformed frames

[Alexander Aring]

Hi,

On 07/22/2016 07:18 PM, Aristeu Rozanski wrote:
> Signed-off-by: Aristeu Rozanski <arozansk@redhat.com>

Acked-by: Alexander Aring <aar@pengutronix.de>

- Alex

Re: [PATCH 2/3] mac802154: use rate limited warnings for malformed frames

[Marcel Holtmann]

Hi Aristeu,

> Signed-off-by: Aristeu Rozanski <arozansk@redhat.com>
> ---
> net/mac802154/rx.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)

the patch does not apply to bluetooth-next tree.

Regards

Marcel

[PATCH 3/3] ieee802154: encrypt frame before ieee802154_subif_start_xmit is called

[Aristeu Rozanski]

Move mac802154_llsec_encrypt() call to right before dev_queue_xmit()
call and out of ieee802154_subif_start_xmit(). This prevents packets
failing to send on raw sockets.

Signed-off-by: Aristeu Rozanski <arozansk@redhat.com>
---
 include/net/mac802154.h     | 14 ++++++++++++++
 net/ieee802154/6lowpan/tx.c |  4 ++--
 net/ieee802154/socket.c     |  4 +++-
 net/mac802154/tx.c          | 29 +++++++++++++++++------------
 4 files changed, 36 insertions(+), 15 deletions(-)

diff --git a/include/net/mac802154.h b/include/net/mac802154.h
index e465c85..ee24a0e 100644
--- a/include/net/mac802154.h
+++ b/include/net/mac802154.h
@@ -377,6 +377,20 @@ void ieee802154_wake_queue(struct ieee802154_hw *hw);
 void ieee802154_stop_queue(struct ieee802154_hw *hw);
 
 /**
+ * ieee802154_finish_frame - finish a frame before queueing for transmission
+ *
+ * @skb: the buffer to be finished
+ */
+#ifdef CONFIG_MAC802154
+int ieee802154_finish_frame(struct sk_buff *skb);
+#else	/* CONFIG_MAC802154 */
+static inline int ieee802154_finish_frame(struct sk_buff *skb)
+{
+	return dev_queue_xmit(skb);
+}
+#endif	/* !CONFIG_MAC802154 */
+
+/**
  * ieee802154_xmit_complete - frame transmission complete
  *
  * @hw: pointer as obtained from ieee802154_alloc_hw().
diff --git a/net/ieee802154/6lowpan/tx.c b/net/ieee802154/6lowpan/tx.c
index e459afd..113d3c8 100644
--- a/net/ieee802154/6lowpan/tx.c
+++ b/net/ieee802154/6lowpan/tx.c
@@ -135,7 +135,7 @@ lowpan_xmit_fragment(struct sk_buff *skb, const struct ieee802154_hdr *wpan_hdr,
 
 	raw_dump_table(__func__, " fragment dump", frag->data, frag->len);
 
-	return dev_queue_xmit(frag);
+	return ieee802154_finish_frame(frag);
 }
 
 static int
@@ -286,7 +286,7 @@ netdev_tx_t lowpan_xmit(struct sk_buff *skb, struct net_device *ldev)
 		skb->dev = lowpan_802154_dev(ldev)->wdev;
 		ldev->stats.tx_packets++;
 		ldev->stats.tx_bytes += dgram_size;
-		return dev_queue_xmit(skb);
+		return ieee802154_finish_frame(skb);
 	} else {
 		netdev_tx_t rc;
 
diff --git a/net/ieee802154/socket.c b/net/ieee802154/socket.c
index e0bd013..8ef159a 100644
--- a/net/ieee802154/socket.c
+++ b/net/ieee802154/socket.c
@@ -33,6 +33,7 @@
 
 #include <net/af_ieee802154.h>
 #include <net/ieee802154_netdev.h>
+#include <net/mac802154.h>
 
 /* Utility function for families */
 static struct net_device*
@@ -306,6 +307,7 @@ static int raw_sendmsg(struct sock *sk, struct msghdr *msg, size_t size)
 
 	dev_put(dev);
 
+	/* For raw sockets we don't go through ieee802154_finish_frame() */
 	err = dev_queue_xmit(skb);
 	if (err > 0)
 		err = net_xmit_errno(err);
@@ -695,7 +697,7 @@ static int dgram_sendmsg(struct sock *sk, struct msghdr *msg, size_t size)
 
 	dev_put(dev);
 
-	err = dev_queue_xmit(skb);
+	err = ieee802154_finish_frame(skb);
 	if (err > 0)
 		err = net_xmit_errno(err);
 
diff --git a/net/mac802154/tx.c b/net/mac802154/tx.c
index 7e25345..82a996e 100644
--- a/net/mac802154/tx.c
+++ b/net/mac802154/tx.c
@@ -56,6 +56,23 @@ err_tx:
 	netdev_dbg(dev, "transmission failed\n");
 }
 
+int ieee802154_finish_frame(struct sk_buff *skb)
+{
+	struct net_device *dev = skb->dev;
+	struct ieee802154_sub_if_data *sdata = IEEE802154_DEV_TO_SUB_IF(dev);
+	int rc;
+
+	rc = mac802154_llsec_encrypt(&sdata->sec, skb);
+	if (rc) {
+		netdev_warn(dev, "encryption failed: %i\n", rc);
+		kfree_skb(skb);
+		return NET_XMIT_DROP;
+	}
+
+	return dev_queue_xmit(skb);
+}
+EXPORT_SYMBOL_GPL(ieee802154_finish_frame);
+
 static netdev_tx_t
 ieee802154_tx(struct ieee802154_local *local, struct sk_buff *skb)
 {
@@ -107,18 +124,6 @@ netdev_tx_t
 ieee802154_subif_start_xmit(struct sk_buff *skb, struct net_device *dev)
 {
 	struct ieee802154_sub_if_data *sdata = IEEE802154_DEV_TO_SUB_IF(dev);
-	int rc;
-
-	/* TODO we should move it to wpan_dev_hard_header and dev_hard_header
-	 * functions. The reason is wireshark will show a mac header which is
-	 * with security fields but the payload is not encrypted.
-	 */
-	rc = mac802154_llsec_encrypt(&sdata->sec, skb);
-	if (rc) {
-		netdev_warn(dev, "encryption failed: %i\n", rc);
-		kfree_skb(skb);
-		return NETDEV_TX_OK;
-	}
 
 	skb->skb_iif = dev->ifindex;
 
-- 
1.8.3.1

Re: [PATCH 3/3] ieee802154: encrypt frame before ieee802154_subif_start_xmit is called

[Alexander Aring]

Hi,

On 07/22/2016 07:18 PM, Aristeu Rozanski wrote:
> Move mac802154_llsec_encrypt() call to right before dev_queue_xmit()
> call and out of ieee802154_subif_start_xmit(). This prevents packets
> failing to send on raw sockets.
> 
> Signed-off-by: Aristeu Rozanski <arozansk@redhat.com>

Please don't use af_802154 raw sockets which are a long time broken, use
AF_PACKET raw. See as example [0] which allows to run RIOT-OS native as
elf binary in userspace and talk with the 802.15.4 6LoWPAN kernel stuff.
Also the AF_PACKET RAW will directly called xmit callback and not
dev_queue_xmit -> no qdisc is involved and mostly you like to have that
for such kind of sockets.

The socket "af_802154" should be used to doing MAC frames by kernel,
AF_PACKET DGRAM sockets has a very limited UAPI for that.

Unfortunately this will not fix your issues if you sending out raw
frames with security bits enabled.

You found the right issue because the "xmit callback" should not touch
the header again. I also had issues by using wireshark, because it
doesn't show the encrypted frame.

It's the same for rx handling where decryrption should be "somehow" done
after the packet-layer.

The issue getting even bigger (and the reason why I didn't solved it)
when you think about HardMAC transceivers.

HardMAC transceivers do encryption end decryption on transceivers side,
so on linux side we have always not encrypted frames. This issue is a
whole unsolved issue in out architecture. Maybe we should simple handle
then as "not encrypted" but then the MAC headers frame control bits need
to show that. Or we tell the userspace, "You have here a HardMAC
transceiver and please note there is no control for showing encrypted
payload while dumping e.g. in libpcap". I think it's okay to have the
frame control secbits sets there, but we need some way to tell the
userspace "there is no support for showing encrypted payload" for
HardMAC transceivers and security enabled in frames.

I had no idea for that currently, HardMAC transceivers will also
introduce some other stuff we need to do. At the moment we ignore
"HardMAC" transceivers at some points right now.

I am fine if we fix this behaviour for SoftMAC transceivers side (later
we can introduce such flag for UAPI that "encrypted payload not
supported").

> ---
>  include/net/mac802154.h     | 14 ++++++++++++++
>  net/ieee802154/6lowpan/tx.c |  4 ++--
>  net/ieee802154/socket.c     |  4 +++-
>  net/mac802154/tx.c          | 29 +++++++++++++++++------------
>  4 files changed, 36 insertions(+), 15 deletions(-)
> 
> diff --git a/include/net/mac802154.h b/include/net/mac802154.h
> index e465c85..ee24a0e 100644
> --- a/include/net/mac802154.h
> +++ b/include/net/mac802154.h
> @@ -377,6 +377,20 @@ void ieee802154_wake_queue(struct ieee802154_hw *hw);
>  void ieee802154_stop_queue(struct ieee802154_hw *hw);
>  
>  /**
> + * ieee802154_finish_frame - finish a frame before queueing for transmission
> + *
> + * @skb: the buffer to be finished
> + */
> +#ifdef CONFIG_MAC802154
> +int ieee802154_finish_frame(struct sk_buff *skb);
> +#else	/* CONFIG_MAC802154 */
> +static inline int ieee802154_finish_frame(struct sk_buff *skb)
> +{
> +	return dev_queue_xmit(skb);
> +}
> +#endif	/* !CONFIG_MAC802154 */

This will not work if we later add support for running HardMAC and
SoftMAC. I tried to fix it on my own (can't find the branch anymore and
patches never get into mainline). If I remember correctly, I used a callback
but I saw then the better idea would it to move encryption to
wpan_dev_hard_header, callback.

Some parameters what for a frame we need and SoftMAC/HardMAC can be
doing (something) for that. For HardMAC I need to figure out how this
can be working then. At least wireshark/tcpdump will see the mac header
so everybody should do the same stuff there.

If both do the same stuff, it's indicate that this is for
"net/ieee802154" branch.

Another issue is that wpan_dev_hard_header callback was simpled
copy&pasted right now from the global generic "dev_hard_header" callback
which had several issues, because it was called by AF_PACKET DGRAM
sockets and this had 8 bytes limited parameters for e.g. addresses and
the callback had some out-of-bound array access there.

Nevertheless the wpan_dev_hard_header fixed it because it will called by
802.15.4 upper layers only. The bad thing is that skb->cb is still used
there, which was a workaround for "dev_hard_header" callback to extend
the callback with additional information. This should be removed, since
we have our own callback for that where we have control over it and can
add more parameters.

Everything is ugly solved and fixing this requires to fix header
creation currently. :-) I also don't know if we really needs this
callback or simple have creation functions in "net/ieee802154" which
will be called by upper layers. HardMAC drivers need to parse the frame
then and doing something that these settings for this frame will be done
on transceivers side.

It's a big TODO there.

---

Nevertheless it's okay for me to fix that on rx/tx side so the RAW
socket are really RAW sockets which sends user generated frames out
(also with secbit enabled).

But please fix this for rx/tx side and not in a way which makes the
behaviour with future HardMAC transceivers more possible.

For tx side it should be okay to move it to the end of
"wpan_dev_hard_header" callback which should be not called by AF_PACKET
raw sockets. This should be a simple fix...

For rx side it would be harder. I thought maybe there exists some
packet_layer "hook" before starting packet_type->func e.g. [1]

Do encrypt payload, so we don't need to duplicate code much... if such
hook doesn't exist then we need out own hook mechanism for
packet_type->func callback.

- Alex

[0] https://github.com/linux-wpan/RIOT/blob/26a86d6fbd5695fe56c1defb097470787922f440/cpu/native/netdev2_raw802154/netdev2_raw802154.c#L281
[1] http://lxr.free-electrons.com/source/net/ieee802154/6lowpan/rx.c#L320

Re: [PATCH 3/3] ieee802154: encrypt frame before ieee802154_subif_start_xmit is called

[Aristeu Rozanski]

Hi Alexander,
On Sat, Jul 23, 2016 at 03:43:05PM +0200, Alexander Aring wrote:
> Please don't use af_802154 raw sockets which are a long time broken, use
> AF_PACKET raw. See as example [0] which allows to run RIOT-OS native as
> elf binary in userspace and talk with the 802.15.4 6LoWPAN kernel stuff.
> Also the AF_PACKET RAW will directly called xmit callback and not
> dev_queue_xmit -> no qdisc is involved and mostly you like to have that
> for such kind of sockets.
> 
> The socket "af_802154" should be used to doing MAC frames by kernel,
> AF_PACKET DGRAM sockets has a very limited UAPI for that.

my idea using it was to have the MAC implemented in userspace and use
raw sockets to interface with existing drivers.

> Unfortunately this will not fix your issues if you sending out raw
> frames with security bits enabled.
>
> You found the right issue because the "xmit callback" should not touch
> the header again. I also had issues by using wireshark, because it
> doesn't show the encrypted frame.
> 
> It's the same for rx handling where decryrption should be "somehow" done
> after the packet-layer.

I haven't notice that yet, still trying to work out sending and
receiving frames without encryption.

> The issue getting even bigger (and the reason why I didn't solved it)
> when you think about HardMAC transceivers.
> 
> HardMAC transceivers do encryption end decryption on transceivers side,
> so on linux side we have always not encrypted frames. This issue is a
> whole unsolved issue in out architecture. Maybe we should simple handle
> then as "not encrypted" but then the MAC headers frame control bits need
> to show that. Or we tell the userspace, "You have here a HardMAC
> transceiver and please note there is no control for showing encrypted
> payload while dumping e.g. in libpcap". I think it's okay to have the
> frame control secbits sets there, but we need some way to tell the
> userspace "there is no support for showing encrypted payload" for
> HardMAC transceivers and security enabled in frames.
> 
> I had no idea for that currently, HardMAC transceivers will also
> introduce some other stuff we need to do. At the moment we ignore
> "HardMAC" transceivers at some points right now.
> 
> I am fine if we fix this behaviour for SoftMAC transceivers side (later
> we can introduce such flag for UAPI that "encrypted payload not
> supported").
> 
> This will not work if we later add support for running HardMAC and
> SoftMAC. I tried to fix it on my own (can't find the branch anymore and
> patches never get into mainline). If I remember correctly, I used a callback
> but I saw then the better idea would it to move encryption to
> wpan_dev_hard_header, callback.
> 
> Some parameters what for a frame we need and SoftMAC/HardMAC can be
> doing (something) for that. For HardMAC I need to figure out how this
> can be working then. At least wireshark/tcpdump will see the mac header
> so everybody should do the same stuff there.
> 
> If both do the same stuff, it's indicate that this is for
> "net/ieee802154" branch.
> 
> Another issue is that wpan_dev_hard_header callback was simpled
> copy&pasted right now from the global generic "dev_hard_header" callback
> which had several issues, because it was called by AF_PACKET DGRAM
> sockets and this had 8 bytes limited parameters for e.g. addresses and
> the callback had some out-of-bound array access there.
> 
> Nevertheless the wpan_dev_hard_header fixed it because it will called by
> 802.15.4 upper layers only. The bad thing is that skb->cb is still used
> there, which was a workaround for "dev_hard_header" callback to extend
> the callback with additional information. This should be removed, since
> we have our own callback for that where we have control over it and can
> add more parameters.
> 
> Everything is ugly solved and fixing this requires to fix header
> creation currently. :-) I also don't know if we really needs this
> callback or simple have creation functions in "net/ieee802154" which
> will be called by upper layers. HardMAC drivers need to parse the frame
> then and doing something that these settings for this frame will be done
> on transceivers side.
> 
> It's a big TODO there.

Thanks for the explanation, as someone starting working with this it
saves me a ton of time! Much appreciated!

> Nevertheless it's okay for me to fix that on rx/tx side so the RAW
> socket are really RAW sockets which sends user generated frames out
> (also with secbit enabled).
> 
> But please fix this for rx/tx side and not in a way which makes the
> behaviour with future HardMAC transceivers more possible.
> 
> For tx side it should be okay to move it to the end of
> "wpan_dev_hard_header" callback which should be not called by AF_PACKET
> raw sockets. This should be a simple fix...
> 
> For rx side it would be harder. I thought maybe there exists some
> packet_layer "hook" before starting packet_type->func e.g. [1]
> 
> Do encrypt payload, so we don't need to duplicate code much... if such
> hook doesn't exist then we need out own hook mechanism for
> packet_type->func callback.

I'll take a look on it and see if I can come with an acceptable
solution.
Thanks!

-- 
Aristeu

Re: [PATCH 1/3] mac802154: don't warn on unsupported frames

[Alexander Aring]

Hi,

On 07/22/2016 07:18 PM, Aristeu Rozanski wrote:
> Just because we don't support certain types of frames yet doesn't mean
> we have to flood the message log with warnings about "invalid" frames.
> 

agree, but this patch introduce a different stats handling right now. :-)

> Signed-off-by: Aristeu Rozanski <arozansk@redhat.com>
> ---
>  net/mac802154/rx.c | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/net/mac802154/rx.c b/net/mac802154/rx.c
> index 446e130..d388bf2 100644
> --- a/net/mac802154/rx.c
> +++ b/net/mac802154/rx.c
> @@ -82,6 +82,7 @@ ieee802154_subif_frame(struct ieee802154_sub_if_data *sdata,
>  		break;
>  	default:
>  		pr_debug("invalid dest mode\n");
> +		sdata->dev->stats.rx_frame_errors++;
>  		goto fail;
>  	}
>  
> @@ -97,15 +98,22 @@ ieee802154_subif_frame(struct ieee802154_sub_if_data *sdata,
>  		goto fail;
>  	}
>  
> -	sdata->dev->stats.rx_packets++;
> -	sdata->dev->stats.rx_bytes += skb->len;
> -
>  	switch (mac_cb(skb)->type) {
> +	case IEEE802154_FC_TYPE_BEACON:
> +	case IEEE802154_FC_TYPE_ACK:
> +	case IEEE802154_FC_TYPE_MAC_CMD:
> +		sdata->dev->stats.rx_dropped++;
> +		goto fail;
> +
>  	case IEEE802154_FC_TYPE_DATA:
> +		sdata->dev->stats.rx_bytes += skb->len;
> +		sdata->dev->stats.rx_packets++;

This should be moved into ieee802154_deliver_skb, right before calling
netif_rx(...);

The reason is because we have for "stats.rx_bytes" and
"stats.rx_packets" a very clean definition what it means. It means, the
frames hit a state where it is ready to put it into the packet-layer,
this is what netif_rx is doing.

It's okay for me to add support for rx_frame_errors and also rx_dropped
stuff. But with a very big warning that the meaning of such stats will
be touched again later if we found some strategie where we we define
what each stats attribute means. Also there exists subsystems which
simple ignore these stats, because at userspace side you need always to
lookup what the definition means and when we run such stats counters and
when not and some subsystems don't care about that.

- Alex

Re: [PATCH 1/3] mac802154: don't warn on unsupported frames

[Aristeu Rozanski]

Hi Alexander,
On Sat, Jul 23, 2016 at 02:46:28PM +0200, Alexander Aring wrote:
> agree, but this patch introduce a different stats handling right now. :-)

sorry, should have split both parts in different patches.

> This should be moved into ieee802154_deliver_skb, right before calling
> netif_rx(...);
> 
> The reason is because we have for "stats.rx_bytes" and
> "stats.rx_packets" a very clean definition what it means. It means, the
> frames hit a state where it is ready to put it into the packet-layer,
> this is what netif_rx is doing.
> 
> It's okay for me to add support for rx_frame_errors and also rx_dropped
> stuff. But with a very big warning that the meaning of such stats will
> be touched again later if we found some strategie where we we define
> what each stats attribute means. Also there exists subsystems which
> simple ignore these stats, because at userspace side you need always to
> lookup what the definition means and when we run such stats counters and
> when not and some subsystems don't care about that.

Fair enough, I'll redo the patch just removing the warning.
Thanks!

-- 
Aristeu