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 <[email protected]>
---
v0 -> v1:
- rename tags
drivers/net/tun.c | 68 ++++++++++++++++++++++-------------------------
1 file changed, 32 insertions(+), 36 deletions(-)
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)
--
2.37.2
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)
Thanks, I will fix these issues and submit again.
On Thu, Nov 10, 2022 at 9:59 AM Jakub Kicinski <[email protected]> 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)
>