[PATCH v2 0/1] ipheth URB overflow fix

[PATCH v2 0/1] ipheth URB overflow fix

[Jan Kiszka]

Resent (v1 was mangled).

Note that the "Fixes:" tag is a strong assumption of mine. Speak up if
you think that is wrong.

Jan

Georgi Valkov (1):
  ipheth: fix EOVERFLOW in ipheth_rcvbulk_callback

 drivers/net/usb/ipheth.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

-- 
2.34.1

[PATCH v2 1/1] ipheth: fix EOVERFLOW in ipheth_rcvbulk_callback

[Jan Kiszka]

From: Georgi Valkov <gvalkov@abv.bg>

When rx_buf is allocated we need to account for IPHETH_IP_ALIGN,
which reduces the usable size by 2 bytes. Otherwise we have 1512
bytes usable instead of 1514, and if we receive more than 1512
bytes, ipheth_rcvbulk_callback is called with status -EOVERFLOW,
after which the driver malfunctiones and all communication stops.

Resolves ipheth 2-1:4.2: ipheth_rcvbulk_callback: urb status: -75

Fixes: f33d9e2b48a3 ("usbnet: ipheth: fix connectivity with iOS 14")
Signed-off-by: Georgi Valkov <gvalkov@abv.bg>
Tested-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 drivers/net/usb/ipheth.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/usb/ipheth.c b/drivers/net/usb/ipheth.c
index cd33955df0b6..6a769df0b421 100644
--- a/drivers/net/usb/ipheth.c
+++ b/drivers/net/usb/ipheth.c
@@ -121,7 +121,7 @@ static int ipheth_alloc_urbs(struct ipheth_device *iphone)
 	if (tx_buf == NULL)
 		goto free_rx_urb;
 
-	rx_buf = usb_alloc_coherent(iphone->udev, IPHETH_BUF_SIZE,
+	rx_buf = usb_alloc_coherent(iphone->udev, IPHETH_BUF_SIZE + IPHETH_IP_ALIGN,
 				    GFP_KERNEL, &rx_urb->transfer_dma);
 	if (rx_buf == NULL)
 		goto free_tx_buf;
@@ -146,7 +146,7 @@ static int ipheth_alloc_urbs(struct ipheth_device *iphone)
 
 static void ipheth_free_urbs(struct ipheth_device *iphone)
 {
-	usb_free_coherent(iphone->udev, IPHETH_BUF_SIZE, iphone->rx_buf,
+	usb_free_coherent(iphone->udev, IPHETH_BUF_SIZE + IPHETH_IP_ALIGN, iphone->rx_buf,
 			  iphone->rx_urb->transfer_dma);
 	usb_free_coherent(iphone->udev, IPHETH_BUF_SIZE, iphone->tx_buf,
 			  iphone->tx_urb->transfer_dma);
@@ -317,7 +317,7 @@ static int ipheth_rx_submit(struct ipheth_device *dev, gfp_t mem_flags)
 
 	usb_fill_bulk_urb(dev->rx_urb, udev,
 			  usb_rcvbulkpipe(udev, dev->bulk_in),
-			  dev->rx_buf, IPHETH_BUF_SIZE,
+			  dev->rx_buf, IPHETH_BUF_SIZE + IPHETH_IP_ALIGN,
 			  ipheth_rcvbulk_callback,
 			  dev);
 	dev->rx_urb->transfer_flags |= URB_NO_TRANSFER_DMA_MAP;
-- 
2.34.1

Re: [PATCH v2 0/1] ipheth URB overflow fix

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (master)
by Jakub Kicinski <kuba@kernel.org>:

On Tue, 1 Feb 2022 08:16:17 +0100 you wrote:
> Resent (v1 was mangled).
> 
> Note that the "Fixes:" tag is a strong assumption of mine. Speak up if
> you think that is wrong.
> 
> Jan
> 
> [...]

Here is the summary with links:
  - [v2,1/1] ipheth: fix EOVERFLOW in ipheth_rcvbulk_callback
    https://git.kernel.org/netdev/net/c/63e4b45c82ed

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH v2 0/1] ipheth URB overflow fix

[Yves-Alexis Perez]

On Tue, 2022-02-01 at 08:16 +0100, Jan Kiszka wrote:
> Georgi Valkov (1):
>   ipheth: fix EOVERFLOW in ipheth_rcvbulk_callback
> 
>  drivers/net/usb/ipheth.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)

Hi,

sorry for the extra-long delay. I finally tested the patch, and it seems to
work fine. I've tried it on my laptop for few hours without issue, but to be
fair it was working just fine before, I never experienced the EOVERFLOW
myself.

Regards,
-- 
Yves-Alexis

Re: [PATCH v2 0/1] ipheth URB overflow fix

[Georgi Valkov]

> On 2022-02-02, at 10:09 AM, Yves-Alexis Perez <corsac@corsac.net> wrote:
> 
> On Tue, 2022-02-01 at 08:16 +0100, Jan Kiszka wrote:
>> Georgi Valkov (1):
>>   ipheth: fix EOVERFLOW in ipheth_rcvbulk_callback
>> 
>>  drivers/net/usb/ipheth.c | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> Hi,
> 
> sorry for the extra-long delay. I finally tested the patch, and it seems to
> work fine. I've tried it on my laptop for few hours without issue, but to be
> fair it was working just fine before, I never experienced the EOVERFLOW
> myself.

Thank you for testing and committing the patch!

Hi Yves!
In order to experience the EOVERFLOW, the iPhone has to receive a large packet
of size 1514 bytes. Note that it is common for ISPs to limit the MTU, which results
in dropping large packets before they arrive at the iPhone. For example if I run

mtr 8.8.8.8 -n

 1. 172.20.10.1
 2. (waiting for reply)
 3. 10.98.8.1
 4. 10.98.8.253
 5. 46.10.207.99
 6. 212.39.69.106
 7. 212.39.66.222
 8. 216.239.59.239
 9. 74.125.251.185
10. 8.8.8.8

Host 5 drops large packets, while 3 and 4 replay. Now run
ping 10.98.8.1 -D -s 1472

Without the patch I get EOVERFLOW and there is no further communication.
It would be nice if a failsafe mechanism is implemented to recover from faults
like that or in the event that no communication is detected over a certain period.
With the patch applied, everything works fine:
1480 bytes from 10.98.8.1: icmp_seq=0 ttl=253 time=50.234 ms

There is another issue with my iPhone 7 Plus, which is unrelated to this patch:
If an iPhone is tethered to a MacBook, the next time it gets connected to an
OpenWRT router the USB Ethernet interface appears, but there is no
communication. Hence I would assume this issue has to be fixed in another
patch. I can confirm that in this state macOS and Windows are able to use
USB tethering, only OpenWRT is affected. So far I found the following
workarounds:
* reboot the phone or run:
* usbreset 002/002 && /etc/init.d/usbmuxd restart
* or in macOS disable the USB Ethernet interface, before the iPhone is
unplugged: e.g. Settings, Network, iPhone USB: check Disable unless needed,
then connect over wifi. The USB interface gets disabled.
* the same effect can also be achieved using QuickTime, File,
New Movie Recording. Selecting the iPhone, causes the USB Ethernet interface
to disappear. If we unplug and tether to OpenWRT now, it works fine. This looks
like an incomplete initialisation, likely in usbmuxd or ipheth, which needs to switch
the iPhone to the proper mode.

The same happens if the phone is powered off, and then restarted while tethered,
or if it reboots due to extreme cold temperatures or low battery. Finally there is
also a bug or possible hardware/baseband fault in my phone where every few
days the modem reboots: the LTE icon disappears for a few seconds, and
tethering is turned off. In the last case Personal Hotspot disappears in Settings,
but can still be accessed under Mobile Data. This is likely another iOS bug.

Either way, running the commands mentioned above re-enable tethering and
restore the communication instantly. There is no need to unlock the iPhone with
a passcode after it restarts. It would be nice if a watchdog is integrated
in ipheth to trigger recovery automatically.

Any ideas how to implement a watchdog to fix this separate issue?
--
Georgi Valkov

Re: [PATCH v2 0/1] ipheth URB overflow fix

[Yves-Alexis Perez]

On Wed, 2022-02-02 at 12:35 +0200, Georgi Valkov wrote:
> Without the patch I get EOVERFLOW and there is no further communication.
> It would be nice if a failsafe mechanism is implemented to recover from
> faults
> like that or in the event that no communication is detected over a certain
> period.
> With the patch applied, everything works fine:
> 1480 bytes from 10.98.8.1: icmp_seq=0 ttl=253 time=50.234 ms

Thanks for the steps. I can confirm that without the patch I get:

[  +4.516398] ipheth 1-3.3:4.2: ipheth_rcvbulk_callback: urb status: -75

and the interface doesn't work anymore.

With the patch applied, I get ping results and the network keeps going.

Regards,
-- 
Yves-Alexis