Hi Alex, On 16/09/14 18:38, Alexander Aring wrote:
Hi Martin, On Tue, Sep 16, 2014 at 03:44:43PM +0100, Martin Townsend wrote:I would like to keep freeing skb's out of process_data as process_data will become something like iphc_decompress_hdr and it would be good if that's all it did. Otherwise I feel we are going to put a constraint on all future header decompression routines in that they must free the skb on error. I think it would be better to defer this so on error you might want to try something else with the skb, maybe not but at least the option is there. So how about struct sk_buff * ret_skb; switch (skb->data[0] & 0xe0) { case LOWPAN_DISPATCH_IPHC: /* ipv6 datagram */ ret_skb = process_data(skb, &hdr); if (IS_ERR(ret_skb)) goto drop_skb; else skb = ret_skb; break; I know we currently have 3 calls to process_data so it will look fairly ugly in this patch but in my next patch to fix lowpan_rcv to handle uncompressed IPv6 packets that are fragmented there will only be one call to process_data so it won't look so bad. You could even wrap it in a macro but I'm not a fan of this as they can obfuscate the code a bit. Thoughts?sorry, I can't follow how this solve the issue if the "parameter skb" is already consumed or not. If process_data returns a error before parameter consume, then we should run kfree_skb(parameter_skb), if it's afterwards we should do nothing. Point is we don't know that there. I suppose if we do consume_skb and refcount reach 0 the parameter_skb becomes a dangling pointer. - Alex
process_data never consumes the skb, it may copy_expand and then consume the old one so it will either return an error or an skb that contains the uncompressed ipv6 header. By calling process_data using a different sk_buff pointer (ret_skb) that the parameter we can check this for an error and if so goto drop_skb which will kfree_skb(skb) which is fine as skb is still valid. if ret_skb is good and we assign to skb and carry on to the function that passes the skb up the stack, lowpan_give_skb_to_devices, which deals with either consuming or kfreeing.
Or am I missing something? - Martin. -- 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
