Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20221107090940.686229-1-nashuiliang@gmail.com> <20221109175908.593df5da@kernel.org>
In-Reply-To: <20221109175908.593df5da@kernel.org>
From:   Chuang W <nashuiliang@gmail.com>
Date:   Thu, 10 Nov 2022 10:12:15 +0800
Message-ID: <CACueBy4U-MBZx0JwTc0Z5G2iFjBu4wPtdW+bs0aZOr9_QQFv2w@mail.gmail.com>
Subject: Re: [PATCH v1] net: tun: rebuild error handling in tun_get_user
To:     Jakub Kicinski <kuba@kernel.org>
Cc:     "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Paolo Abeni <pabeni@redhat.com>, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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