Hello,
Patches are mostly ok with me.
On 10/27/2011 07:40 PM, Alexander Smirnov wrote:
> This patch adds support for fragmentation. Now there are no
> limitations on packet size. Currently fragments are identified
> by tag only.
>
> Signed-off-by: Alexander Smirnov<alex.bluesman.smir...@gmail.com>
> ---
> include/net/ieee802154.h | 6 +
> net/ieee802154/6lowpan.c | 248
> +++++++++++++++++++++++++++++++++++++++++++++-
> net/ieee802154/6lowpan.h | 18 ++++
> 3 files changed, 269 insertions(+), 3 deletions(-)
[skipped]
> +static void lowpan_fragment_timer_expired(unsigned long entry_addr)
> +{
> + struct lowpan_fragment *entry = (struct lowpan_fragment *)entry_addr;
> +
> + pr_debug("%s: timer expired for frame with tag %d\n", __func__,
> + entry->tag);
> +
> + spin_lock(&flist_lock);
> + list_del(&entry->list);
> + spin_unlock(&flist_lock);
> +
> + dev_kfree_skb(entry->skb);
> + kfree(entry);
> +}
This does not look good wrt. RCU list traversal later what if RCU user
references the entry you have just kfree()'d?
> +
> static int
> lowpan_process_data(struct sk_buff *skb)
> {
> @@ -525,6 +566,105 @@ lowpan_process_data(struct sk_buff *skb)
> if (skb->len< 2)
> goto drop;
> iphc0 = lowpan_fetch_skb_u8(skb);
> +
> + /* fragments assembling */
> + switch (iphc0& LOWPAN_DISPATCH_MASK) {
> + case LOWPAN_DISPATCH_FRAG1:
> + case LOWPAN_DISPATCH_FRAGN:
> + {
> + struct lowpan_fragment *frame;
> + u16 tag;
> +
> + tmp = lowpan_fetch_skb_u8(skb); /* frame length */
Hmmm. I'd like to have a more descriptive variable name here.
> + tag = lowpan_fetch_skb_u16(skb);
> +
> + /*
> + * check if frame assembling with the same tag is
> + * already in progress
> + */
> + spin_lock(&flist_lock);
> +
> + rcu_read_lock();
> + list_for_each_entry_rcu(frame,&lowpan_fragments, list)
> + if (frame->tag == tag)
> + break;
> + rcu_read_unlock();
> +
> + /* alloc new frame structure */
> + if (frame->tag != tag) {
> + frame = kzalloc(sizeof(struct lowpan_fragment),
> + GFP_ATOMIC);
> + if (!frame)
> + goto drop;
> +
> + INIT_LIST_HEAD(&frame->list);
> +
> + frame->length = (iphc0& 7) | (tmp<< 3);
> + frame->tag = tag;
> +
> + /* allocate buffer for frame assembling */
> + frame->skb = alloc_skb(frame->length +
> + sizeof(struct ipv6hdr), GFP_ATOMIC);
> +
> + /* copy control information */
> + memcpy(frame->skb->cb, skb->cb, sizeof(frame->skb->cb));
This looks wrong. What if skb->cb in reality is contains a list_head
instance here? Either we are the "cb owners" here and we can (mostly)
ignore cb or we are not and we should not touch cb at all.
Could you please add some notes here, as what can be in CB, what is used
later, who owns that data, etc.
> + frame->skb->priority = skb->priority;
> + frame->skb->dev = skb->dev;
> +
> + if (!frame->skb) {
> + kfree(frame);
> + goto drop;
> + }
> +
> + /* reserve headroom for uncompressed ipv6 header */
> + skb_reserve(frame->skb, sizeof(struct ipv6hdr));
> + skb_put(frame->skb, frame->length);
> +
> + init_timer(&frame->timer);
> + /* time out is the same as for ipv6 - 60 sec */
> + frame->timer.expires = jiffies + LOWPAN_FRAG_TIMEOUT;
> + frame->timer.data = (unsigned long)frame;
> + frame->timer.function = lowpan_fragment_timer_expired;
> +
> + add_timer(&frame->timer);
> +
> + list_add_tail(&frame->list,&lowpan_fragments);
> + }
> +
> + if ((iphc0& LOWPAN_DISPATCH_MASK) == LOWPAN_DISPATCH_FRAG1)
> + goto drop;
> +
> + tmp = lowpan_fetch_skb_u8(skb); /* fetch offset */
> +
> + /* if payload fits buffer, copy it */
> + if (likely((tmp * 8 + skb->len)<= frame->length))
> + skb_copy_to_linear_data_offset(frame->skb, tmp * 8,
> + skb->data, skb->len);
> + else
> + goto drop;
Does this code cope with out-of-order packets? Are such fragments
permitted by RFC?
> +static int
> +lowpan_fragment_skb(struct sk_buff *skb, u8 *head,
> + int mlen, int plen, int offset)
> +{
> + struct sk_buff *frag;
> + int hlen, ret;
> +
> + /* if payload length is zero, therefore it's a first fragment */
> + hlen = (plen == 0 ? LOWPAN_FRAG1_HEAD_SIZE : LOWPAN_FRAGN_HEAD_SIZE);
> +
> + lowpan_raw_dump_inline(__func__, "6lowpan fragment header", head, hlen);
> +
> + frag = dev_alloc_skb(hlen + mlen + plen + IEEE802154_MFR_SIZE);
> + if (!frag)
> + return -ENOMEM;
> +
> + /* copy control information */
> + memcpy(frag->cb, skb->cb, sizeof(frag->cb));
Same issue with CB.
> + frag->priority = skb->priority;
> + frag->dev = skb->dev;
> +
> + /* copy header, MFR and payload */
> + memcpy(skb_put(frag, mlen), skb->data, mlen);
> + memcpy(skb_put(frag, hlen), head, hlen);
> +
> + if (plen)
> + skb_copy_from_linear_data_offset(skb, offset + mlen,
> + skb_put(frag, plen), plen);
> +
> + lowpan_raw_dump_table(__func__, " raw fragment dump", frag->data,
> + frag->len);
> +
> + ret = dev_queue_xmit(frag);
> +
> + if (ret< 0)
> + dev_kfree_skb(frag);
> +
> + return ret;
> +}
> +
> static netdev_tx_t lowpan_xmit(struct sk_buff *skb, struct net_device *dev)
> {
> int err = 0;
> + int header_length, payload_length, tag, offset = 0;
> + u8 head[5];
>
> pr_debug("(%s): package xmit\n", __func__);
>
> skb->dev = lowpan_dev_info(dev)->real_dev;
> if (skb->dev == NULL) {
> pr_debug("(%s) ERROR: no real wpan device found\n", __func__);
> - dev_kfree_skb(skb);
> - } else
> + goto error;
> + }
> +
> + if (skb->len<= IEEE802154_MTU) {
> err = dev_queue_xmit(skb);
> + goto out;
> + }
> +
> + pr_debug("(%s): frame is too big, fragmentation is needed\n",
> + __func__);
> +
> + header_length = lowpan_get_mac_header_length(skb);
> + payload_length = skb->len - header_length;
> + tag = fragment_tag++;
> +
> + /* first fragment header */
> + head[0] = LOWPAN_DISPATCH_FRAG1 | (payload_length& 0x7);
> + head[1] = (payload_length>> 3)& 0xff;
> + head[2] = tag& 0xff;
> + head[3] = tag>> 8;
> +
> + err = lowpan_fragment_skb(skb, head, header_length, 0, 0);
> +
> + /* next fragment header */
> + head[0]&= ~LOWPAN_DISPATCH_FRAG1;
> + head[0] |= LOWPAN_DISPATCH_FRAGN;
> +
> + while ((payload_length - offset> 0)&& (err>= 0)) {
> + int len = LOWPAN_FRAG_SIZE;
> +
> + head[4] = offset / 8;
> +
> + if (payload_length - offset< len)
> + len = payload_length - offset;
> +
> + err = lowpan_fragment_skb(skb, head, header_length,
> + len, offset);
> + offset += len;
> + }
Please refactor to a separate function, if possible.
> +
> +error:
> + dev_kfree_skb(skb);
> +out:
> + if (err< 0)
> + pr_debug("(%s): ERROR: xmit failed\n", __func__);
>
> return (err< 0 ? NETDEV_TX_BUSY : NETDEV_TX_OK);
> }
> @@ -765,8 +1000,15 @@ static int lowpan_rcv(struct sk_buff *skb, struct
> net_device *dev,
> goto drop;
>
> /* check that it's our buffer */
> - if ((skb->data[0]& 0xe0) == 0x60)
> + switch (skb->data[0]& 0xe0) {
> + case 0x60: /* ipv6 datagram */
> + case 0xc0: /* first fragment header */
> + case 0xe0: /* next fragments headers */
Hmmm.
> lowpan_process_data(skb);
> + break;
> + default:
> + break;
> + }
>
> return NET_RX_SUCCESS;
--
With best wishes
Dmitry
------------------------------------------------------------------------------
RSA® Conference 2012
Save $700 by Nov 18
Register now
http://p.sf.net/sfu/rsa-sfdev2dev1
_______________________________________________
Linux-zigbee-devel mailing list
Linux-zigbee-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-zigbee-devel
