ftrace: tracepoint of net_dev_xmit sees freed skb and causes panic

PATCH In review Koki Sanagi 4 participants 11 messages May 31, 2011 – Jun 3, 2011

Subsystems (3)

Networking Network drivers Tracing

lore

Share & tools

lore

Koki Sanagi

author

2011-05-31 07:48 UTC #0 lore raw

PATCH 1/1 ftrace: tracepoint of net_dev_xmit sees freed skb and causes panic

Because there is a possibility that skb is kfree_skb()ed and zero cleared after ndo_start_xmit, we should not see the contents of skb like skb->len and skb->dev->name after ndo_start_xmit. But trace_net_dev_xmit does that and causes panic by NULL pointer dereference.
This patch fixes trace_net_dev_xmit not to see the contents of skb directly.

If you want to reproduce this panic,

1. Get tracepoint of net_dev_xmit on
2. Create 2 guests on KVM
2. Make 2 guests use virtio_net
4. Execute netperf from one to another for a long time as a network burden
5. host will panic(It takes about 30 minutes)

Signed-off-by Koki Sanagi <[email protected]>

---
 include/trace/events/net.h |   12 +++++++-----
 net/core/dev.c             |    7 +++++--
 2 files changed, 12 insertions(+), 7 deletions(-)

include/trace/events/net.h +7 -5

index 5f247f5..f99645d 100644
@@ -12,22 +12,24 @@
 TRACE_EVENT(net_dev_xmit,
 
 	TP_PROTO(struct sk_buff *skb,
-		 int rc),
+		 int rc,
+		 struct net_device *dev,
+		 unsigned int skb_len),
 
-	TP_ARGS(skb, rc),
+	TP_ARGS(skb, rc, dev, skb_len),
 
 	TP_STRUCT__entry(
 		__field(	void *,		skbaddr		)
 		__field(	unsigned int,	len		)
 		__field(	int,		rc		)
-		__string(	name,		skb->dev->name	)
+		__string(	name,		dev->name	)
 	),
 
 	TP_fast_assign(
 		__entry->skbaddr = skb;
-		__entry->len = skb->len;
+		__entry->len = skb_len;
 		__entry->rc = rc;
-		__assign_str(name, skb->dev->name);
+		__assign_str(name, dev->name);
 	),
 
 	TP_printk("dev=%s skbaddr=%p len=%u rc=%d",

net/core/dev.c +5 -2

index d945379..f0e15df 100644
@@ -2089,6 +2089,7 @@ int dev_hard_start_xmit(struct sk_buff *skb, struct net_device *dev,
 {
 	const struct net_device_ops *ops = dev->netdev_ops;
 	int rc = NETDEV_TX_OK;
+	unsigned int skb_len;
 
 	if (likely(!skb->next)) {
 		u32 features;

@@ -2139,8 +2140,9 @@ int dev_hard_start_xmit(struct sk_buff *skb, struct net_device *dev,
 			}
 		}
 
+		skb_len = skb->len;
 		rc = ops->ndo_start_xmit(skb, dev);
-		trace_net_dev_xmit(skb, rc);
+		trace_net_dev_xmit(skb, rc, dev, skb_len);
 		if (rc == NETDEV_TX_OK)
 			txq_trans_update(txq);
 		return rc;

@@ -2160,8 +2162,9 @@ gso:
 		if (dev->priv_flags & IFF_XMIT_DST_RELEASE)
 			skb_dst_drop(nskb);
 
+		skb_len = nskb->len;
 		rc = ops->ndo_start_xmit(nskb, dev);
-		trace_net_dev_xmit(nskb, rc);
+		trace_net_dev_xmit(nskb, rc, dev, skb_len);
 		if (unlikely(rc != NETDEV_TX_OK)) {
 			if (rc & ~NETDEV_TX_MASK)
 				goto out_kfree_gso_skb;

Steven Rostedt

maintainer

2011-05-31 15:13 UTC #1 lore raw

Quoted 22 lines · Koki Sanagi

Because there is a possibility that skb is kfree_skb()ed and zero cleared after ndo_start_xmit, we should not see the contents of skb like skb->len and skb->dev->name after ndo_start_xmit. But trace_net_dev_xmit does that and causes panic by NULL pointer dereference.
This patch fixes trace_net_dev_xmit not to see the contents of skb directly.
net/core/dev.c +2 -1
 	if (likely(!skb->next)) {
 		u32 features;
@@ -2139,8 +2140,9 @@ int dev_hard_start_xmit(struct sk_buff *skb, struct net_device *dev,
 			}
 		}
 
+		skb_len = skb->len;
 		rc = ops->ndo_start_xmit(skb, dev);
-		trace_net_dev_xmit(skb, rc);
+		trace_net_dev_xmit(skb, rc, dev, skb_len);
 		if (rc == NETDEV_TX_OK)
 			txq_trans_update(txq);
 		return rc;

net/core/dev.c gso: · line 2167

Quoted 7 lines

net/core/dev.c +2 -1

@@ -2160,8 +2162,9 @@ gso:
 		if (dev->priv_flags & IFF_XMIT_DST_RELEASE)
 			skb_dst_drop(nskb);
 
+		skb_len = nskb->len;
 		rc = ops->ndo_start_xmit(nskb, dev);
-		trace_net_dev_xmit(nskb, rc);
+		trace_net_dev_xmit(nskb, rc, dev, skb_len);

What if you just put the tracepoint before the call to ops->ndo_start_xmit?

-- Steve

Quoted 3 lines

net/core/dev.c +0 -0

 		if (unlikely(rc != NETDEV_TX_OK)) {
 			if (rc & ~NETDEV_TX_MASK)
 				goto out_kfree_gso_skb;

Neil Horman

2011-05-31 16:11 UTC #2 lore raw

Quoted 26 lines · Steven Rostedt

Quoted 25 lines · Koki Sanagi
Because there is a possibility that skb is kfree_skb()ed and zero cleared after ndo_start_xmit, we should not see the contents of skb like skb->len and skb->dev->name after ndo_start_xmit. But trace_net_dev_xmit does that and causes panic by NULL pointer dereference.
This patch fixes trace_net_dev_xmit not to see the contents of skb directly.
net/core/dev.c +2 -1
 	if (likely(!skb->next)) {
 		u32 features;
@@ -2139,8 +2140,9 @@ int dev_hard_start_xmit(struct sk_buff *skb, struct net_device *dev,
 			}
 		}
 
+		skb_len = skb->len;
 		rc = ops->ndo_start_xmit(skb, dev);
-		trace_net_dev_xmit(skb, rc);
+		trace_net_dev_xmit(skb, rc, dev, skb_len);
 		if (rc == NETDEV_TX_OK)
 			txq_trans_update(txq);
 		return rc;
@@ -2160,8 +2162,9 @@ gso:
 		if (dev->priv_flags & IFF_XMIT_DST_RELEASE)
 			skb_dst_drop(nskb);
 

Quoted 8 lines

net/core/dev.c gso: · line 2167
Quoted 4 lines
net/core/dev.c +2 -1
+		skb_len = nskb->len;
 		rc = ops->ndo_start_xmit(nskb, dev);
-		trace_net_dev_xmit(nskb, rc);
+		trace_net_dev_xmit(nskb, rc, dev, skb_len);
What if you just put the tracepoint before the call to ops->ndo_start_xmit?

Then you won't know the return code of ndo_start_xmit, which this tracepoint records.
Neil

Quoted 8 lines

-- Steve

Quoted 3 lines

net/core/dev.c +0 -0

 		if (unlikely(rc != NETDEV_TX_OK)) {
 			if (rc & ~NETDEV_TX_MASK)
 				goto out_kfree_gso_skb;

Steven Rostedt

maintainer

2011-05-31 16:21 UTC #3 lore raw

Quoted 10 lines · Neil Horman

skb_dst_drop(nskb);
Quoted 9 lines
net/core/dev.c gso: · line 2167
Quoted 5 lines
net/core/dev.c +2 -1
+		skb_len = nskb->len;
 		rc = ops->ndo_start_xmit(nskb, dev);
-		trace_net_dev_xmit(nskb, rc);
+		trace_net_dev_xmit(nskb, rc, dev, skb_len);
What if you just put the tracepoint before the call to ops->ndo_start_xmit?

Quoted 2 lines

Then you won't know the return code of ndo_start_xmit, which this tracepoint records.

Doh! Yeah, I see that now ;)

-- Steve

Steven Rostedt

maintainer

2011-05-31 15:14 UTC #4 lore raw

In reply to Koki Sanagi

Note, the subject should not be "ftrace:", but "net:" or maybe even "net/tracing", as this really has nothing to do with ftrace code. The tracepoints are more generic than ftrace.

-- Steve

David Miller

maintainer

2011-06-02 20:59 UTC #5 lore raw

From: Steven Rostedt <[email protected]>
Date: Tue, 31 May 2011 11:14:27 -0400

Quoted 3 lines

Note, the subject should not be "ftrace:", but "net:" or maybe even "net/tracing", as this really has nothing to do with ftrace code. The tracepoints are more generic than ftrace.

Do you guys mind if I take this in via the net-2.6 tree?

Steven Rostedt

maintainer

2011-06-02 21:01 UTC #6 lore raw

Quoted 8 lines · David Miller

From: Steven Rostedt <[email protected]>
Date: Tue, 31 May 2011 11:14:27 -0400
Quoted 3 lines
Note, the subject should not be "ftrace:", but "net:" or maybe even "net/tracing", as this really has nothing to do with ftrace code. The tracepoints are more generic than ftrace.
Do you guys mind if I take this in via the net-2.6 tree?

I have no issue with that. Heck, I prefer it.

-- Steve

David Miller

maintainer

2011-06-02 21:04 UTC #7 lore raw

From: Steven Rostedt <[email protected]>
Date: Thu, 02 Jun 2011 17:01:52 -0400

Quoted 11 lines · David Miller

Quoted 8 lines
From: Steven Rostedt <[email protected]>
Date: Tue, 31 May 2011 11:14:27 -0400
Quoted 3 lines
Note, the subject should not be "ftrace:", but "net:" or maybe even "net/tracing", as this really has nothing to do with ftrace code. The tracepoints are more generic than ftrace.
Do you guys mind if I take this in via the net-2.6 tree?
I have no issue with that. Heck, I prefer it.

Great, thanks!

Koki Sanagi

author

2011-06-03 05:03 UTC #8 lore raw

Quoted 11 lines · David Miller

From: Steven Rostedt <[email protected]>
Date: Thu, 02 Jun 2011 17:01:52 -0400
Quoted 8 lines · David Miller
Quoted 7 lines
From: Steven Rostedt <[email protected]>
Date: Tue, 31 May 2011 11:14:27 -0400
Quoted 3 lines
Note, the subject should not be "ftrace:", but "net:" or maybe even "net/tracing", as this really has nothing to do with ftrace code. The tracepoints are more generic than ftrace.

Quoted 5 lines

Quoted 3 lines
Quoted 1 line
Do you guys mind if I take this in via the net-2.6 tree?
I have no issue with that. Heck, I prefer it.
Great, thanks!

O.K.
I will resubmit this patch with appropriate subject "net/tracing:"

Thanks,
Koki Sanagi

David Miller

maintainer

2011-06-03 05:23 UTC #9 lore raw

From: Koki Sanagi <[email protected]>
Date: Fri, 03 Jun 2011 14:03:51 +0900

Quoted 1 line

I will resubmit this patch with appropriate subject "net/tracing:"

You don't need to, I fixed it up when I applied your patch.

Koki Sanagi

author

2011-06-03 05:57 UTC #10 lore raw

Quoted 7 lines · David Miller

From: Koki Sanagi <[email protected]>
Date: Fri, 03 Jun 2011 14:03:51 +0900
Quoted 1 line
I will resubmit this patch with appropriate subject "net/tracing:"
You don't need to, I fixed it up when I applied your patch.

O.K.

Thanks!
Koki Sanagi