netdev - Re: [PATCH 4/7] 8139cp: Fix TSO/scatter-gather descriptor setup

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1442872067.7367.86.camel@infradead.org>
Date:	Mon, 21 Sep 2015 22:47:47 +0100
From:	David Woodhouse <dwmw2@...radead.org>
To:	Francois Romieu <romieu@...zoreil.com>
Cc:	netdev@...r.kernel.org
Subject: Re: [PATCH 4/7] 8139cp: Fix TSO/scatter-gather descriptor setup

On Mon, 2015-09-21 at 23:01 +0200, Francois Romieu wrote:
> 
> Can you pile a patch to replace BUG with WARN_ON_ONCE(1) ?

Let's avoid having three copies of the same damn code, while we're at
it... http://git.infradead.org/users/dwmw2/linux-8139cp.git has this
and the appropriate minor fixes to subsequent patches in the series.

What do you think of finally enabling hw csum and TSO by default, btw?

Subject: [PATCH 4½/7] 8139cp: Reduce duplicate csum/tso code in cp_start_xmit()

We calculate the value of the opts1 descriptor field in three different
places. With two different behaviours when given an invalid packet to
be checksummed — none of them correct. Sort that out.

Signed-off-by: David Woodhouse <David.Woodhouse@...el.com>
---
 drivers/net/ethernet/realtek/8139cp.c | 61 ++++++++++++-----------------------
 1 file changed, 20 insertions(+), 41 deletions(-)

diff --git a/drivers/net/ethernet/realtek/8139cp.c b/drivers/net/ethernet/realtek/8139cp.c
index 3b219aa..a2c471d 100644
--- a/drivers/net/ethernet/realtek/8139cp.c
+++ b/drivers/net/ethernet/realtek/8139cp.c
@@ -740,7 +740,7 @@ static netdev_tx_t cp_start_xmit (struct sk_buff *skb,
 {
 	struct cp_private *cp = netdev_priv(dev);
 	unsigned entry;
-	u32 eor, flags;
+	u32 eor, opts1;
 	unsigned long intr_flags;
 	__le32 opts2;
 	int mss = 0;
@@ -760,6 +760,21 @@ static netdev_tx_t cp_start_xmit (struct sk_buff *skb,
 	mss = skb_shinfo(skb)->gso_size;
 
 	opts2 = cpu_to_le32(cp_tx_vlan_tag(skb));
+	opts1 = DescOwn;
+	if (mss)
+		opts1 |= LargeSend | ((mss & MSSMask) << MSSShift);
+	else if (skb->ip_summed == CHECKSUM_PARTIAL) {
+		const struct iphdr *ip = ip_hdr(skb);
+		if (ip->protocol == IPPROTO_TCP)
+			opts1 |= IPCS | TCPCS;
+		else if (ip->protocol == IPPROTO_UDP)
+			opts1 |= IPCS | UDPCS;
+		else {
+			WARN_ONCE(1,
+				  "Net bug: asked to checksum invalid Legacy IP packet\n");
+			goto out_dma_error;
+		}
+	}
 
 	if (skb_shinfo(skb)->nr_frags == 0) {
 		struct cp_desc *txd = &cp->tx_ring[entry];
@@ -775,21 +790,9 @@ static netdev_tx_t cp_start_xmit (struct sk_buff *skb,
 		txd->addr = cpu_to_le64(mapping);
 		wmb();
 
-		flags = eor | len | DescOwn | FirstFrag | LastFrag;
-
-		if (mss)
-			flags |= LargeSend | ((mss & MSSMask) << MSSShift);
-		else if (skb->ip_summed == CHECKSUM_PARTIAL) {
-			const struct iphdr *ip = ip_hdr(skb);
-			if (ip->protocol == IPPROTO_TCP)
-				flags |= IPCS | TCPCS;
-			else if (ip->protocol == IPPROTO_UDP)
-				flags |= IPCS | UDPCS;
-			else
-				WARN_ON(1);	/* we need a WARN() */
-		}
+		opts1 |= eor | len | FirstFrag | LastFrag;
 
-		txd->opts1 = cpu_to_le32(flags);
+		txd->opts1 = cpu_to_le32(opts1);
 		wmb();
 
 		cp->tx_skb[entry] = skb;
@@ -800,7 +803,6 @@ static netdev_tx_t cp_start_xmit (struct sk_buff *skb,
 		u32 first_len, first_eor, ctrl;
 		dma_addr_t first_mapping;
 		int frag, first_entry = entry;
-		const struct iphdr *ip = ip_hdr(skb);
 
 		/* We must give this initial chunk to the device last.
 		 * Otherwise we could race with the device.
@@ -832,19 +834,7 @@ static netdev_tx_t cp_start_xmit (struct sk_buff *skb,
 
 			eor = (entry == (CP_TX_RING_SIZE - 1)) ? RingEnd : 0;
 
-			ctrl = eor | len | DescOwn;
-
-			if (mss)
-				ctrl |= LargeSend |
-					((mss & MSSMask) << MSSShift);
-			else if (skb->ip_summed == CHECKSUM_PARTIAL) {
-				if (ip->protocol == IPPROTO_TCP)
-					ctrl |= IPCS | TCPCS;
-				else if (ip->protocol == IPPROTO_UDP)
-					ctrl |= IPCS | UDPCS;
-				else
-					BUG();
-			}
+			ctrl = opts1 | eor | len;
 
 			if (frag == skb_shinfo(skb)->nr_frags - 1)
 				ctrl |= LastFrag;
@@ -864,18 +854,7 @@ static netdev_tx_t cp_start_xmit (struct sk_buff *skb,
 		txd->addr = cpu_to_le64(first_mapping);
 		wmb();
 
-		ctrl = first_eor | first_len | FirstFrag | DescOwn;
-		if (mss)
-			ctrl |= LargeSend | (mss & MSSMask) << MSSShift);
-		else if (skb->ip_summed == CHECKSUM_PARTIAL) {
-			if (ip->protocol == IPPROTO_TCP)
-				ctrl |= IPCS | TCPCS;
-			else if (ip->protocol == IPPROTO_UDP)
-				ctrl |= IPCS | UDPCS;
-			else
-				BUG();
-		}
-
+		ctrl = opts1 | first_eor | first_len | FirstFrag;
 		txd->opts1 = cpu_to_le32(ctrl);
 		wmb();
 
-- 
2.4.3



-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@...el.com                              Intel Corporation


Download attachment "smime.p7s" of type "application/x-pkcs7-signature" (5691 bytes)