Mitch Williams
2009-Feb-18 00:24 UTC
[Xen-devel] [PATCH ixgbe] Don''t depend of skb->data for VMDq
Occasionally, vmq_alloc_skb() will give us an skb with a nonzero but bogus data pointer. Dereferencing this pointer will then cause a panic. Although we really need to find out where these are coming from, this patch will enable us to continue development and testing in the short term. Signed-off-by: Mitch Williams <mitch.a.williams@intel.com> (Patch is attached as well as inline due to email mangling issues.) diff -urpN -X dontdiff b/drivers/net/ixgbe/ixgbe_main.c linux-2.6.18.hg/drivers/net/ixgbe/ixgbe_main.c --- b/drivers/net/ixgbe/ixgbe_main.c 2009-02-17 13:48:00.000000000 -0800 +++ linux-2.6.18.hg/drivers/net/ixgbe/ixgbe_main.c 2009-02-17 14:06:58.000000000 -0800 @@ -1144,7 +1144,8 @@ static bool ixgbe_clean_rx_irq(struct ix total_rx_bytes += skb->len; total_rx_packets++; #ifdef CONFIG_XEN_NETDEV2_VMQ - if (skb->data) + if (!((adapter->flags & IXGBE_FLAG_VMDQ_ENABLED) && + rx_ring->queue_index)) #endif skb->protocol = eth_type_trans(skb, adapter->netdev); _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Steven Smith
2009-Feb-18 14:10 UTC
[Xen-devel] Re: [PATCH ixgbe] Don''t depend of skb->data for VMDq
> Occasionally, vmq_alloc_skb() will give us an skb with a nonzero but > bogus data pointer. Dereferencing this pointer will then cause a > panic.That''s disturbing. The SKB needs to have a data area (because otherwise there''s nowhere to put the skb_shinfo, and hence no frags list), so the data pointer really ought to be valid (i.e. NULL is also bad here). Is it possible that the entire SKB is bad? If we''re doing a VMQ receive, we don''t actually use very much of the skbuff structure apart from the shinfo bit. If the packet were getting kfree_skb()ed somewhere then it might look like this. I''ve applied the patch, but I think there''s something fairly fundamentally wrong here. Steven. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Williams, Mitch A
2009-Feb-18 22:18 UTC
[Xen-devel] RE: [PATCH ixgbe] Don''t depend of skb->data for VMDq
>> Occasionally, vmq_alloc_skb() will give us an skb with a nonzero but >> bogus data pointer. Dereferencing this pointer will then cause a >> panic. >That''s disturbing. The SKB needs to have a data area (because >otherwise there''s nowhere to put the skb_shinfo, and hence no frags >list), so the data pointer really ought to be valid (i.e. NULL is also >bad here). >OK, I need to eat some crow here. The patch is correct, but my description is completely wrong, and is based on false assumptions. I completely forgot that skb_shinfo hangs off the data area, and assumed that "no packet data in skb->data" was equivalent to "skb->data is NULL". Completely, totally, wrong. As you noted, skb->data has got to be valid for the skb to be useful at all. The use of "if (skb->data)" is completely incorrect in any context. I should know this -- I''ve stared at skbuff.h long enough over the years. In my defense, the patch went through two internal review cycles and nobody else noticed it either. So I feel a little better. It''s a use case that just doesn''t come up that often. Please keep the patch, but ignore (or change) the description. Sorry about the confusion. -Mitch _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel