From patchwork Mon Dec 9 15:31:10 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Vladislav Yasevich X-Patchwork-Id: 299103 X-Patchwork-Delegate: davem@davemloft.net Return-Path: X-Original-To: patchwork-incoming@ozlabs.org Delivered-To: patchwork-incoming@ozlabs.org Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by ozlabs.org (Postfix) with ESMTP id B6A562C00A3 for ; Tue, 10 Dec 2013 02:31:31 +1100 (EST) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932539Ab3LIPbP (ORCPT ); Mon, 9 Dec 2013 10:31:15 -0500 Received: from mail-yh0-f46.google.com ([209.85.213.46]:33727 "EHLO mail-yh0-f46.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755710Ab3LIPbO (ORCPT ); Mon, 9 Dec 2013 10:31:14 -0500 Received: by mail-yh0-f46.google.com with SMTP id l109so2728467yhq.33 for ; Mon, 09 Dec 2013 07:31:13 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=message-id:date:from:user-agent:mime-version:to:cc:subject :references:in-reply-to:content-type:content-transfer-encoding; bh=thK7sTPLvFmZ7HKYIf194dgudlMC2EC0jkKsHizOGI8=; b=CSm5lpcHA6cQtEvhuI6qO0+3z7YrXQLCWmqj3WqUFUepOjCtHdNaV9GfgAPGCVrtWW /bWKFbtg/7kVZ3hcV7iT7hFDsOyqDNbCyg3Dff7FZgvY74UNfdxTCgWzXPck3oNpj/Li w/cvXzNMMyJtNY1qwcjK+SPp/SAZBRlStto5rc1eGyZXSJmYhV1ltCx+GvNVenwldPCy /cs+IQ6WayBlAueQ+LX4C3J2n5Kr9StZPK+BuR27CsZgk8w4x63hu2sHUMo8I2lZYHyq C5cYv6k/238UKLbkc6hhmNPJPvA4Okj1cEq0YQAkX8Y6LcCePyh0vr0kS+PjPTcIOUfp CmSA== X-Received: by 10.236.5.174 with SMTP id 34mr3314897yhl.48.1386603073182; Mon, 09 Dec 2013 07:31:13 -0800 (PST) Received: from [192.168.98.160] (pool-70-109-158-146.cncdnh.east.myfairpoint.net. [70.109.158.146]) by mx.google.com with ESMTPSA id j67sm16607939yhe.26.2013.12.09.07.31.11 for (version=TLSv1 cipher=ECDHE-RSA-RC4-SHA bits=128/128); Mon, 09 Dec 2013 07:31:12 -0800 (PST) Message-ID: <52A5E23E.9030604@gmail.com> Date: Mon, 09 Dec 2013 10:31:10 -0500 From: Vlad Yasevich User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.1.0 MIME-Version: 1.0 To: "Michael S. Tsirkin" , Jason Wang CC: davem@davemloft.net, netdev@vger.kernel.org, linux-kernel@vger.kernel.org, Zhi Yong Wu Subject: Re: [PATCH net 1/2] tun: unbreak truncated packet signalling References: <1386584717-5776-1-git-send-email-jasowang@redhat.com> <20131209105529.GD15055@redhat.com> In-Reply-To: <20131209105529.GD15055@redhat.com> Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org On 12/09/2013 05:55 AM, Michael S. Tsirkin wrote: > On Mon, Dec 09, 2013 at 06:25:16PM +0800, Jason Wang wrote: >> Commit 6680ec68eff47d36f67b4351bc9836fd6cba9532 >> (tuntap: hardware vlan tx support) breaks the truncated packet signal by never >> return a length greater than iov length in tun_put_user(). This patch fixes this >> by always return the length of packet plus possible vlan header. Caller can >> detect the truncated packet by comparing the return value and the size of iov >> length. >> >> Reported-by: Vlad Yasevich >> Cc: Vlad Yasevich >> Cc: Zhi Yong Wu >> Signed-off-by: Jason Wang > > So writer gets back a value greater than what was written? > >> --- >> The patch is needed for stable. >> --- >> drivers/net/tun.c | 23 ++++++++++++----------- >> 1 file changed, 12 insertions(+), 11 deletions(-) >> >> diff --git a/drivers/net/tun.c b/drivers/net/tun.c >> index e26cbea..dd1bd7a 100644 >> --- a/drivers/net/tun.c >> +++ b/drivers/net/tun.c >> @@ -1183,7 +1183,11 @@ static ssize_t tun_put_user(struct tun_struct *tun, >> const struct iovec *iv, int len) >> { >> struct tun_pi pi = { 0, skb->protocol }; >> - ssize_t total = 0; >> + struct { >> + __be16 h_vlan_proto; >> + __be16 h_vlan_TCI; >> + } veth; >> + ssize_t total = 0, off = 0; > > Why off = 0 here? > We initialize it to total unconditionally, don't we? > >> int vlan_offset = 0; >> >> if (!(tun->flags & TUN_NO_PI)) { >> @@ -1248,14 +1252,11 @@ static ssize_t tun_put_user(struct tun_struct *tun, >> total += tun->vnet_hdr_sz; >> } >> >> + off = total; >> if (!vlan_tx_tag_present(skb)) { >> len = min_t(int, skb->len, len); >> } else { >> int copy, ret; >> - struct { >> - __be16 h_vlan_proto; >> - __be16 h_vlan_TCI; >> - } veth; >> >> veth.h_vlan_proto = skb->vlan_proto; >> veth.h_vlan_TCI = htons(vlan_tx_tag_get(skb)); >> @@ -1264,22 +1265,22 @@ static ssize_t tun_put_user(struct tun_struct *tun, >> len = min_t(int, skb->len + VLAN_HLEN, len); >> >> copy = min_t(int, vlan_offset, len); >> - ret = skb_copy_datagram_const_iovec(skb, 0, iv, total, copy); >> + ret = skb_copy_datagram_const_iovec(skb, 0, iv, off, copy); >> len -= copy; >> - total += copy; >> + off += copy; >> if (ret || !len) >> goto done; >> >> copy = min_t(int, sizeof(veth), len); >> - ret = memcpy_toiovecend(iv, (void *)&veth, total, copy); >> + ret = memcpy_toiovecend(iv, (void *)&veth, off, copy); >> len -= copy; >> - total += copy; >> + off += copy; >> if (ret || !len) >> goto done; > > This seems wrong: if one of the branches above is taken, total is > never incremented. > >> } >> >> - skb_copy_datagram_const_iovec(skb, vlan_offset, iv, total, len); >> - total += len; >> + skb_copy_datagram_const_iovec(skb, vlan_offset, iv, off, len); >> + total += skb->len + (vlan_offset ? sizeof(veth) : 0); >> >> done: >> tun->dev->stats.tx_packets++; > > I also think it's inelegant that the veth struct is now in the > outside scope, and the extra ? is also ugly. > > Here's a smaller patch to fix all these problems - what do you think? > > > > Signed-off-by: Michael S. Tsirkin > > --- > > diff --git a/drivers/net/tun.c b/drivers/net/tun.c > index 782e38b..3297e41 100644 > --- a/drivers/net/tun.c > +++ b/drivers/net/tun.c > @@ -1183,7 +1183,7 @@ static ssize_t tun_put_user(struct tun_struct *tun, > const struct iovec *iv, int len) > { > struct tun_pi pi = { 0, skb->protocol }; > - ssize_t total = 0; > + ssize_t total = 0, offset; > int vlan_offset = 0; > > if (!(tun->flags & TUN_NO_PI)) { > @@ -1248,6 +1248,8 @@ static ssize_t tun_put_user(struct tun_struct *tun, > total += tun->vnet_hdr_sz; > } > > + offset = total; > + total += skb->len; > if (!vlan_tx_tag_present(skb)) { > len = min_t(int, skb->len, len); > } else { > @@ -1257,6 +1259,8 @@ static ssize_t tun_put_user(struct tun_struct *tun, > __be16 h_vlan_TCI; > } veth; > > + total += sizeof(veth); > + > veth.h_vlan_proto = skb->vlan_proto; > veth.h_vlan_TCI = htons(vlan_tx_tag_get(skb)); > > @@ -1279,7 +1283,6 @@ static ssize_t tun_put_user(struct tun_struct *tun, > } > > skb_copy_datagram_const_iovec(skb, vlan_offset, iv, total, len); > - total += len; > > done: > tun->dev->stats.tx_packets++; > You have to use 'offset' instead of 'total' when doing skb_copy and adjust offset as you write the vlan header. I think something like this will fix it: -vlad --- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html diff --git a/drivers/net/tun.c b/drivers/net/tun.c index 782e38b..d71c393 100644 --- a/drivers/net/tun.c +++ b/drivers/net/tun.c @@ -1183,7 +1183,7 @@ static ssize_t tun_put_user(struct tun_struct *tun, const struct iovec *iv, int len) { struct tun_pi pi = { 0, skb->protocol }; - ssize_t total = 0; + ssize_t total = 0, offset; int vlan_offset = 0; if (!(tun->flags & TUN_NO_PI)) { @@ -1248,6 +1248,8 @@ static ssize_t tun_put_user(struct tun_struct *tun, total += tun->vnet_hdr_sz; } + offset = total; + total += skb->len; if (!vlan_tx_tag_present(skb)) { len = min_t(int, skb->len, len); } else { @@ -1262,24 +1264,24 @@ static ssize_t tun_put_user(struct tun_struct *tun, vlan_offset = offsetof(struct vlan_ethhdr, h_vlan_proto); len = min_t(int, skb->len + VLAN_HLEN, len); + total += VLAN_HLEN; copy = min_t(int, vlan_offset, len); - ret = skb_copy_datagram_const_iovec(skb, 0, iv, total, copy); + ret = skb_copy_datagram_const_iovec(skb, 0, iv, offset, copy); len -= copy; - total += copy; + offset += copy; if (ret || !len) goto done; copy = min_t(int, sizeof(veth), len); - ret = memcpy_toiovecend(iv, (void *)&veth, total, copy); + ret = memcpy_toiovecend(iv, (void *)&veth, offset, copy); len -= copy; - total += copy; + offset += copy; if (ret || !len) goto done; } - skb_copy_datagram_const_iovec(skb, vlan_offset, iv, total, len); - total += len; + skb_copy_datagram_const_iovec(skb, vlan_offset, iv, offset, len); done: tun->dev->stats.tx_packets++;