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;
Date:   Wed, 9 Nov 2022 17:59:20 -0800
From:   Jakub Kicinski <kuba@kernel.org>
To:     Chuang Wang <nashuiliang@gmail.com>
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
Subject: Re: [PATCH v1] net: tun: rebuild error handling in tun_get_user
Message-ID: <20221109175908.593df5da@kernel.org>
In-Reply-To: <20221107090940.686229-1-nashuiliang@gmail.com>
References: <20221107090940.686229-1-nashuiliang@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

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)