On Wed, 2011-05-18 at 13:40 +0200, MichaÅ MirosÅaw wrote: > >> >> Not more other restrictions, skb clone is OK. pskb_expand_head() > looks > >> >> OK to me from code review. > >> > Hmm. pskb_expand_head calls skb_release_data while keeping > >> > references to pages. How is that ok? What do I miss? > >> It's making copy of the skb_shinfo earlier, so the pages refcount > >> stays the same. > > Exactly. But the callback is invoked so the guest thinks it's ok to > > change this memory. If it does a corrupted packet will be sent out. > > Hmm. I tool a quick look at skb_clone(), and it looks like this > sequence will break this scheme: > > skb2 = skb_clone(skb...); > kfree_skb(skb) or pskb_expand_head(skb); /* callback called */ > [use skb2, pages still referenced] > kfree_skb(skb); /* callback called again */ > > This sequence is common in bridge, might be in other places. > > Maybe this ubuf thing should just track clones? This will make it work > on all devices then. The callback was only invoked when last reference of skb was gone. skb_clone does increase skb refcnt. I tested tcpdump on lower device, it worked. For the sequence of: skb_clone -> last refcnt + 1 kfree_skb() or pskb_expand_head -> callback not called kfree_skb() -> callback called I will check page refcount to see whether it's balanced. Thanks shirley -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
