netdev - RE: [PATCH] net: fec: Don't clear IPV6 header checksum field when IP accelerator enable

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] [day] [month] [year] [list]

Date:	Thu, 19 Jun 2014 02:03:50 +0000
From:	"fugang.duan@...escale.com" <fugang.duan@...escale.com>
To:	Russell King - ARM Linux <linux@....linux.org.uk>
CC:	"davem@...emloft.net" <davem@...emloft.net>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: RE: [PATCH] net: fec: Don't clear IPV6 header checksum field when	IP
 accelerator enable

From: Russell King - ARM Linux <linux@....linux.org.uk> Data: Thursday, June 19, 2014 12:09 AM
>To: Duan Fugang-B38611
>Cc: davem@...emloft.net; netdev@...r.kernel.org
>Subject: Re: [PATCH] net: fec: Don't clear IPV6 header checksum field when
>IP accelerator enable
[...]
>Secondly, there's another couple of bugs in the merged patches:
>
>	net: fec: Add Scatter/gather support
>
>+static int
>+fec_enet_txq_submit_frag_skb(struct sk_buff *skb, struct net_device
>+*ndev) {
>...
>+		if (dma_mapping_error(&fep->pdev->dev, bdp->cbd_bufaddr)) {
>...
>+			goto dma_mapping_error;
>... successful exit ...
>+	return 0;
>+
>+dma_mapping_error:
>... failure exit ...
>+	return NETDEV_TX_OK;
>+}
>
>+static int fec_enet_txq_submit_skb(struct sk_buff *skb, struct
>+net_device *ndev) {
>...
>+		ret = fec_enet_txq_submit_frag_skb(skb, ndev);
>+		if (ret)
>+			return ret;
>
>Firstly, NETDEV_TX_OK is enumerated as value zero.  So, whatever happens
>in fec_enet_txq_submit_frag_skb(), this function always returns zero even
>if we have an error.  This would lead to the packet ring being left in a
>confused state, with the header part of the packet sitting in the ring
>without the LAST bit set, and then the next packet to be submitted may
>cause the "packet" as seen by the hardware to be completed with a LAST
>buffer.
>
>Secondly, if fec_enet_txq_submit_skb() were to be fixed not to return zero,
>this still doesn't help the situation much - where's the error handling to
>clean up the header part of the packet?
>
For error handle, it is trouble. So .fec_enet_txq_submit_skb() return non-zero when dma mapping error,
And continue to dmp unmapping the header part of the packet.
I see some driver don't process dma mapping error like drivers/net/ethernet/marvell/mv643xx_eth.c.

>I'm currently porting my FEC patch set on top of your patches which were
>merged in the last window, which is causing me to notice various problems
>such as the above, which annoyingly I had already addressed in my version
>of SG support.  I'm working towards being able to post them, but it's
>going to be something like a week of solid work on my patches before I'm
>ready to do that.
>
If your SG patch is better than this, you can submit one patch to convert this to yours.
I believe there have better method for this, but I had done overninght test on this patch.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html