From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-pl1-f177.google.com (mail-pl1-f177.google.com [209.85.214.177]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 2A713207A2C for ; Fri, 10 Jan 2025 08:28:46 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.214.177 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1736497727; cv=none; b=NbMV+plPzSXq6YjIG+k+ne8Dm6r6/973MMX69yRASPZ18qd9L7pvuFwXq4hf/HjP+9oULKobysBdbBJIf2BxQfSmN+fYdcMzWUky0rg/Vy0uCXktufSHTuAfk0NS5Kzw3+iTBzznBAMht7L7ETWkKgp9bAeYMtcgNscxNXUzHr4= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1736497727; c=relaxed/simple; bh=mpE7hYntObRlHLj2s8ybBavAi+cZh37RwpDamWOgN40=; h=Message-ID:Date:MIME-Version:Subject:To:References:From: In-Reply-To:Content-Type; b=IV0n7ND/nXw3GRlHAQMpi/0lDXEzSx/u5J5JDXoLtldfScqADJ6fWAql0wr59PESn+sVvq4G+YVf4RQfFPCzK3yohpkkozCjNQg1em9vQgtlEm160Fd8rYA24bbXJcijoNaN7wRIDSf1sPSM+lAnEF4pm3Ug6CWS/8dzCPUvubE= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=daynix.com; spf=pass smtp.mailfrom=daynix.com; dkim=pass (2048-bit key) header.d=daynix-com.20230601.gappssmtp.com header.i=@daynix-com.20230601.gappssmtp.com header.b=ibSmOIFU; arc=none smtp.client-ip=209.85.214.177 Authentication-Results: smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=daynix.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=daynix.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=daynix-com.20230601.gappssmtp.com header.i=@daynix-com.20230601.gappssmtp.com header.b="ibSmOIFU" Received: by mail-pl1-f177.google.com with SMTP id d9443c01a7336-216728b1836so29094585ad.0 for ; Fri, 10 Jan 2025 00:28:46 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=daynix-com.20230601.gappssmtp.com; s=20230601; t=1736497725; x=1737102525; darn=vger.kernel.org; h=content-transfer-encoding:in-reply-to:from:content-language :references:to:subject:user-agent:mime-version:date:message-id:from :to:cc:subject:date:message-id:reply-to; bh=fMHAnr47gYJmPF4Dg6UKLdQQ9r7gUgpmT9CNdLeTBRg=; b=ibSmOIFU95uLATydrlyBKwidi+SiBBJLTbHROLEupNuyUdhtsPo3JPxU1iBTcry3Fd Y3sh+sXTQ54mu063/v8sDikWMRzTPNMPlj4T0fMvhzjKyqUQSUnUKDEjOwRrXk0XkVh5 zKvEI5+Y2dGpMMXdaXAeddnE08xaeLFOlOlRRV4pXq2xUmzWOZF68D94xeLjfmmgY4cW uNpL4wu2pFrEf5ql8q4QogNgiSzOwwh2j9lfMup4CLPMlqotjagZLNuWj8ViTR8FbM9k jfuveoSIx3pOMk/nzwYvdeDYrp3YWpAe59RktpbQRTv/5tpLtPOtEOj5o8YuI2MYxD9G 5ZQA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1736497725; x=1737102525; h=content-transfer-encoding:in-reply-to:from:content-language :references:to:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=fMHAnr47gYJmPF4Dg6UKLdQQ9r7gUgpmT9CNdLeTBRg=; b=nOs4QUQq+1Bf/Fq5UsUykvl3ugI/PZIyosPI1u/3TrgH2UGw5xIM8AKJULEwiOQWHi yLWRlklpFX3FOImgv8kS2HYANH1Y+0Q0YRX5yrSStDqrdUyWfPCMErfI27hMTdN64MzG pwIhS36ZJHzj54+jjLeIpEphO6MF/sG8FwVl8yhLWByyoVnu6iVW5yvDZBFlY3sx5Aby hnFc+mgk2olnCq5A8xvl8KD9IveKjPI4TVE+FllgVeV7KNVoSjztt//96TLf/8y7rE6X xi4Pb+J/zv0t530v/cqW0HkqbL2xJ+1hY60xb2ZliC/tChWpq7176gZWrCVvey7tsU8x 8v5Q== X-Forwarded-Encrypted: i=1; AJvYcCUQ/gZXwRcDVA1iU2FJh+g3cf/rrLwySjsQjAXWCExDFYHhdafPjXJMrIXx0fHSKK3A5nGGCl8=@vger.kernel.org X-Gm-Message-State: AOJu0YzibPcCQzR5tktV8KS+nd6wJVL03ntJT3FX20j0kr0h3whfaj/E v/XsNaq4Gc7tvfPYDVW4xfE/UXDBsvt9rgej7lzbpwj/cVmeBeTY7fh3bOyd31Q= X-Gm-Gg: ASbGncvUqCGF1mu0h8BZ+irFLDCwt/tVh+sHs+MsZ8c8l4LRf/RUp1pUCfa7l/B67YW L6+QlXr2np6nYg5MZsPksDTqebCkNpCE5i0gfiW/iaWCOt9mGm4U02kIkkDPJiradGNZ2FHVG0M 56h7hgBrAgZeIsKn7hYaCOE94XBYG7cwiwcRWddfcHlbYgZIoIFSijPR/kQU5UUogo9oKueXU5a 8oOmHOG0mRo2njP+b8gOMXwZOHs6bQy6H2IUTW9TN6ktl0GrbmdZyx7Je4z6uBu04U= X-Google-Smtp-Source: AGHT+IF1ZJ34S8wHfK4e0giFWrsLi/td4d5wyPmgupD6AicJN1a26QYSVhI7ULzCco4C0ujTpbXL5A== X-Received: by 2002:a05:6a20:c70a:b0:1dc:37a:8dc0 with SMTP id adf61e73a8af0-1e88cfd5b94mr16188605637.21.1736497725427; Fri, 10 Jan 2025 00:28:45 -0800 (PST) Received: from [157.82.203.37] ([157.82.203.37]) by smtp.gmail.com with ESMTPSA id d2e1a72fcca58-72d40569fddsm1067441b3a.39.2025.01.10.00.28.39 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Fri, 10 Jan 2025 00:28:44 -0800 (PST) Message-ID: Date: Fri, 10 Jan 2025 17:28:38 +0900 Precedence: bulk X-Mailing-List: netdev@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v2 1/3] tun: Unify vnet implementation To: Willem de Bruijn , Jonathan Corbet , Jason Wang , "David S. Miller" , Eric Dumazet , Jakub Kicinski , Paolo Abeni , "Michael S. Tsirkin" , Xuan Zhuo , Shuah Khan , linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org, netdev@vger.kernel.org, kvm@vger.kernel.org, virtualization@lists.linux-foundation.org, linux-kselftest@vger.kernel.org, Yuri Benditovich , Andrew Melnychenko , Stephen Hemminger , gur.stavi@huawei.com, devel@daynix.com References: <20250109-tun-v2-0-388d7d5a287a@daynix.com> <20250109-tun-v2-1-388d7d5a287a@daynix.com> <677fd7d26e090_362bc129432@willemb.c.googlers.com.notmuch> Content-Language: en-US From: Akihiko Odaki In-Reply-To: <677fd7d26e090_362bc129432@willemb.c.googlers.com.notmuch> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit On 2025/01/09 23:06, Willem de Bruijn wrote: > Akihiko Odaki wrote: >> Both tun and tap exposes the same set of virtio-net-related features. >> Unify their implementations to ease future changes. >> >> Signed-off-by: Akihiko Odaki >> --- >> MAINTAINERS | 1 + >> drivers/net/Kconfig | 5 ++ >> drivers/net/Makefile | 1 + >> drivers/net/tap.c | 172 ++++++---------------------------------- >> drivers/net/tun.c | 208 ++++++++----------------------------------------- >> drivers/net/tun_vnet.c | 186 +++++++++++++++++++++++++++++++++++++++++++ >> drivers/net/tun_vnet.h | 24 ++++++ >> 7 files changed, 273 insertions(+), 324 deletions(-) >> >> diff --git a/MAINTAINERS b/MAINTAINERS >> index 910305c11e8a..1be8a452d11f 100644 >> --- a/MAINTAINERS >> +++ b/MAINTAINERS >> @@ -23903,6 +23903,7 @@ F: Documentation/networking/tuntap.rst >> F: arch/um/os-Linux/drivers/ >> F: drivers/net/tap.c >> F: drivers/net/tun.c >> +F: drivers/net/tun_vnet.h >> >> TURBOCHANNEL SUBSYSTEM >> M: "Maciej W. Rozycki" >> diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig >> index 1fd5acdc73c6..255c8f9f1d7c 100644 >> --- a/drivers/net/Kconfig >> +++ b/drivers/net/Kconfig >> @@ -395,6 +395,7 @@ config TUN >> tristate "Universal TUN/TAP device driver support" >> depends on INET >> select CRC32 >> + select TUN_VNET > > No need for this new Kconfig I will merge tun_vnet.c into TAP. > >> static struct proto tap_proto = { >> .name = "tap", >> @@ -641,10 +576,10 @@ static ssize_t tap_get_user(struct tap_queue *q, void *msg_control, >> struct sk_buff *skb; >> struct tap_dev *tap; >> unsigned long total_len = iov_iter_count(from); >> - unsigned long len = total_len; >> + unsigned long len; >> int err; >> struct virtio_net_hdr vnet_hdr = { 0 }; >> - int vnet_hdr_len = 0; >> + int hdr_len; >> int copylen = 0; >> int depth; >> bool zerocopy = false; >> @@ -652,38 +587,20 @@ static ssize_t tap_get_user(struct tap_queue *q, void *msg_control, >> enum skb_drop_reason drop_reason; >> >> if (q->flags & IFF_VNET_HDR) { >> - vnet_hdr_len = READ_ONCE(q->vnet_hdr_sz); >> - >> - err = -EINVAL; >> - if (len < vnet_hdr_len) >> - goto err; >> - len -= vnet_hdr_len; >> - >> - err = -EFAULT; >> - if (!copy_from_iter_full(&vnet_hdr, sizeof(vnet_hdr), from)) >> - goto err; >> - iov_iter_advance(from, vnet_hdr_len - sizeof(vnet_hdr)); >> - if ((vnet_hdr.flags & VIRTIO_NET_HDR_F_NEEDS_CSUM) && >> - tap16_to_cpu(q, vnet_hdr.csum_start) + >> - tap16_to_cpu(q, vnet_hdr.csum_offset) + 2 > >> - tap16_to_cpu(q, vnet_hdr.hdr_len)) >> - vnet_hdr.hdr_len = cpu_to_tap16(q, >> - tap16_to_cpu(q, vnet_hdr.csum_start) + >> - tap16_to_cpu(q, vnet_hdr.csum_offset) + 2); >> - err = -EINVAL; >> - if (tap16_to_cpu(q, vnet_hdr.hdr_len) > len) >> + hdr_len = tun_vnet_hdr_get(READ_ONCE(q->vnet_hdr_sz), q->flags, from, &vnet_hdr); >> + if (hdr_len < 0) { >> + err = hdr_len; >> goto err; >> + } >> + } else { >> + hdr_len = 0; >> } >> >> - err = -EINVAL; >> - if (unlikely(len < ETH_HLEN)) >> - goto err; >> - > > Is this check removal intentional? No, I'm not sure what this check is for, but it is irrlevant with vnet header and shouldn't be modified with this patch. I'll restore the check with the next version. > >> + len = iov_iter_count(from); >> if (msg_control && sock_flag(&q->sk, SOCK_ZEROCOPY)) { >> struct iov_iter i; >> >> - copylen = vnet_hdr.hdr_len ? >> - tap16_to_cpu(q, vnet_hdr.hdr_len) : GOODCOPY_LEN; >> + copylen = hdr_len ? hdr_len : GOODCOPY_LEN; >> if (copylen > good_linear) >> copylen = good_linear; >> else if (copylen < ETH_HLEN) >> @@ -697,7 +614,7 @@ static ssize_t tap_get_user(struct tap_queue *q, void *msg_control, >> >> if (!zerocopy) { >> copylen = len; >> - linear = tap16_to_cpu(q, vnet_hdr.hdr_len); >> + linear = hdr_len; >> if (linear > good_linear) >> linear = good_linear; >> else if (linear < ETH_HLEN) >> @@ -732,9 +649,8 @@ static ssize_t tap_get_user(struct tap_queue *q, void *msg_control, >> } >> skb->dev = tap->dev; >> >> - if (vnet_hdr_len) { >> - err = virtio_net_hdr_to_skb(skb, &vnet_hdr, >> - tap_is_little_endian(q)); >> + if (q->flags & IFF_VNET_HDR) { >> + err = tun_vnet_hdr_to_skb(q->flags, skb, &vnet_hdr); >> if (err) { >> rcu_read_unlock(); >> drop_reason = SKB_DROP_REASON_DEV_HDR; >> @@ -797,23 +713,17 @@ static ssize_t tap_put_user(struct tap_queue *q, >> int total; >> >> if (q->flags & IFF_VNET_HDR) { >> - int vlan_hlen = skb_vlan_tag_present(skb) ? VLAN_HLEN : 0; >> struct virtio_net_hdr vnet_hdr; >> >> vnet_hdr_len = READ_ONCE(q->vnet_hdr_sz); >> - if (iov_iter_count(iter) < vnet_hdr_len) >> - return -EINVAL; >> - >> - if (virtio_net_hdr_from_skb(skb, &vnet_hdr, >> - tap_is_little_endian(q), true, >> - vlan_hlen)) >> - BUG(); >> >> - if (copy_to_iter(&vnet_hdr, sizeof(vnet_hdr), iter) != >> - sizeof(vnet_hdr)) >> - return -EFAULT; >> + ret = tun_vnet_hdr_from_skb(q->flags, NULL, skb, &vnet_hdr); >> + if (ret < 0) >> + goto done; >> >> - iov_iter_advance(iter, vnet_hdr_len - sizeof(vnet_hdr)); >> + ret = tun_vnet_hdr_put(vnet_hdr_len, iter, &vnet_hdr); >> + if (ret < 0) >> + goto done; > > Please split this patch in to a series of smaller patches. > > If feasible: > > 1. one that move the head of tun.c into tun_vnet.[hc]. > 2. then one that uses that also in tap.c. > 3. then a separate patch for the ioctl changes. > 4. then introduce tun_vnet_hdr_from_skb, tun_vnet_hdr_put > and friends in (a) follow-up patch(es). I will do so. > > This is subtle code. Please report what tests you ran to ensure > that it does not introduce behavioral changes / regressions. I tried: - curl on QEMU with macvtap (vhost=on) - curl on QEMU with macvtap (vhost=off)