Hi Alex, 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. I just got a kernel log http://ix.io/lng about some skb issue so this could be related, I am investigating it. > > 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; Cheers, Jukka -- 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
