Message ID | 1359135881.12374.3249.camel@edumazet-glaptop |
---|---|
State | Accepted, archived |
Delegated to: | David Miller |
Headers | show |
From: Eric Dumazet <eric.dumazet@gmail.com> Date: Fri, 25 Jan 2013 09:44:41 -0800 > From: Eric Dumazet <edumazet@google.com> > > Ben Greear reported crashes in ip_rcv_finish() on a stress > test involving many macvlans. > > We tracked the bug to a dst use after free. ip_rcv_finish() > was calling dst->input() and got garbage for dst->input value. > > It appears the bug is in loopback driver, lacking > a skb_dst_force() before calling netif_rx(). > > As a result, a non refcounted dst, normally protected by a > RCU read_lock section, was escaping this section and could > be freed before the packet being processed. ... > This bug was introduced in linux-2.6.35, in commit > 7fee226ad2397b (net: add a noref bit on skb dst) > > skb_dst_force() is enforced in dev_queue_xmit() for devices having a > qdisc. > > Reported-by: Ben Greear <greearb@candelatech.com> > Signed-off-by: Eric Dumazet <edumazet@google.com> > Tested-by: Ben Greear <greearb@candelatech.com> Applied, I suspect we need to tree wide audit for other instances of this situation. Probably greps for skb_orphan() would be a good start. -- 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 Sun, 2013-01-27 at 01:32 -0500, David Miller wrote: > Applied, I suspect we need to tree wide audit for other instances of > this situation. Probably greps for skb_orphan() would be a good > start. I highly suspect macvlan needs a skb_dst_drop(), but couldnt trigger a case to trigger the fault. -- 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
From: Eric Dumazet <eric.dumazet@gmail.com> Date: Sun, 27 Jan 2013 09:25:08 -0800 > On Sun, 2013-01-27 at 01:32 -0500, David Miller wrote: > >> Applied, I suspect we need to tree wide audit for other instances of >> this situation. Probably greps for skb_orphan() would be a good >> start. > > I highly suspect macvlan needs a skb_dst_drop(), but couldnt trigger > a case to trigger the fault. I completely agree, when we hit the ->forward() or ->receive() method we are leaving the RCU safety zone for the cached dst. Even if you can't trigger it, this seems clear enough to me that we should put those skb_dst_drop() calls in, so please formally submit that patch you posted for Ben as an RFC last week. Thanks. -- 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/loopback.c b/drivers/net/loopback.c index 81f8f9e..fcbf680 100644 --- a/drivers/net/loopback.c +++ b/drivers/net/loopback.c @@ -77,6 +77,11 @@ static netdev_tx_t loopback_xmit(struct sk_buff *skb, skb_orphan(skb); + /* Before queueing this packet to netif_rx(), + * make sure dst is refcounted. + */ + skb_dst_force(skb); + skb->protocol = eth_type_trans(skb, dev); /* it's OK to use per_cpu_ptr() because BHs are off */