| Message ID | 20190315031947.63245-1-edumazet@google.com |
|---|---|
| State | Accepted |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net] tun: properly test for IFF_UP | expand |
From: Eric Dumazet <edumazet@google.com> Date: Thu, 14 Mar 2019 20:19:47 -0700 > Same reasons than the ones explained in commit 4179cb5a4c92 > ("vxlan: test dev->flags & IFF_UP before calling netif_rx()") > > netif_rx_ni() or napi_gro_frags() 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. > > A similar protocol is used for gro layer. > > 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. > > Fixes: 1bd4978a88ac ("tun: honor IFF_UP in tun_get_user()") > Signed-off-by: Eric Dumazet <edumazet@google.com> > Reported-by: syzbot <syzkaller@googlegroups.com> Applied and queued up for -stable, thanks Eric.
On 03/14/2019 08:19 PM, Eric Dumazet wrote: > Same reasons than the ones explained in commit 4179cb5a4c92 > ("vxlan: test dev->flags & IFF_UP before calling netif_rx()") > > netif_rx_ni() or napi_gro_frags() 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. > > A similar protocol is used for gro layer. > > 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. > > Fixes: 1bd4978a88ac ("tun: honor IFF_UP in tun_get_user()") > Signed-off-by: Eric Dumazet <edumazet@google.com> > Reported-by: syzbot <syzkaller@googlegroups.com> > --- > drivers/net/tun.c | 15 +++++++++++---- > 1 file changed, 11 insertions(+), 4 deletions(-) > > diff --git a/drivers/net/tun.c b/drivers/net/tun.c > index 1d68921723dc08532b3f5321a52865076ad66336..0d343359f647ff58fee35462358827e61857c837 100644 > --- a/drivers/net/tun.c > +++ b/drivers/net/tun.c > @@ -1763,9 +1763,6 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile, > int skb_xdp = 1; > bool frags = tun_napi_frags_enabled(tfile); > > - if (!(tun->dev->flags & IFF_UP)) > - return -EIO; > - > if (!(tun->flags & IFF_NO_PI)) { > if (len < sizeof(pi)) > return -EINVAL; > @@ -1867,6 +1864,8 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile, > err = skb_copy_datagram_from_iter(skb, 0, from, len); > > if (err) { > + err = -EFAULT; > +drop: > this_cpu_inc(tun->pcpu_stats->rx_dropped); > kfree_skb(skb); > if (frags) { > @@ -1874,7 +1873,7 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile, > mutex_unlock(&tfile->napi_mutex); > } > > - return -EFAULT; > + return err; > } > } > > @@ -1958,6 +1957,12 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile, > !tfile->detached) > rxhash = __skb_get_hash_symmetric(skb); > > + rcu_read_lock(); > + if (unlikely(!(tun->dev->flags & IFF_UP))) { > + err = -EIO; Hmm, it looks like I forgot a rcu_read_unlock() here. > + goto drop; > + } > + > if (frags) { > /* Exercise flow dissector code path. */ > u32 headlen = eth_get_headlen(skb->data, skb_headlen(skb)); > @@ -1965,6 +1970,7 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile, > if (unlikely(headlen > skb_headlen(skb))) { > this_cpu_inc(tun->pcpu_stats->rx_dropped); > napi_free_frags(&tfile->napi); > + rcu_read_unlock(); > mutex_unlock(&tfile->napi_mutex); > WARN_ON(1); > return -ENOMEM; > @@ -1992,6 +1998,7 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile, > } else { > netif_rx_ni(skb); > } > + rcu_read_unlock(); > > stats = get_cpu_ptr(tun->pcpu_stats); > u64_stats_update_begin(&stats->syncp); >
diff --git a/drivers/net/tun.c b/drivers/net/tun.c index 1d68921723dc08532b3f5321a52865076ad66336..0d343359f647ff58fee35462358827e61857c837 100644 --- a/drivers/net/tun.c +++ b/drivers/net/tun.c @@ -1763,9 +1763,6 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile, int skb_xdp = 1; bool frags = tun_napi_frags_enabled(tfile); - if (!(tun->dev->flags & IFF_UP)) - return -EIO; - if (!(tun->flags & IFF_NO_PI)) { if (len < sizeof(pi)) return -EINVAL; @@ -1867,6 +1864,8 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile, err = skb_copy_datagram_from_iter(skb, 0, from, len); if (err) { + err = -EFAULT; +drop: this_cpu_inc(tun->pcpu_stats->rx_dropped); kfree_skb(skb); if (frags) { @@ -1874,7 +1873,7 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile, mutex_unlock(&tfile->napi_mutex); } - return -EFAULT; + return err; } } @@ -1958,6 +1957,12 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile, !tfile->detached) rxhash = __skb_get_hash_symmetric(skb); + rcu_read_lock(); + if (unlikely(!(tun->dev->flags & IFF_UP))) { + err = -EIO; + goto drop; + } + if (frags) { /* Exercise flow dissector code path. */ u32 headlen = eth_get_headlen(skb->data, skb_headlen(skb)); @@ -1965,6 +1970,7 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile, if (unlikely(headlen > skb_headlen(skb))) { this_cpu_inc(tun->pcpu_stats->rx_dropped); napi_free_frags(&tfile->napi); + rcu_read_unlock(); mutex_unlock(&tfile->napi_mutex); WARN_ON(1); return -ENOMEM; @@ -1992,6 +1998,7 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile, } else { netif_rx_ni(skb); } + rcu_read_unlock(); stats = get_cpu_ptr(tun->pcpu_stats); u64_stats_update_begin(&stats->syncp);
Same reasons than the ones explained in commit 4179cb5a4c92 ("vxlan: test dev->flags & IFF_UP before calling netif_rx()") netif_rx_ni() or napi_gro_frags() 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. A similar protocol is used for gro layer. 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. Fixes: 1bd4978a88ac ("tun: honor IFF_UP in tun_get_user()") Signed-off-by: Eric Dumazet <edumazet@google.com> Reported-by: syzbot <syzkaller@googlegroups.com> --- drivers/net/tun.c | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-)