| Message ID | 1469207896-26481-3-git-send-email-arozansk@redhat.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
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 -- To unsubscribe from this list: send the line "unsubscribe linux-wpan" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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!
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;
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(-)