[PATCH net v2 0/2] net: ethoc: Error path and transmit fixes

[PATCH net v2 0/2] net: ethoc: Error path and transmit fixes

[Florian Fainelli]

Hi all,

This patch series contains two patches for the ethoc driver while testing on a
TS-7300 board where ethoc is provided by an on-board FPGA.

First patch was cooked after chasing crashes with invalid resources passed to
the driver.

Second patch was cooked after seeing that an interface configured with IP
192.168.2.2 was sending ARP packets for 192.168.0.0, no wonder why it could not
work.

I don't have access to any other platform using an ethoc interface so
it could be good to some testing on Xtensa for instance.

Changes in v2, fixed the first commit message

Florian Fainelli (2):
  net: ethoc: Fix early error paths
  net: ethoc: Correctly pad short packets

 drivers/net/ethernet/ethoc.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

-- 
2.7.4

[PATCH net v2 1/2] net: ethoc: Fix early error paths

[Florian Fainelli]

In case any operation fails before we can successfully go the point
where we would register a MDIO bus, we would be going to an error label
which involves unregistering then freeing this yet to be created MDIO
bus. Update all error paths to go to label free which is the only one
valid until either the clock is enabled, or the MDIO bus is allocated
and registered. This fixes kernel oops observed while trying to
dereference the MDIO bus structure which is not yet allocated.

Fixes: a1702857724f ("net: Add support for the OpenCores 10/100 Mbps Ethernet MAC.")
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/ethernet/ethoc.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/ethoc.c b/drivers/net/ethernet/ethoc.c
index 4edb98c3c6c7..06ae14a8e946 100644
--- a/drivers/net/ethernet/ethoc.c
+++ b/drivers/net/ethernet/ethoc.c
@@ -1086,7 +1086,7 @@ static int ethoc_probe(struct platform_device *pdev)
 	if (!priv->iobase) {
 		dev_err(&pdev->dev, "cannot remap I/O memory space\n");
 		ret = -ENXIO;
-		goto error;
+		goto free;
 	}
 
 	if (netdev->mem_end) {
@@ -1095,7 +1095,7 @@ static int ethoc_probe(struct platform_device *pdev)
 		if (!priv->membase) {
 			dev_err(&pdev->dev, "cannot remap memory space\n");
 			ret = -ENXIO;
-			goto error;
+			goto free;
 		}
 	} else {
 		/* Allocate buffer memory */
@@ -1106,7 +1106,7 @@ static int ethoc_probe(struct platform_device *pdev)
 			dev_err(&pdev->dev, "cannot allocate %dB buffer\n",
 				buffer_size);
 			ret = -ENOMEM;
-			goto error;
+			goto free;
 		}
 		netdev->mem_end = netdev->mem_start + buffer_size;
 		priv->dma_alloc = buffer_size;
@@ -1120,7 +1120,7 @@ static int ethoc_probe(struct platform_device *pdev)
 		128, (netdev->mem_end - netdev->mem_start + 1) / ETHOC_BUFSIZ);
 	if (num_bd < 4) {
 		ret = -ENODEV;
-		goto error;
+		goto free;
 	}
 	priv->num_bd = num_bd;
 	/* num_tx must be a power of two */
@@ -1133,7 +1133,7 @@ static int ethoc_probe(struct platform_device *pdev)
 	priv->vma = devm_kzalloc(&pdev->dev, num_bd*sizeof(void *), GFP_KERNEL);
 	if (!priv->vma) {
 		ret = -ENOMEM;
-		goto error;
+		goto free;
 	}
 
 	/* Allow the platform setup code to pass in a MAC address. */
-- 
2.7.4

[PATCH net v2 2/2] net: ethoc: Correctly pad short packets

[Florian Fainelli]

Even though the hardware can be doing zero padding, we want the SKB to
be going out on the wire with the appropriate size. This fixes packet
truncations observed with e.g: ARP packets.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/ethernet/ethoc.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/net/ethernet/ethoc.c b/drivers/net/ethernet/ethoc.c
index 06ae14a8e946..ca678d46c322 100644
--- a/drivers/net/ethernet/ethoc.c
+++ b/drivers/net/ethernet/ethoc.c
@@ -860,6 +860,11 @@ static netdev_tx_t ethoc_start_xmit(struct sk_buff *skb, struct net_device *dev)
 	unsigned int entry;
 	void *dest;
 
+	if (skb_put_padto(skb, ETHOC_ZLEN)) {
+		dev->stats.tx_errors++;
+		goto out;
+	}
+
 	if (unlikely(skb->len > ETHOC_BUFSIZ)) {
 		dev->stats.tx_errors++;
 		goto out;
-- 
2.7.4

Re: [PATCH net v2 0/2] net: ethoc: Error path and transmit fixes

[David Miller]

From: Florian Fainelli <f.fainelli@gmail.com>
Date: Mon, 11 Jul 2016 16:35:53 -0700

> I don't have access to any other platform using an ethoc interface so
> it could be good to some testing on Xtensa for instance.

So I'll wait until someone does such testing before applying this
series.

Re: [PATCH net v2 2/2] net: ethoc: Correctly pad short packets

[Max Filippov]

Hi Florian,

On Mon, Jul 11, 2016 at 04:35:55PM -0700, Florian Fainelli wrote:
> Even though the hardware can be doing zero padding, we want the SKB to
> be going out on the wire with the appropriate size. This fixes packet
> truncations observed with e.g: ARP packets.
> 
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
>  drivers/net/ethernet/ethoc.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/net/ethernet/ethoc.c b/drivers/net/ethernet/ethoc.c
> index 06ae14a8e946..ca678d46c322 100644
> --- a/drivers/net/ethernet/ethoc.c
> +++ b/drivers/net/ethernet/ethoc.c
> @@ -860,6 +860,11 @@ static netdev_tx_t ethoc_start_xmit(struct sk_buff *skb, struct net_device *dev)
>  	unsigned int entry;
>  	void *dest;
>  
> +	if (skb_put_padto(skb, ETHOC_ZLEN)) {
> +		dev->stats.tx_errors++;
> +		goto out;
> +	}

skb_put_padto says that skb is freed on error, so this shoudn't go to
label 'out' on error where skb is freed again?

-- 
Thanks.
-- Max

Re: [PATCH net v2 0/2] net: ethoc: Error path and transmit fixes

[Max Filippov]

Hello,

On Mon, Jul 11, 2016 at 04:35:53PM -0700, Florian Fainelli wrote:
> This patch series contains two patches for the ethoc driver while testing on a
> TS-7300 board where ethoc is provided by an on-board FPGA.
> 
> First patch was cooked after chasing crashes with invalid resources passed to
> the driver.
> 
> Second patch was cooked after seeing that an interface configured with IP
> 192.168.2.2 was sending ARP packets for 192.168.0.0, no wonder why it could not
> work.

I can see opencores intrerface sending ARP packets shorter than 64 bytes,
but I couldn't reproduce truncation that affects packet contents on my
hardware.

> I don't have access to any other platform using an ethoc interface so
> it could be good to some testing on Xtensa for instance.

I've tested success and error paths affected by this series with the
following additional change on top of it:

diff --git a/drivers/net/ethernet/ethoc.c b/drivers/net/ethernet/ethoc.c
index ca678d4..8c94f45 100644
--- a/drivers/net/ethernet/ethoc.c
+++ b/drivers/net/ethernet/ethoc.c
@@ -862,7 +862,7 @@ static netdev_tx_t ethoc_start_xmit(struct sk_buff *skb, struct net_device *dev)
 
 	if (skb_put_padto(skb, ETHOC_ZLEN)) {
 		dev->stats.tx_errors++;
-		goto out;
+		return NETDEV_TX_OK;
 	}
 
 	if (unlikely(skb->len > ETHOC_BUFSIZ)) {

Without it the interface becomes non-functional after the first error
in skb_put_padto.

Tested-by: Max Filippov <jcmvbkbc@gmail.com>
Reviewed-by: Max Filippov <jcmvbkbc@gmail.com>

-- 
Thanks.
-- Max

Re: [PATCH net v2 0/2] net: ethoc: Error path and transmit fixes

[David Miller]

From: Max Filippov <jcmvbkbc@gmail.com>
Date: Tue, 12 Jul 2016 16:51:10 +0300

> Hello,
> 
> On Mon, Jul 11, 2016 at 04:35:53PM -0700, Florian Fainelli wrote:
>> This patch series contains two patches for the ethoc driver while testing on a
>> TS-7300 board where ethoc is provided by an on-board FPGA.
>> 
>> First patch was cooked after chasing crashes with invalid resources passed to
>> the driver.
>> 
>> Second patch was cooked after seeing that an interface configured with IP
>> 192.168.2.2 was sending ARP packets for 192.168.0.0, no wonder why it could not
>> work.
> 
> I can see opencores intrerface sending ARP packets shorter than 64 bytes,
> but I couldn't reproduce truncation that affects packet contents on my
> hardware.
> 
>> I don't have access to any other platform using an ethoc interface so
>> it could be good to some testing on Xtensa for instance.
> 
> I've tested success and error paths affected by this series with the
> following additional change on top of it:
> 
> diff --git a/drivers/net/ethernet/ethoc.c b/drivers/net/ethernet/ethoc.c
> index ca678d4..8c94f45 100644
> --- a/drivers/net/ethernet/ethoc.c
> +++ b/drivers/net/ethernet/ethoc.c
> @@ -862,7 +862,7 @@ static netdev_tx_t ethoc_start_xmit(struct sk_buff *skb, struct net_device *dev)
>  
>  	if (skb_put_padto(skb, ETHOC_ZLEN)) {
>  		dev->stats.tx_errors++;
> -		goto out;
> +		return NETDEV_TX_OK;
>  	}
>  
>  	if (unlikely(skb->len > ETHOC_BUFSIZ)) {
> 
> Without it the interface becomes non-functional after the first error
> in skb_put_padto.
> 
> Tested-by: Max Filippov <jcmvbkbc@gmail.com>
> Reviewed-by: Max Filippov <jcmvbkbc@gmail.com>

Indeed, skb_put_padto() frees the skb on error so this would have caused
a double free.

Florian, please respin this series with the fix and tags added.

Thanks.