| Message ID | AANLkTim1YZd0FgunGKN_YPiLW+uExXajwingUkQeW=a=@mail.gmail.com |
|---|---|
| State | RFC, archived |
| Delegated to: | David Miller |
| Headers | show |
Le lundi 20 décembre 2010 à 16:56 +0800, Changli Gao a écrit : > 2010/12/20 Paweł Staszewski <pstaszewski@itcare.pl>: > >> > > Yes reverting this patch solves the problem. > > > > > > I am not sure reverting is the right fix. Maybe you can try this patch > attached instead. > > BTW: there are some others NIC drivers and net_sched actions don't > take care of shared skbs. > This patch cant be right as is. if (skb_is_gso(skb)) { + if (skb_shared(skb)) { + struct sk_buff *nskb; + + nskb = skb_clone(skb, GFP_ATOMIC); + if (!nskb) + return -ENOMEM; + kfree_skb(skb); // HERE + skb = nskb; + } You free original skb, while caller might dereference it later. So you must pass cloned skb back to caller. (line 6393 :) count = ixgbe_tx_map(adapter, tx_ring, skb, tx_flags, first); -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Dec 20, 2010 at 5:08 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote: > Le lundi 20 décembre 2010 à 16:56 +0800, Changli Gao a écrit : > > This patch cant be right as is. > > if (skb_is_gso(skb)) { > + if (skb_shared(skb)) { > + struct sk_buff *nskb; > + > + nskb = skb_clone(skb, GFP_ATOMIC); > + if (!nskb) > + return -ENOMEM; > + kfree_skb(skb); // HERE > + skb = nskb; > + } > > You free original skb, while caller might dereference it later. > So you must pass cloned skb back to caller. > > (line 6393 :) > count = ixgbe_tx_map(adapter, tx_ring, skb, tx_flags, first); > > Yes, you are right. I just wonder where shared skbs are allowed. Is there any rule?
Le lundi 20 décembre 2010 à 17:11 +0800, Changli Gao a écrit : > On Mon, Dec 20, 2010 at 5:08 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote: > > Le lundi 20 décembre 2010 à 16:56 +0800, Changli Gao a écrit : > > > > This patch cant be right as is. > > > > if (skb_is_gso(skb)) { > > + if (skb_shared(skb)) { > > + struct sk_buff *nskb; > > + > > + nskb = skb_clone(skb, GFP_ATOMIC); > > + if (!nskb) > > + return -ENOMEM; > > + kfree_skb(skb); // HERE > > + skb = nskb; > > + } > > > > You free original skb, while caller might dereference it later. > > So you must pass cloned skb back to caller. > > > > (line 6393 :) > > count = ixgbe_tx_map(adapter, tx_ring, skb, tx_flags, first); > > > > > > Yes, you are right. I just wonder where shared skbs are allowed. Is > there any rule? > Shared skbs are allowed for sure (pktgen is a provider of such skbs). -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Dec 20, 2010 at 10:21:25AM +0100, Eric Dumazet wrote: > Le lundi 20 décembre 2010 ?? 17:11 +0800, Changli Gao a écrit : > > Yes, you are right. I just wonder where shared skbs are allowed. Is > > there any rule? > > > > Shared skbs are allowed for sure (pktgen is a provider of such skbs). But pktgen doesn't use common dev_queue_xmit() path. It looks like some places checking segmentation call pskb_expand_head() assuming skb isn't shared. Btw, it seems ifb should have GSO features similarly to loopback. Anyway, until all this is verified, IMHO we should revert Changli's mirred patch in stable. Jarek P. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Le lundi 20 décembre 2010 à 10:32 +0000, Jarek Poplawski a écrit : > On Mon, Dec 20, 2010 at 10:21:25AM +0100, Eric Dumazet wrote: > > Le lundi 20 décembre 2010 ?? 17:11 +0800, Changli Gao a écrit : > > > Yes, you are right. I just wonder where shared skbs are allowed. Is > > > there any rule? > > > > > > > Shared skbs are allowed for sure (pktgen is a provider of such skbs). > > But pktgen doesn't use common dev_queue_xmit() path. It looks like > some places checking segmentation call pskb_expand_head() assuming skb > isn't shared. Btw, it seems ifb should have GSO features similarly to > loopback. Anyway, until all this is verified, IMHO we should revert > Changli's mirred patch in stable. > I thought Changli question was : is a skb given to a device ndo_start_xmit() handler is allowed to be shared ;) I believe Changli patch should be reverted, because not doing a skb_clone() should be known by caller. To avoid the skb_clone(), we must tell caller what we did, so that caller dont even try to reuse skb (even freeing it) -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Dec 20, 2010 at 11:41:11AM +0100, Eric Dumazet wrote: > Le lundi 20 décembre 2010 ?? 10:32 +0000, Jarek Poplawski a écrit : > > On Mon, Dec 20, 2010 at 10:21:25AM +0100, Eric Dumazet wrote: > > > Le lundi 20 décembre 2010 ?? 17:11 +0800, Changli Gao a écrit : > > > > Yes, you are right. I just wonder where shared skbs are allowed. Is > > > > there any rule? > > > > > > > > > > Shared skbs are allowed for sure (pktgen is a provider of such skbs). > > > > But pktgen doesn't use common dev_queue_xmit() path. It looks like > > some places checking segmentation call pskb_expand_head() assuming skb > > isn't shared. Btw, it seems ifb should have GSO features similarly to > > loopback. Anyway, until all this is verified, IMHO we should revert > > Changli's mirred patch in stable. > > > > I thought Changli question was : is a skb given to a device > ndo_start_xmit() handler is allowed to be shared ;) > > I believe Changli patch should be reverted, because not doing a > skb_clone() should be known by caller. > > To avoid the skb_clone(), we must tell caller what we did, so that > caller dont even try to reuse skb (even freeing it) To tell the truth, Changli could assume we don't need to tell anything, because this mirred skb is almost not shared (except the refcount ;-). Jarek P. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Dec 20, 2010 at 11:11:41AM +0000, Jarek Poplawski wrote: > To tell the truth, Changli could assume we don't need to tell anything, > because this mirred skb is almost not shared (except the refcount ;-). Btw, since there was ifb involved, I doubt this skb could hit ixgbe xmit before unsharing, and patching the driver could matter. Anyway, Changli, I guess, Pawel needs some instructions on this last patch? Jarek P. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/net/ixgbe/ixgbe_main.c b/drivers/net/ixgbe/ixgbe_main.c index ca9036d..602cd32 100644 --- a/drivers/net/ixgbe/ixgbe_main.c +++ b/drivers/net/ixgbe/ixgbe_main.c @@ -6096,6 +6096,15 @@ static int ixgbe_tso(struct ixgbe_adapter *adapter, u32 mss_l4len_idx, l4len; if (skb_is_gso(skb)) { + if (skb_shared(skb)) { + struct sk_buff *nskb; + + nskb = skb_clone(skb, GFP_ATOMIC); + if (!nskb) + return -ENOMEM; + kfree_skb(skb); + skb = nskb; + } if (skb_header_cloned(skb)) { err = pskb_expand_head(skb, 0, 0, GFP_ATOMIC); if (err)