Hi Alex,
the fixes in this patchset are fine and I verified with real hw that
everything works ok. The fix in this patch did not fix the skb poison
overwritten problem I am seeing and that needs more investigation.
Acked-by: Jukka Rissanen <jukka.rissanen@xxxxxxxxxxxxxxx>
Cheers,
Jukka
On ti, 2015-10-13 at 13:42 +0200, Alexander Aring wrote:
> This patch adds a check if the dataroom of skb contains a dispatch value
> by checking if skb->len != 0. This patch also change the dispatch
> evaluation by the recently introduced helpers for checking the common
> 6LoWPAN dispatch values for IPv6 and IPHC header.
>
> There was also a forgotten else branch which should drop the packet if
> no matching dispatch is available.
>
> Signed-off-by: Alexander Aring <alex.aring@xxxxxxxxx>
> ---
> Jukka,
>
> there seems something wrong or I don't get it. There is a skb_share_check but
> inside the lowpan_is_iphc branch there is a skb_clone again. From my
> understanding skb_share_check will check if the skb (structure) is used
> somewhere else by looking at some refcounts, if yes then clone it.
>
> So after that the skb is always a clone, which means the skb (structure only)
> is not used somewhere else. Also IPHC will manipulate the skb (buffer) as well,
> so we need somewhere to be sure that the buffer isn't shared. There exists a
> function "skb_unshare" which is identically like skb_share_check just for the
> buffer.
>
> It could be that you ensure that the buffer is not shared somewhere else. Anyway
> I think the solution with checking the "refcounts" is a very low cost operation.
>
> btw:
> In case of IPv6 dispatch this is something else, because we move pointers there
> only (adjust skb->headroom with skb->data), not manipulate the buffer, so clone
> is enough there.
>
> net/bluetooth/6lowpan.c | 57 +++++++++++++++++++++++--------------------------
> 1 file changed, 27 insertions(+), 30 deletions(-)
>
> diff --git a/net/bluetooth/6lowpan.c b/net/bluetooth/6lowpan.c
> index db73b8a..665bf38 100644
> --- a/net/bluetooth/6lowpan.c
> +++ b/net/bluetooth/6lowpan.c
> @@ -314,15 +314,17 @@ static int recv_pkt(struct sk_buff *skb, struct net_device *dev,
> if (!netif_running(dev))
> goto drop;
>
> - if (dev->type != ARPHRD_6LOWPAN)
> + if (dev->type != ARPHRD_6LOWPAN || !skb->len)
> goto drop;
>
> + skb_reset_network_header(skb);
> +
> skb = skb_share_check(skb, GFP_ATOMIC);
> if (!skb)
> goto drop;
>
> /* check that it's our buffer */
> - if (skb->data[0] == LOWPAN_DISPATCH_IPV6) {
> + if (lowpan_is_ipv6(*skb_network_header(skb))) {
> /* Copy the packet so that the IPv6 header is
> * properly aligned.
> */
> @@ -334,7 +336,6 @@ static int recv_pkt(struct sk_buff *skb, struct net_device *dev,
> local_skb->protocol = htons(ETH_P_IPV6);
> local_skb->pkt_type = PACKET_HOST;
>
> - skb_reset_network_header(local_skb);
> skb_set_transport_header(local_skb, sizeof(struct ipv6hdr));
>
> if (give_skb_to_upper(local_skb, dev) != NET_RX_SUCCESS) {
> @@ -347,38 +348,34 @@ static int recv_pkt(struct sk_buff *skb, struct net_device *dev,
>
> consume_skb(local_skb);
> consume_skb(skb);
> - } else {
> - switch (skb->data[0] & 0xe0) {
> - case LOWPAN_DISPATCH_IPHC: /* ipv6 datagram */
> - local_skb = skb_clone(skb, GFP_ATOMIC);
> - if (!local_skb)
> - goto drop;
> + } else if (lowpan_is_iphc(*skb_network_header(skb))) {
> + local_skb = skb_clone(skb, GFP_ATOMIC);
> + if (!local_skb)
> + goto drop;
>
> - ret = iphc_decompress(local_skb, dev, chan);
> - if (ret < 0) {
> - kfree_skb(local_skb);
> - goto drop;
> - }
> + ret = iphc_decompress(local_skb, dev, chan);
> + if (ret < 0) {
> + kfree_skb(local_skb);
> + goto drop;
> + }
>
> - local_skb->protocol = htons(ETH_P_IPV6);
> - local_skb->pkt_type = PACKET_HOST;
> - local_skb->dev = dev;
> + local_skb->protocol = htons(ETH_P_IPV6);
> + local_skb->pkt_type = PACKET_HOST;
> + local_skb->dev = dev;
>
> - if (give_skb_to_upper(local_skb, dev)
> - != NET_RX_SUCCESS) {
> - kfree_skb(local_skb);
> - goto drop;
> - }
> + if (give_skb_to_upper(local_skb, dev)
> + != NET_RX_SUCCESS) {
> + kfree_skb(local_skb);
> + goto drop;
> + }
>
> - dev->stats.rx_bytes += skb->len;
> - dev->stats.rx_packets++;
> + dev->stats.rx_bytes += skb->len;
> + dev->stats.rx_packets++;
>
> - consume_skb(local_skb);
> - consume_skb(skb);
> - break;
> - default:
> - break;
> - }
> + consume_skb(local_skb);
> + consume_skb(skb);
> + } else {
> + goto drop;
> }
>
> return NET_RX_SUCCESS;
--
To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
