diff mbox series

[v1] net: tun: rebuild error handling in tun_get_user

Message ID 20221107090940.686229-1-nashuiliang@gmail.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series [v1] net: tun: rebuild error handling in tun_get_user | expand

Checks

Context Check Description
netdev/tree_selection success Guessed tree name to be net-next
netdev/fixes_present success Fixes tag not required for -next series
netdev/subject_prefix warning Target tree name not specified in the subject
netdev/cover_letter success Single patches do not need cover letters
netdev/patch_count success Link
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/cc_maintainers success CCed 5 of 5 maintainers
netdev/build_clang success Errors and warnings before: 0 this patch: 0
netdev/module_param success Was 0 now: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 0 this patch: 0
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 127 lines checked
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Chuang Wang Nov. 7, 2022, 9:09 a.m. UTC 
The error handling in tun_get_user is very scattered.
This patch unifies error handling, reduces duplication of code, and
makes the logic clearer.

Signed-off-by: Chuang Wang <nashuiliang@gmail.com>
---
v0 -> v1:
- rename tags

 drivers/net/tun.c | 68 ++++++++++++++++++++++-------------------------
 1 file changed, 32 insertions(+), 36 deletions(-)

Comments

Jakub Kicinski Nov. 10, 2022, 1:59 a.m. UTC | #1 
On Mon,  7 Nov 2022 17:09:40 +0800 Chuang Wang wrote:
> The error handling in tun_get_user is very scattered.
> This patch unifies error handling, reduces duplication of code, and
> makes the logic clearer.

You're also making some functional changes tho, they at the very least
need to be enumerated or preferably separate patches.

> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> index 4bf2b268df4a..5ceec73baf98 100644
> --- a/drivers/net/tun.c
> +++ b/drivers/net/tun.c
> @@ -1742,11 +1742,11 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
>  	int good_linear;
>  	int copylen;
>  	bool zerocopy = false;
> -	int err;
> +	int err = 0;

Don't zero-init the variables like this, instead...

>  	u32 rxhash = 0;
>  	int skb_xdp = 1;
>  	bool frags = tun_napi_frags_enabled(tfile);
> -	enum skb_drop_reason drop_reason;
> +	enum skb_drop_reason drop_reason = SKB_DROP_REASON_NOT_SPECIFIED;
>  
>  	if (!(tun->flags & IFF_NO_PI)) {
>  		if (len < sizeof(pi))
> @@ -1808,11 +1808,11 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
>  		 */
>  		skb = tun_build_skb(tun, tfile, from, &gso, len, &skb_xdp);

... use

	err = PTR_ERR_OR_ZERO(skb);

close to the jumps. It's safer to always init err before jumping.

>  		if (IS_ERR(skb)) {
> -			dev_core_stats_rx_dropped_inc(tun->dev);
> -			return PTR_ERR(skb);
> +			err = PTR_ERR(skb);
> +			goto drop;
>  		}
>  		if (!skb)
> -			return total_len;
> +			goto out;

>  	if (virtio_net_hdr_to_skb(skb, &gso, tun_is_little_endian(tun))) {
>  		atomic_long_inc(&tun->rx_frame_errors);
> -		kfree_skb(skb);

now we'll increment error and drop counters, that's not right.

> -		if (frags) {
> -			tfile->napi.skb = NULL;
> -			mutex_unlock(&tfile->napi_mutex);
> -		}
> -
> -		return -EINVAL;
> +		err = -EINVAL;
> +		goto drop;
>  	}

> @@ -1952,8 +1932,8 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
>  
>  	rcu_read_lock();
>  	if (unlikely(!(tun->dev->flags & IFF_UP))) {
> -		err = -EIO;
>  		rcu_read_unlock();
> +		err = -EIO;

this change is unnecessary, please refrain from making it

>  		drop_reason = SKB_DROP_REASON_DEV_READY;
>  		goto drop;
>  	}
> @@ -2007,7 +1987,23 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
>  	if (rxhash)
>  		tun_flow_update(tun, rxhash, tfile);
>  
> -	return total_len;
> +	goto out;

keep

	return total_len;

that's much easier to read, and there's no concern of err being
uninitialized.

> +
> +drop:
> +	if (err != -EAGAIN)
> +		dev_core_stats_rx_dropped_inc(tun->dev);
> +
> +	if (!IS_ERR_OR_NULL(skb))
> +		kfree_skb_reason(skb, drop_reason);
> +
> +unlock_frags:
> +	if (frags) {
> +		tfile->napi.skb = NULL;
> +		mutex_unlock(&tfile->napi_mutex);
> +	}
> +
> +out:
> +	return err ?: total_len;
>  }
>  
>  static ssize_t tun_chr_write_iter(struct kiocb *iocb, struct iov_iter *from)
Chuang Wang Nov. 10, 2022, 2:12 a.m. UTC | #2 
Thanks, I will fix these issues and submit again.

On Thu, Nov 10, 2022 at 9:59 AM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Mon,  7 Nov 2022 17:09:40 +0800 Chuang Wang wrote:
> > The error handling in tun_get_user is very scattered.
> > This patch unifies error handling, reduces duplication of code, and
> > makes the logic clearer.
>
> You're also making some functional changes tho, they at the very least
> need to be enumerated or preferably separate patches.
>
> > diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> > index 4bf2b268df4a..5ceec73baf98 100644
> > --- a/drivers/net/tun.c
> > +++ b/drivers/net/tun.c
> > @@ -1742,11 +1742,11 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
> >       int good_linear;
> >       int copylen;
> >       bool zerocopy = false;
> > -     int err;
> > +     int err = 0;
>
> Don't zero-init the variables like this, instead...
>
> >       u32 rxhash = 0;
> >       int skb_xdp = 1;
> >       bool frags = tun_napi_frags_enabled(tfile);
> > -     enum skb_drop_reason drop_reason;
> > +     enum skb_drop_reason drop_reason = SKB_DROP_REASON_NOT_SPECIFIED;
> >
> >       if (!(tun->flags & IFF_NO_PI)) {
> >               if (len < sizeof(pi))
> > @@ -1808,11 +1808,11 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
> >                */
> >               skb = tun_build_skb(tun, tfile, from, &gso, len, &skb_xdp);
>
> ... use
>
>         err = PTR_ERR_OR_ZERO(skb);
>
> close to the jumps. It's safer to always init err before jumping.
>
> >               if (IS_ERR(skb)) {
> > -                     dev_core_stats_rx_dropped_inc(tun->dev);
> > -                     return PTR_ERR(skb);
> > +                     err = PTR_ERR(skb);
> > +                     goto drop;
> >               }
> >               if (!skb)
> > -                     return total_len;
> > +                     goto out;
>
> >       if (virtio_net_hdr_to_skb(skb, &gso, tun_is_little_endian(tun))) {
> >               atomic_long_inc(&tun->rx_frame_errors);
> > -             kfree_skb(skb);
>
> now we'll increment error and drop counters, that's not right.
>
> > -             if (frags) {
> > -                     tfile->napi.skb = NULL;
> > -                     mutex_unlock(&tfile->napi_mutex);
> > -             }
> > -
> > -             return -EINVAL;
> > +             err = -EINVAL;
> > +             goto drop;
> >       }
>
> > @@ -1952,8 +1932,8 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
> >
> >       rcu_read_lock();
> >       if (unlikely(!(tun->dev->flags & IFF_UP))) {
> > -             err = -EIO;
> >               rcu_read_unlock();
> > +             err = -EIO;
>
> this change is unnecessary, please refrain from making it
>
> >               drop_reason = SKB_DROP_REASON_DEV_READY;
> >               goto drop;
> >       }
> > @@ -2007,7 +1987,23 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
> >       if (rxhash)
> >               tun_flow_update(tun, rxhash, tfile);
> >
> > -     return total_len;
> > +     goto out;
>
> keep
>
>         return total_len;
>
> that's much easier to read, and there's no concern of err being
> uninitialized.
>
> > +
> > +drop:
> > +     if (err != -EAGAIN)
> > +             dev_core_stats_rx_dropped_inc(tun->dev);
> > +
> > +     if (!IS_ERR_OR_NULL(skb))
> > +             kfree_skb_reason(skb, drop_reason);
> > +
> > +unlock_frags:
> > +     if (frags) {
> > +             tfile->napi.skb = NULL;
> > +             mutex_unlock(&tfile->napi_mutex);
> > +     }
> > +
> > +out:
> > +     return err ?: total_len;
> >  }
> >
> >  static ssize_t tun_chr_write_iter(struct kiocb *iocb, struct iov_iter *from)
>
diff mbox series

Patch

diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index 4bf2b268df4a..5ceec73baf98 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -1742,11 +1742,11 @@  static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
 	int good_linear;
 	int copylen;
 	bool zerocopy = false;
-	int err;
+	int err = 0;
 	u32 rxhash = 0;
 	int skb_xdp = 1;
 	bool frags = tun_napi_frags_enabled(tfile);
-	enum skb_drop_reason drop_reason;
+	enum skb_drop_reason drop_reason = SKB_DROP_REASON_NOT_SPECIFIED;
 
 	if (!(tun->flags & IFF_NO_PI)) {
 		if (len < sizeof(pi))
@@ -1808,11 +1808,11 @@  static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
 		 */
 		skb = tun_build_skb(tun, tfile, from, &gso, len, &skb_xdp);
 		if (IS_ERR(skb)) {
-			dev_core_stats_rx_dropped_inc(tun->dev);
-			return PTR_ERR(skb);
+			err = PTR_ERR(skb);
+			goto drop;
 		}
 		if (!skb)
-			return total_len;
+			goto out;
 	} else {
 		if (!zerocopy) {
 			copylen = len;
@@ -1836,11 +1836,8 @@  static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
 		}
 
 		if (IS_ERR(skb)) {
-			if (PTR_ERR(skb) != -EAGAIN)
-				dev_core_stats_rx_dropped_inc(tun->dev);
-			if (frags)
-				mutex_unlock(&tfile->napi_mutex);
-			return PTR_ERR(skb);
+			err = PTR_ERR(skb);
+			goto drop;
 		}
 
 		if (zerocopy)
@@ -1851,27 +1848,14 @@  static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
 		if (err) {
 			err = -EFAULT;
 			drop_reason = SKB_DROP_REASON_SKB_UCOPY_FAULT;
-drop:
-			dev_core_stats_rx_dropped_inc(tun->dev);
-			kfree_skb_reason(skb, drop_reason);
-			if (frags) {
-				tfile->napi.skb = NULL;
-				mutex_unlock(&tfile->napi_mutex);
-			}
-
-			return err;
+			goto drop;
 		}
 	}
 
 	if (virtio_net_hdr_to_skb(skb, &gso, tun_is_little_endian(tun))) {
 		atomic_long_inc(&tun->rx_frame_errors);
-		kfree_skb(skb);
-		if (frags) {
-			tfile->napi.skb = NULL;
-			mutex_unlock(&tfile->napi_mutex);
-		}
-
-		return -EINVAL;
+		err = -EINVAL;
+		goto drop;
 	}
 
 	switch (tun->flags & TUN_TYPE_MASK) {
@@ -1887,9 +1871,9 @@  static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
 				pi.proto = htons(ETH_P_IPV6);
 				break;
 			default:
-				dev_core_stats_rx_dropped_inc(tun->dev);
-				kfree_skb(skb);
-				return -EINVAL;
+				err = -EINVAL;
+				drop_reason = SKB_DROP_REASON_INVALID_PROTO;
+				goto drop;
 			}
 		}
 
@@ -1931,11 +1915,7 @@  static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
 			if (ret != XDP_PASS) {
 				rcu_read_unlock();
 				local_bh_enable();
-				if (frags) {
-					tfile->napi.skb = NULL;
-					mutex_unlock(&tfile->napi_mutex);
-				}
-				return total_len;
+				goto unlock_frags;
 			}
 		}
 		rcu_read_unlock();
@@ -1952,8 +1932,8 @@  static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
 
 	rcu_read_lock();
 	if (unlikely(!(tun->dev->flags & IFF_UP))) {
-		err = -EIO;
 		rcu_read_unlock();
+		err = -EIO;
 		drop_reason = SKB_DROP_REASON_DEV_READY;
 		goto drop;
 	}
@@ -2007,7 +1987,23 @@  static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
 	if (rxhash)
 		tun_flow_update(tun, rxhash, tfile);
 
-	return total_len;
+	goto out;
+
+drop:
+	if (err != -EAGAIN)
+		dev_core_stats_rx_dropped_inc(tun->dev);
+
+	if (!IS_ERR_OR_NULL(skb))
+		kfree_skb_reason(skb, drop_reason);
+
+unlock_frags:
+	if (frags) {
+		tfile->napi.skb = NULL;
+		mutex_unlock(&tfile->napi_mutex);
+	}
+
+out:
+	return err ?: total_len;
 }
 
 static ssize_t tun_chr_write_iter(struct kiocb *iocb, struct iov_iter *from)