Re: [PATCH net 1/3] net: hns: Add TX CSUM check when fill TX description

Re: [PATCH net 1/3] net: hns: Add TX CSUM check when fill TX description

[David Miller]

From: Lin Yun Sheng <linyunsheng@huawei.com>
Date: Tue, 4 Jul 2017 18:47:31 +0800

> From: Yunsheng Lin <linyunsheng@huawei.com>
> 
> If driver support checksum offload, should check netdev feature
> before fill TX description and get CSUM err bit from RX
> description. HNS driver do the check in RX derction but it doesn't
> do the check in TX direction.
> 
> Signed-off-by: lipeng <lipeng321@huawei.com>
> Reviewed-by: Daode Huang <huangdaode@hisilicon.com>
> Reviewed-by: Yunsheng Lin <linyunsheng@huawei.com>

This is not correct.

You should be checking the skb->checksum field to decide if you should
offload the TX checksum of the packet or not.

Correct drivers, as far as I am aware, do not check the feature flags
so I wonder where you got this idea from.  Always use other well
established existing drivers as a model for how to handle things like
this.

And this makes sense.  An SKB can have it's checksumming determination
made first, then the netdev feature change is made afterwards.  For
correctness you still need to TX checksum offload that SKB otherwise
it will be emitted without a correctly computed checksum.

Thank you.

[PATCH net 0/3] Bugfixs for hns ethernet driver

[Lin Yun Sheng]

This patchset fix skb uesd after used, C45 op code and
Tx description filling issues in hns driver.

Yunsheng Lin (3):
  net: hns: Add TX CSUM check when fill TX description
  net: hns: Fix a wrong op phy C45 code
  net: hns: Fix a skb used after free bug

 drivers/net/ethernet/hisilicon/hns/hns_enet.c | 58 ++++++++++++++++-----------
 drivers/net/ethernet/hisilicon/hns/hns_enet.h |  8 ++--
 drivers/net/ethernet/hisilicon/hns_mdio.c     |  2 +-
 3 files changed, 40 insertions(+), 28 deletions(-)

-- 
1.9.1

[PATCH net 1/3] net: hns: Add TX CSUM check when fill TX description

[Lin Yun Sheng]

From: Yunsheng Lin <linyunsheng@huawei.com>

If driver support checksum offload, should check netdev feature
before fill TX description and get CSUM err bit from RX
description. HNS driver do the check in RX derction but it doesn't
do the check in TX direction.

Signed-off-by: lipeng <lipeng321@huawei.com>
Reviewed-by: Daode Huang <huangdaode@hisilicon.com>
Reviewed-by: Yunsheng Lin <linyunsheng@huawei.com>
---
 drivers/net/ethernet/hisilicon/hns/hns_enet.c | 36 +++++++++++++++++++--------
 drivers/net/ethernet/hisilicon/hns/hns_enet.h |  2 +-
 2 files changed, 26 insertions(+), 12 deletions(-)

diff --git a/drivers/net/ethernet/hisilicon/hns/hns_enet.c b/drivers/net/ethernet/hisilicon/hns/hns_enet.c
index c6700b9..b1e7224 100644
--- a/drivers/net/ethernet/hisilicon/hns/hns_enet.c
+++ b/drivers/net/ethernet/hisilicon/hns/hns_enet.c
@@ -40,12 +40,14 @@
 #define SKB_TMP_LEN(SKB) \
 	(((SKB)->transport_header - (SKB)->mac_header) + tcp_hdrlen(SKB))
 
-static void fill_v2_desc(struct hnae_ring *ring, void *priv,
+static void fill_v2_desc(struct hns_nic_ring_data *ring_data, void *priv,
 			 int size, dma_addr_t dma, int frag_end,
 			 int buf_num, enum hns_desc_type type, int mtu)
 {
+	struct hnae_ring *ring = ring_data->ring;
 	struct hnae_desc *desc = &ring->desc[ring->next_to_use];
 	struct hnae_desc_cb *desc_cb = &ring->desc_cb[ring->next_to_use];
+	struct net_device *ndev = ring_data->napi.dev;
 	struct iphdr *iphdr;
 	struct ipv6hdr *ipv6hdr;
 	struct sk_buff *skb;
@@ -90,8 +92,13 @@ static void fill_v2_desc(struct hnae_ring *ring, void *priv,
 
 			if (skb->protocol == htons(ETH_P_IP)) {
 				iphdr = ip_hdr(skb);
-				hnae_set_bit(rrcfv, HNSV2_TXD_L3CS_B, 1);
-				hnae_set_bit(rrcfv, HNSV2_TXD_L4CS_B, 1);
+
+				if (ndev->features & NETIF_F_IP_CSUM) {
+					hnae_set_bit(rrcfv, HNSV2_TXD_L3CS_B,
+						     1);
+					hnae_set_bit(rrcfv, HNSV2_TXD_L4CS_B,
+						     1);
+				}
 
 				/* check for tcp/udp header */
 				if (iphdr->protocol == IPPROTO_TCP &&
@@ -105,7 +112,10 @@ static void fill_v2_desc(struct hnae_ring *ring, void *priv,
 			} else if (skb->protocol == htons(ETH_P_IPV6)) {
 				hnae_set_bit(tvsvsn, HNSV2_TXD_IPV6_B, 1);
 				ipv6hdr = ipv6_hdr(skb);
-				hnae_set_bit(rrcfv, HNSV2_TXD_L4CS_B, 1);
+
+				if (ndev->features & NETIF_F_IPV6_CSUM)
+					hnae_set_bit(rrcfv, HNSV2_TXD_L4CS_B,
+						     1);
 
 				/* check for tcp/udp header */
 				if (ipv6hdr->nexthdr == IPPROTO_TCP &&
@@ -140,12 +150,14 @@ static void fill_v2_desc(struct hnae_ring *ring, void *priv,
 };
 MODULE_DEVICE_TABLE(acpi, hns_enet_acpi_match);
 
-static void fill_desc(struct hnae_ring *ring, void *priv,
+static void fill_desc(struct hns_nic_ring_data *ring_data, void *priv,
 		      int size, dma_addr_t dma, int frag_end,
 		      int buf_num, enum hns_desc_type type, int mtu)
 {
+	struct hnae_ring *ring = ring_data->ring;
 	struct hnae_desc *desc = &ring->desc[ring->next_to_use];
 	struct hnae_desc_cb *desc_cb = &ring->desc_cb[ring->next_to_use];
+	struct net_device *ndev = ring_data->napi.dev;
 	struct sk_buff *skb;
 	__be16 protocol;
 	u32 ip_offset;
@@ -179,12 +191,14 @@ static void fill_desc(struct hnae_ring *ring, void *priv,
 				skb->protocol = protocol;
 			}
 
-			if (skb->protocol == htons(ETH_P_IP)) {
+			if (skb->protocol == htons(ETH_P_IP) &&
+			    (ndev->features & NETIF_F_IP_CSUM)) {
 				flag_ipoffset |= 1 << HNS_TXD_L3CS_B;
 				/* check for tcp/udp header */
 				flag_ipoffset |= 1 << HNS_TXD_L4CS_B;
 
-			} else if (skb->protocol == htons(ETH_P_IPV6)) {
+			} else if (skb->protocol == htons(ETH_P_IPV6) &&
+				   (ndev->features & NETIF_F_IPV6_CSUM)) {
 				/* ipv6 has not l3 cs, check for L4 header */
 				flag_ipoffset |= 1 << HNS_TXD_L4CS_B;
 			}
@@ -275,7 +289,7 @@ static int hns_nic_maybe_stop_tso(
 	return 0;
 }
 
-static void fill_tso_desc(struct hnae_ring *ring, void *priv,
+static void fill_tso_desc(struct hns_nic_ring_data *ring_data, void *priv,
 			  int size, dma_addr_t dma, int frag_end,
 			  int buf_num, enum hns_desc_type type, int mtu)
 {
@@ -289,7 +303,7 @@ static void fill_tso_desc(struct hnae_ring *ring, void *priv,
 
 	/* when the frag size is bigger than hardware, split this frag */
 	for (k = 0; k < frag_buf_num; k++)
-		fill_v2_desc(ring, priv,
+		fill_v2_desc(ring_data, priv,
 			     (k == frag_buf_num - 1) ?
 					sizeoflast : BD_MAX_SEND_SIZE,
 			     dma + BD_MAX_SEND_SIZE * k,
@@ -339,7 +353,7 @@ int hns_nic_net_xmit_hw(struct net_device *ndev,
 		ring->stats.sw_err_cnt++;
 		goto out_err_tx_ok;
 	}
-	priv->ops.fill_desc(ring, skb, size, dma, seg_num == 1 ? 1 : 0,
+	priv->ops.fill_desc(ring_data, skb, size, dma, seg_num == 1 ? 1 : 0,
 			    buf_num, DESC_TYPE_SKB, ndev->mtu);
 
 	/* fill the fragments */
@@ -352,7 +366,7 @@ int hns_nic_net_xmit_hw(struct net_device *ndev,
 			ring->stats.sw_err_cnt++;
 			goto out_map_frag_fail;
 		}
-		priv->ops.fill_desc(ring, skb_frag_page(frag), size, dma,
+		priv->ops.fill_desc(ring_data, skb_frag_page(frag), size, dma,
 				    seg_num - 1 == i ? 1 : 0, buf_num,
 				    DESC_TYPE_PAGE, ndev->mtu);
 	}
diff --git a/drivers/net/ethernet/hisilicon/hns/hns_enet.h b/drivers/net/ethernet/hisilicon/hns/hns_enet.h
index 1b83232..da1afcc 100644
--- a/drivers/net/ethernet/hisilicon/hns/hns_enet.h
+++ b/drivers/net/ethernet/hisilicon/hns/hns_enet.h
@@ -46,7 +46,7 @@ struct hns_nic_ring_data {
 
 /* compatible the difference between two versions */
 struct hns_nic_ops {
-	void (*fill_desc)(struct hnae_ring *ring, void *priv,
+	void (*fill_desc)(struct hns_nic_ring_data *ring, void *priv,
 			  int size, dma_addr_t dma, int frag_end,
 			  int buf_num, enum hns_desc_type type, int mtu);
 	int (*maybe_stop_tx)(struct sk_buff **out_skb,
-- 
1.9.1

[PATCH net 2/3] net: hns: Fix a wrong op phy C45 code

[Lin Yun Sheng]

From: Yunsheng Lin <linyunsheng@huawei.com>

As the user manual described, the second step to write to C45 phy
by mdio should be data, but not address. Here we should fix this
issue.

Signed-off-by: Yankejian <yankejian@huawei.com>
Reviewed-by: lipeng <lipeng321@huawei.com>
Reviewed-by: Yunsheng Lin <linyunsheng@huawei.com>
---
 drivers/net/ethernet/hisilicon/hns_mdio.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/hisilicon/hns_mdio.c b/drivers/net/ethernet/hisilicon/hns_mdio.c
index e5221d9..017e084 100644
--- a/drivers/net/ethernet/hisilicon/hns_mdio.c
+++ b/drivers/net/ethernet/hisilicon/hns_mdio.c
@@ -261,7 +261,7 @@ static int hns_mdio_write(struct mii_bus *bus,
 
 		/* config the data needed writing */
 		cmd_reg_cfg = devad;
-		op = MDIO_C45_WRITE_ADDR;
+		op = MDIO_C45_WRITE_DATA;
 	}
 
 	MDIO_SET_REG_FIELD(mdio_dev, MDIO_WDATA_REG, MDIO_WDATA_DATA_M,
-- 
1.9.1

[PATCH net 3/3] net: hns: Fix a skb used after free bug

[Lin Yun Sheng]

From: Yunsheng Lin <linyunsheng@huawei.com>

skb maybe freed in hns_nic_net_xmit_hw() and return NETDEV_TX_OK,
which cause hns_nic_net_xmit to use a freed skb.

BUG: KASAN: use-after-free in hns_nic_net_xmit_hw+0x62c/0x940...
	[17659.112635]      alloc_debug_processing+0x18c/0x1a0
	[17659.117208]      __slab_alloc+0x52c/0x560
	[17659.120909]      kmem_cache_alloc_node+0xac/0x2c0
	[17659.125309]      __alloc_skb+0x6c/0x260
	[17659.128837]      tcp_send_ack+0x8c/0x280
	[17659.132449]      __tcp_ack_snd_check+0x9c/0xf0
	[17659.136587]      tcp_rcv_established+0x5a4/0xa70
	[17659.140899]      tcp_v4_do_rcv+0x27c/0x620
	[17659.144687]      tcp_prequeue_process+0x108/0x170
	[17659.149085]      tcp_recvmsg+0x940/0x1020
	[17659.152787]      inet_recvmsg+0x124/0x180
	[17659.156488]      sock_recvmsg+0x64/0x80
	[17659.160012]      SyS_recvfrom+0xd8/0x180
	[17659.163626]      __sys_trace_return+0x0/0x4
	[17659.167506] INFO: Freed in kfree_skbmem+0xa0/0xb0 age=23 cpu=1 pid=13
	[17659.174000]      free_debug_processing+0x1d4/0x2c0
	[17659.178486]      __slab_free+0x240/0x390
	[17659.182100]      kmem_cache_free+0x24c/0x270
	[17659.186062]      kfree_skbmem+0xa0/0xb0
	[17659.189587]      __kfree_skb+0x28/0x40
	[17659.193025]      napi_gro_receive+0x168/0x1c0
	[17659.197074]      hns_nic_rx_up_pro+0x58/0x90
	[17659.201038]      hns_nic_rx_poll_one+0x518/0xbc0
	[17659.205352]      hns_nic_common_poll+0x94/0x140
	[17659.209576]      net_rx_action+0x458/0x5e0
	[17659.213363]      __do_softirq+0x1b8/0x480
	[17659.217062]      run_ksoftirqd+0x64/0x80
	[17659.220679]      smpboot_thread_fn+0x224/0x310
	[17659.224821]      kthread+0x150/0x170
	[17659.228084]      ret_from_fork+0x10/0x40

	BUG: KASAN: use-after-free in hns_nic_net_xmit+0x8c/0xc0...
	[17751.080490]      __slab_alloc+0x52c/0x560
	[17751.084188]      kmem_cache_alloc+0x244/0x280
	[17751.088238]      __build_skb+0x40/0x150
	[17751.091764]      build_skb+0x28/0x100
	[17751.095115]      __alloc_rx_skb+0x94/0x150
	[17751.098900]      __napi_alloc_skb+0x34/0x90
	[17751.102776]      hns_nic_rx_poll_one+0x180/0xbc0
	[17751.107097]      hns_nic_common_poll+0x94/0x140
	[17751.111333]      net_rx_action+0x458/0x5e0
	[17751.115123]      __do_softirq+0x1b8/0x480
	[17751.118823]      run_ksoftirqd+0x64/0x80
	[17751.122437]      smpboot_thread_fn+0x224/0x310
	[17751.126575]      kthread+0x150/0x170
	[17751.129838]      ret_from_fork+0x10/0x40
	[17751.133454] INFO: Freed in kfree_skbmem+0xa0/0xb0 age=19 cpu=7 pid=43
	[17751.139951]      free_debug_processing+0x1d4/0x2c0
	[17751.144436]      __slab_free+0x240/0x390
	[17751.148051]      kmem_cache_free+0x24c/0x270
	[17751.152014]      kfree_skbmem+0xa0/0xb0
	[17751.155543]      __kfree_skb+0x28/0x40
	[17751.159022]      napi_gro_receive+0x168/0x1c0
	[17751.163074]      hns_nic_rx_up_pro+0x58/0x90
	[17751.167041]      hns_nic_rx_poll_one+0x518/0xbc0
	[17751.171358]      hns_nic_common_poll+0x94/0x140
	[17751.175585]      net_rx_action+0x458/0x5e0
	[17751.179373]      __do_softirq+0x1b8/0x480
	[17751.183076]      run_ksoftirqd+0x64/0x80
	[17751.186691]      smpboot_thread_fn+0x224/0x310
	[17751.190826]      kthread+0x150/0x170
	[17751.194093]      ret_from_fork+0x10/0x40

Reported-by: Jun He <hjat2005@huawei.com>
Signed-off-by: lipeng <lipeng321@huawei.com>
Reviewed-by: Yunsheng Lin <linyunsheng@huawei.com>
---
 drivers/net/ethernet/hisilicon/hns/hns_enet.c | 22 ++++++++++------------
 drivers/net/ethernet/hisilicon/hns/hns_enet.h |  6 +++---
 2 files changed, 13 insertions(+), 15 deletions(-)

diff --git a/drivers/net/ethernet/hisilicon/hns/hns_enet.c b/drivers/net/ethernet/hisilicon/hns/hns_enet.c
index b1e7224..8dfc220 100644
--- a/drivers/net/ethernet/hisilicon/hns/hns_enet.c
+++ b/drivers/net/ethernet/hisilicon/hns/hns_enet.c
@@ -314,9 +314,9 @@ static void fill_tso_desc(struct hns_nic_ring_data *ring_data, void *priv,
 			     mtu);
 }
 
-int hns_nic_net_xmit_hw(struct net_device *ndev,
-			struct sk_buff *skb,
-			struct hns_nic_ring_data *ring_data)
+netdev_tx_t hns_nic_net_xmit_hw(struct net_device *ndev,
+				struct sk_buff *skb,
+				struct hns_nic_ring_data *ring_data)
 {
 	struct hns_nic_priv *priv = netdev_priv(ndev);
 	struct hnae_ring *ring = ring_data->ring;
@@ -375,6 +375,10 @@ int hns_nic_net_xmit_hw(struct net_device *ndev,
 	dev_queue = netdev_get_tx_queue(ndev, skb->queue_mapping);
 	netdev_tx_sent_queue(dev_queue, skb->len);
 
+	netif_trans_update(ndev);
+	ndev->stats.tx_bytes += skb->len;
+	ndev->stats.tx_packets++;
+
 	wmb(); /* commit all data before submit */
 	assert(skb->queue_mapping < priv->ae_handle->q_num);
 	hnae_queue_xmit(priv->ae_handle->qs[skb->queue_mapping], buf_num);
@@ -1483,17 +1487,11 @@ static netdev_tx_t hns_nic_net_xmit(struct sk_buff *skb,
 				    struct net_device *ndev)
 {
 	struct hns_nic_priv *priv = netdev_priv(ndev);
-	int ret;
 
 	assert(skb->queue_mapping < ndev->ae_handle->q_num);
-	ret = hns_nic_net_xmit_hw(ndev, skb,
-				  &tx_ring_data(priv, skb->queue_mapping));
-	if (ret == NETDEV_TX_OK) {
-		netif_trans_update(ndev);
-		ndev->stats.tx_bytes += skb->len;
-		ndev->stats.tx_packets++;
-	}
-	return (netdev_tx_t)ret;
+
+	return hns_nic_net_xmit_hw(ndev, skb,
+				   &tx_ring_data(priv, skb->queue_mapping));
 }
 
 static void hns_nic_drop_rx_fetch(struct hns_nic_ring_data *ring_data,
diff --git a/drivers/net/ethernet/hisilicon/hns/hns_enet.h b/drivers/net/ethernet/hisilicon/hns/hns_enet.h
index da1afcc..648bab3 100644
--- a/drivers/net/ethernet/hisilicon/hns/hns_enet.h
+++ b/drivers/net/ethernet/hisilicon/hns/hns_enet.h
@@ -92,8 +92,8 @@ struct hns_nic_priv {
 void hns_nic_net_reset(struct net_device *ndev);
 void hns_nic_net_reinit(struct net_device *netdev);
 int hns_nic_init_phy(struct net_device *ndev, struct hnae_handle *h);
-int hns_nic_net_xmit_hw(struct net_device *ndev,
-			struct sk_buff *skb,
-			struct hns_nic_ring_data *ring_data);
+netdev_tx_t hns_nic_net_xmit_hw(struct net_device *ndev,
+				struct sk_buff *skb,
+				struct hns_nic_ring_data *ring_data);
 
 #endif	/**__HNS_ENET_H */
-- 
1.9.1

Re: [PATCH net 0/3] Bugfixs for hns ethernet driver

[Florian Fainelli]

On 04/07/2017 03:47, Lin Yun Sheng wrote:
> This patchset fix skb uesd after used, C45 op code and
> Tx description filling issues in hns driver.

Since these are fixes, can you include proper Fixes: tag so it is easier
for -stable maintainers to backport such changes where appropriate?
--
Florian

Re: [PATCH net 1/3] net: hns: Add TX CSUM check when fill TX description

[Yunsheng Lin]

Hi, David

On 2017/7/4 18:28, David Miller wrote:
> From: Lin Yun Sheng <linyunsheng@huawei.com>
> Date: Tue, 4 Jul 2017 18:47:31 +0800
> 
>> From: Yunsheng Lin <linyunsheng@huawei.com>
>>
>> If driver support checksum offload, should check netdev feature
>> before fill TX description and get CSUM err bit from RX
>> description. HNS driver do the check in RX derction but it doesn't
>> do the check in TX direction.
>>
>> Signed-off-by: lipeng <lipeng321@huawei.com>
>> Reviewed-by: Daode Huang <huangdaode@hisilicon.com>
>> Reviewed-by: Yunsheng Lin <linyunsheng@huawei.com>
> 
> This is not correct.
> 
> You should be checking the skb->checksum field to decide if you should
> offload the TX checksum of the packet or not.
> 
> Correct drivers, as far as I am aware, do not check the feature flags
> so I wonder where you got this idea from.  Always use other well
> established existing drivers as a model for how to handle things like
> this.
> 
> And this makes sense.  An SKB can have it's checksumming determination
> made first, then the netdev feature change is made afterwards.  For
> correctness you still need to TX checksum offload that SKB otherwise
> it will be emitted without a correctly computed checksum.
> 
You are right, thanks for pointing out.
driver/net/ethernet/hisilicon/hns/hns_enet.c
fill_desc:
		if (skb->ip_summed == CHECKSUM_PARTIAL) {
			protocol = skb->protocol;
			ip_offset = ETH_HLEN;

			/*if it is a SW VLAN check the next protocol*/
			if (protocol == htons(ETH_P_8021Q)) {
				ip_offset += VLAN_HLEN;
				protocol = vlan_get_protocol(skb);
				skb->protocol = protocol;
			}

			if (skb->protocol == htons(ETH_P_IP)) {
				flag_ipoffset |= 1 << HNS_TXD_L3CS_B;
				/* check for tcp/udp header */
				flag_ipoffset |= 1 << HNS_TXD_L4CS_B;

			} else if (skb->protocol == htons(ETH_P_IPV6)) {
				/* ipv6 has not l3 cs, check for L4 header */
				flag_ipoffset |= 1 << HNS_TXD_L4CS_B;
			}

			flag_ipoffset |= ip_offset << HNS_TXD_IPOFFSET_S;
		}

the hns driver has already check the skb->ip_summed, and checking the ndev->feature is
not correct.

Will remove this patch next revision.

Best Regards
Yunsheng Lin

Re: [PATCH net 0/3] Bugfixs for hns ethernet driver

[Yunsheng Lin]

Hi, Florian

On 2017/7/5 0:56, Florian Fainelli wrote:
> On 04/07/2017 03:47, Lin Yun Sheng wrote:
>> This patchset fix skb uesd after used, C45 op code and
>> Tx description filling issues in hns driver.
> 
> Since these are fixes, can you include proper Fixes: tag so it is easier
> for -stable maintainers to backport such changes where appropriate?
Thanks for pointing out. Will add it next version.

Best Regards
YunshengLin