| Message ID | 20190207202738.155940-1-edumazet@google.com |
|---|---|
| State | Accepted |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net] vxlan: test dev->flags & IFF_UP before calling netif_rx() | expand |
On Thu, Feb 7, 2019 at 12:27 PM Eric Dumazet <edumazet@google.com> wrote: > > netif_rx() must be called under a strict contract. > > At device dismantle phase, core networking clears IFF_UP > and flush_all_backlogs() is called after rcu grace period > to make sure no incoming packet might be in a cpu backlog > and still referencing the device. > > Most drivers call netif_rx() from their interrupt handler, > and since the interrupts are disabled at device dismantle, > netif_rx() does not have to check dev->flags & IFF_UP > > Virtual drivers do not have this guarantee, and must > therefore make the check themselves. Since other netif_rx() callers seem to have the same issue, I wonder if we simply could add a generic check. Most high performance drivers no longer call netif_rx() so an extra check in it wont be a problem.
From: Eric Dumazet <edumazet@google.com> Date: Thu, 7 Feb 2019 12:27:38 -0800 > netif_rx() must be called under a strict contract. > > At device dismantle phase, core networking clears IFF_UP > and flush_all_backlogs() is called after rcu grace period > to make sure no incoming packet might be in a cpu backlog > and still referencing the device. > > Most drivers call netif_rx() from their interrupt handler, > and since the interrupts are disabled at device dismantle, > netif_rx() does not have to check dev->flags & IFF_UP > > Virtual drivers do not have this guarantee, and must > therefore make the check themselves. > > Otherwise we risk use-after-free and/or crashes. > > Note this patch also fixes a small issue that came > with commit ce6502a8f957 ("vxlan: fix a use after free > in vxlan_encap_bypass"), since the dev->stats.rx_dropped > change was done on the wrong device. > > Fixes: d342894c5d2f ("vxlan: virtual extensible lan") > Fixes: ce6502a8f957 ("vxlan: fix a use after free in vxlan_encap_bypass") > Signed-off-by: Eric Dumazet <edumazet@google.com> Applied and queued up for -stable.
diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c index 5209ee9aac47846367d7f469a7e69d08c030087e..2aae11feff0c22c1b073a6a119fc6c311cbe1691 100644 --- a/drivers/net/vxlan.c +++ b/drivers/net/vxlan.c @@ -2219,7 +2219,7 @@ static void vxlan_encap_bypass(struct sk_buff *skb, struct vxlan_dev *src_vxlan, struct pcpu_sw_netstats *tx_stats, *rx_stats; union vxlan_addr loopback; union vxlan_addr *remote_ip = &dst_vxlan->default_dst.remote_ip; - struct net_device *dev = skb->dev; + struct net_device *dev; int len = skb->len; tx_stats = this_cpu_ptr(src_vxlan->dev->tstats); @@ -2239,9 +2239,15 @@ static void vxlan_encap_bypass(struct sk_buff *skb, struct vxlan_dev *src_vxlan, #endif } + rcu_read_lock(); + dev = skb->dev; + if (unlikely(!(dev->flags & IFF_UP))) { + kfree_skb(skb); + goto drop; + } + if (dst_vxlan->cfg.flags & VXLAN_F_LEARN) - vxlan_snoop(skb->dev, &loopback, eth_hdr(skb)->h_source, 0, - vni); + vxlan_snoop(dev, &loopback, eth_hdr(skb)->h_source, 0, vni); u64_stats_update_begin(&tx_stats->syncp); tx_stats->tx_packets++; @@ -2254,8 +2260,10 @@ static void vxlan_encap_bypass(struct sk_buff *skb, struct vxlan_dev *src_vxlan, rx_stats->rx_bytes += len; u64_stats_update_end(&rx_stats->syncp); } else { +drop: dev->stats.rx_dropped++; } + rcu_read_unlock(); } static int encap_bypass_if_local(struct sk_buff *skb, struct net_device *dev,
netif_rx() must be called under a strict contract. At device dismantle phase, core networking clears IFF_UP and flush_all_backlogs() is called after rcu grace period to make sure no incoming packet might be in a cpu backlog and still referencing the device. Most drivers call netif_rx() from their interrupt handler, and since the interrupts are disabled at device dismantle, netif_rx() does not have to check dev->flags & IFF_UP Virtual drivers do not have this guarantee, and must therefore make the check themselves. Otherwise we risk use-after-free and/or crashes. Note this patch also fixes a small issue that came with commit ce6502a8f957 ("vxlan: fix a use after free in vxlan_encap_bypass"), since the dev->stats.rx_dropped change was done on the wrong device. Fixes: d342894c5d2f ("vxlan: virtual extensible lan") Fixes: ce6502a8f957 ("vxlan: fix a use after free in vxlan_encap_bypass") Signed-off-by: Eric Dumazet <edumazet@google.com> Cc: Petr Machata <petrm@mellanox.com> Cc: Ido Schimmel <idosch@mellanox.com> Cc: Roopa Prabhu <roopa@cumulusnetworks.com> Cc: Stefano Brivio <sbrivio@redhat.com> --- drivers/net/vxlan.c | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-)