[PATCH net] tun: properly test for IFF_UP

[PATCH net] tun: properly test for IFF_UP

[Eric Dumazet]

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;
+		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);
-- 
2.21.0.360.g471c308f928-goog

Re: [PATCH net] tun: properly test for IFF_UP

[David Miller]

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.

Re: [PATCH net] tun: properly test for IFF_UP

[Eric Dumazet]

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);
>