diff mbox series

ipheth: fix EOVERFLOW in ipheth_rcvbulk_callback

Message ID B60B8A4B-92A0-49B3-805D-809A2433B46C@abv.bg
State New
Headers show
Series ipheth: fix EOVERFLOW in ipheth_rcvbulk_callback | expand

Commit Message

Georgi Valkov July 20, 2021, 9:37 a.m. UTC
ipheth: fix EOVERFLOW in ipheth_rcvbulk_callback
https://github.com/openwrt/openwrt/pull/4084


From dd109ded2b526636fff438d33433ab64ffd21583 Mon Sep 17 00:00:00 2001
From: Georgi Valkov <gvalkov@abv.bg>
Date: Fri, 16 Apr 2021 20:44:36 +0300
Subject: [PATCH] ipheth: fix EOVERFLOW in ipheth_rcvbulk_callback

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.

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

Signed-off-by: Georgi Valkov <gvalkov@abv.bg>
---
 drivers/net/usb/ipheth.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Comments

Jakub Kicinski July 20, 2021, 10:22 a.m. UTC | #1
On Tue, 20 Jul 2021 12:37:43 +0300, Georgi Valkov wrote:
> ipheth: fix EOVERFLOW in ipheth_rcvbulk_callback
> https://github.com/openwrt/openwrt/pull/4084
> 
> 
> From dd109ded2b526636fff438d33433ab64ffd21583 Mon Sep 17 00:00:00 2001
> From: Georgi Valkov <gvalkov@abv.bg>
> Date: Fri, 16 Apr 2021 20:44:36 +0300
> Subject: [PATCH] ipheth: fix EOVERFLOW in ipheth_rcvbulk_callback

This is all unnecessary, IIUC you're submitting this patch for upstream
inclusion, please rebase it on the netdev/net tree, and try git
send-email on a file generated by git format-patch. Before that please
correct the fixes tag to the common format (you'll find it in docs or
follow what others do).
Georgi Valkov July 20, 2021, 11:39 a.m. UTC | #2
I am doing this for the first time, so any help would be appreciated!
What is to rebase on the netdev/net tree? The patch from my previous e-mail was
generated by `git format-patch -1`. I can’t notice any difference when compared to
to the newly generated patch, which I rebased on the latest master.

According to the description from the link below, I ran the following commands:
https://www.kernel.org/doc/html/latest/process/submitting-patches.html#providing-base-tree-information

git clone git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
cd linux
git checkout -t -b ipheth-fix-RX-EOVERFLOW master
git am --signoff < 0001-ipheth-fix-EOVERFLOW-in-ipheth_rcvbulk_callback.patch
git format-patch --base=auto --cover-letter -o drivers/net/ master


drivers/net/0000-cover-letter.patch

From cd18496373e28af570dc382f618edd442d705252 Mon Sep 17 00:00:00 2001
From: Georgi Valkov <gvalkov@abv.bg>
Date: Tue, 20 Jul 2021 14:15:58 +0300
Subject: [PATCH 0/1] *** SUBJECT HERE ***

*** BLURB HERE ***

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

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


base-commit: 2734d6c1b1a089fb593ef6a23d4b70903526fe0c
Greg Kroah-Hartman July 20, 2021, 11:49 a.m. UTC | #3
On Tue, Jul 20, 2021 at 02:39:49PM +0300, Georgi Valkov wrote:
> I am doing this for the first time, so any help would be appreciated!

Have you read Documentation/process/submitting-patches.rst yet?  If not,
please do so.

And look at examples on this list, you have to send individual patches,
not everything all crammed into one email.

thanks,

greg k-h
Georgi Valkov July 20, 2021, 12:46 p.m. UTC | #4
Yes, I read it, and before my previous e-mail that I also read the link from Jakub,
which essentially provides the same information.

There is only one patch 0001-ipheth-fix-EOVERFLOW-in-ipheth_rcvbulk_callback.patch
The command I used from the example also generated a 0000-cover-letter, so
I included it as well.

I still have no clue what exactly I should do. Can you please help me?

Georgi Valkov

> On 2021-07-20, at 2:49 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
> 
> On Tue, Jul 20, 2021 at 02:39:49PM +0300, Georgi Valkov wrote:
>> I am doing this for the first time, so any help would be appreciated!
> 
> Have you read Documentation/process/submitting-patches.rst yet?  If not,
> please do so.
> 
> And look at examples on this list, you have to send individual patches,
> not everything all crammed into one email.
> 
> thanks,
> 
> greg k-h
>
Georgi Valkov July 20, 2021, 1:12 p.m. UTC | #5
Thank you, Greg!

git send-email drivers/net/0001-ipheth-fix-EOVERFLOW-in-ipheth_rcvbulk_callback.patch
...
Result: OK

I hope I got right. I added most of the e-mail addresses, and also tried adding Message-Id.
I have not received the e-mail yet, so I cannot confirm if it worked or not.

Georgi Valkov


> On 2021-07-20, at 3:54 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
> 
> On Tue, Jul 20, 2021 at 03:46:11PM +0300, Georgi Valkov wrote:
>> Yes, I read it, and before my previous e-mail that I also read the link from Jakub,
>> which essentially provides the same information.
>> 
>> There is only one patch 0001-ipheth-fix-EOVERFLOW-in-ipheth_rcvbulk_callback.patch
> 
> Great, send that using 'git send-email' and all is good.
> 
>> The command I used from the example also generated a 0000-cover-letter, so
>> I included it as well.
> 
> Why do you need a cover letter for 1 patch?
> 
> thanks,
> 
> greg k-h
>
Jan Kiszka Jan. 31, 2022, 9:45 a.m. UTC | #6
Hi Georgi,

On 20.07.21 15:12, Georgi Valkov wrote:
> Thank you, Greg!
> 
> git send-email drivers/net/0001-ipheth-fix-EOVERFLOW-in-ipheth_rcvbulk_callback.patch
> ...
> Result: OK
> 
> I hope I got right. I added most of the e-mail addresses, and also tried adding Message-Id.
> I have not received the e-mail yet, so I cannot confirm if it worked or not.
> 

What happened here afterwards?

I just found out the hard way that this patch is still not in mainline 
but really needed.

Thanks,
Jan

>> On 2021-07-20, at 3:54 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
>>
>> On Tue, Jul 20, 2021 at 03:46:11PM +0300, Georgi Valkov wrote:
>>> Yes, I read it, and before my previous e-mail that I also read the link from Jakub,
>>> which essentially provides the same information.
>>>
>>> There is only one patch 0001-ipheth-fix-EOVERFLOW-in-ipheth_rcvbulk_callback.patch
>>
>> Great, send that using 'git send-email' and all is good.
>>
>>> The command I used from the example also generated a 0000-cover-letter, so
>>> I included it as well.
>>
>> Why do you need a cover letter for 1 patch?
>>
>> thanks,
>>
>> greg k-h
>>
>
Jakub Kicinski Jan. 31, 2022, 5:27 p.m. UTC | #7
On Mon, 31 Jan 2022 10:45:23 +0100 Jan Kiszka wrote:
> On 20.07.21 15:12, Georgi Valkov wrote:
> > Thank you, Greg!
> > 
> > git send-email drivers/net/0001-ipheth-fix-EOVERFLOW-in-ipheth_rcvbulk_callback.patch
> > ...
> > Result: OK
> > 
> > I hope I got right. I added most of the e-mail addresses, and also tried adding Message-Id.
> > I have not received the e-mail yet, so I cannot confirm if it worked or not.
> >   
> 
> What happened here afterwards?
> 
> I just found out the hard way that this patch is still not in mainline 
> but really needed.

I have not seen the repost :(
Jan Kiszka Jan. 31, 2022, 5:35 p.m. UTC | #8
On 31.01.22 18:27, Jakub Kicinski wrote:
> On Mon, 31 Jan 2022 10:45:23 +0100 Jan Kiszka wrote:
>> On 20.07.21 15:12, Georgi Valkov wrote:
>>> Thank you, Greg!
>>>
>>> git send-email drivers/net/0001-ipheth-fix-EOVERFLOW-in-ipheth_rcvbulk_callback.patch
>>> ...
>>> Result: OK
>>>
>>> I hope I got right. I added most of the e-mail addresses, and also tried adding Message-Id.
>>> I have not received the e-mail yet, so I cannot confirm if it worked or not.
>>>    
>>
>> What happened here afterwards?
>>
>> I just found out the hard way that this patch is still not in mainline
>> but really needed.
> 
> I have not seen the repost :(

Would it help if I do that on behalf of Georgi? Meanwhile, I can add a 
tested-by to it, after almost a full working day with it applied.

Jan
Jakub Kicinski Jan. 31, 2022, 5:47 p.m. UTC | #9
On Mon, 31 Jan 2022 18:35:44 +0100 Jan Kiszka wrote:
> On 31.01.22 18:27, Jakub Kicinski wrote:
> > On Mon, 31 Jan 2022 10:45:23 +0100 Jan Kiszka wrote:  
> >> What happened here afterwards?
> >>
> >> I just found out the hard way that this patch is still not in mainline
> >> but really needed.  
> > 
> > I have not seen the repost :(  
> 
> Would it help if I do that on behalf of Georgi? Meanwhile, I can add a 
> tested-by to it, after almost a full working day with it applied.

That's probably the most expedient way to close the issue, yup.
Real Fixes: tag would be useful as well.
Jan Kiszka Jan. 31, 2022, 6:46 p.m. UTC | #10
On 31.01.22 18:47, Jakub Kicinski wrote:
> On Mon, 31 Jan 2022 18:35:44 +0100 Jan Kiszka wrote:
>> On 31.01.22 18:27, Jakub Kicinski wrote:
>>> On Mon, 31 Jan 2022 10:45:23 +0100 Jan Kiszka wrote:
>>>> What happened here afterwards?
>>>>
>>>> I just found out the hard way that this patch is still not in mainline
>>>> but really needed.
>>>
>>> I have not seen the repost :(
>>
>> Would it help if I do that on behalf of Georgi? Meanwhile, I can add a
>> tested-by to it, after almost a full working day with it applied.
> 
> That's probably the most expedient way to close the issue, yup.
> Real Fixes: tag would be useful as well.

Need to check if I can find a causing commit, but I suspect that was
always broken.

Meanwhile, just to make fun of my "tested-by":

[29837.952695] ------------[ cut here ]------------
[29837.957350] WARNING: CPU: 0 PID: 0 at drivers/usb/core/urb.c:364 usb_submit_urb+0x68/0x4c4
[29837.965661] URB dc43e380 submitted while active
[29837.970209] Modules linked in: qcserial pppoe ppp_async option cdc_mbim uvcvideo usb_wwan sierra_net sierra rndis_host qmi_wwan pptp pppox ppp_mppe ppp_generic nf_nat_pptp nf_conntrack_pptp iptable_nat ipt_REJECT ipt_MASQUERADE huawei_cdc_ncm gspca_zc3xx gspca_ov534 gspca_main ebtable_nat ebtable_filter ebtable_broute cdc_ncm cdc_ether xt_time xt_tcpudp xt_tcpmss xt_string xt_statistic xt_state xt_recent xt_quota xt_pkttype xt_owner xt_nat xt_multiport xt_mark xt_mac xt_limit xt_length xt_iprange xt_hl xt_helper xt_ecn xt_dscp xt_conntrack xt_connmark xt_connlimit xt_connlabel xt_connbytes xt_comment xt_bpf xt_addrtype xt_TCPMSS xt_REDIRECT xt_NETMAP xt_LOG xt_HL xt_DSCP xt_CT xt_CLASSIFY wireguard videobuf2_v4l2 usbserial usbnet usblp ums_usbat ums_sddr55 ums_sddr09 ums_karma ums_jumpshot ums_isd200
[29838.041501]  ums_freecom ums_datafab ums_cypress ums_alauda ts_fsm ts_bm slhc nft_set_rbtree nft_set_hash nft_reject_ipv6 nft_reject_ipv4 nft_reject_inet nft_reject nft_redir_ipv4 nft_redir nft_quota nft_numgen nft_nat nft_meta nft_masq_ipv4 nft_masq nft_log nft_limit nft_exthdr nft_ct nft_counter nft_chain_route_ipv6 nft_chain_route_ipv4 nft_chain_nat_ipv4 nf_tables_ipv6 nf_tables_ipv4 nf_tables_inet nf_tables nf_reject_ipv4 nf_nat_tftp nf_nat_snmp_basic nf_nat_sip nf_nat_redirect nf_nat_proto_gre nf_nat_masquerade_ipv4 nf_nat_irc nf_conntrack_ipv4 nf_nat_ipv4 nf_nat_h323 nf_nat_ftp nf_nat_amanda nf_log_ipv4 nf_defrag_ipv4 nf_conntrack_tftp nf_conntrack_snmp nf_conntrack_sip nf_conntrack_rtcache nf_conntrack_proto_gre nf_conntrack_netlink nf_conntrack_irc nf_conntrack_h323 nf_conntrack_ftp nf_conntrack_broadcast
[29838.113904]  ts_kmp nf_conntrack_amanda iptable_raw iptable_mangle iptable_filter ipt_ECN ipheth ip6table_raw ip_tables input_core ebtables ebt_vlan ebt_stp ebt_snat ebt_redirect ebt_pkttype ebt_mark_m ebt_mark ebt_limit ebt_ip ebt_dnat ebt_arpreply ebt_arp ebt_among ebt_802_3 crc_ccitt cdc_wdm cdc_acm br_netfilter fuse sch_teql sch_sfq sch_red sch_prio sch_pie sch_multiq sch_gred sch_fq sch_dsmark sch_codel em_text em_nbyte em_meta em_cmp act_simple act_police act_pedit act_ipt act_gact act_csum libcrc32c sch_tbf sch_ingress sch_htb sch_hfsc em_u32 cls_u32 cls_tcindex cls_route cls_matchall cls_fw cls_flow cls_basic act_skbedit act_mirred videobuf2_vmalloc videobuf2_memops videobuf2_core v4l2_common videodev mwlwifi mac80211 cfg80211 compat xt_set ip_set_list_set ip_set_hash_netportnet ip_set_hash_netport
[29838.185710]  ip_set_hash_netnet ip_set_hash_netiface ip_set_hash_net ip_set_hash_mac ip_set_hash_ipportnet ip_set_hash_ipportip ip_set_hash_ipport ip_set_hash_ipmark ip_set_hash_ip ip_set_bitmap_port ip_set_bitmap_ipmac ip_set_bitmap_ip ip_set nfnetlink xt_weburl xt_webmon xt_timerange xt_bandwidth ip6table_nat nf_conntrack_ipv6 nf_defrag_ipv6 nf_nat_ipv6 ip6t_NPT ip6t_MASQUERADE nf_nat_masquerade_ipv6 nf_nat nf_conntrack ip6t_rt ip6t_mh ip6t_ipv6header ip6t_hbh ip6t_frag ip6t_eui64 ip6t_ah nf_log_ipv6 nf_log_common ip6table_mangle ip6table_filter ip6_tables ip6t_REJECT x_tables nf_reject_ipv6 nfsv4 nfsd nfs msdos ip_gre gre ip6_udp_tunnel udp_tunnel ip_tunnel rpcsec_gss_krb5 auth_rpcgss oid_registry tun vfat fat lockd sunrpc grace hfsplus dns_resolver dm_mirror dm_region_hash dm_log dm_crypt dm_mod
[29838.256898]  dax nls_utf8 nls_koi8_r nls_iso8859_2 nls_iso8859_15 nls_iso8859_13 nls_iso8859_1 nls_cp866 nls_cp852 nls_cp850 nls_cp775 nls_cp437 nls_cp1251 nls_cp1250 dma_shared_buffer md5 hmac ecb cts cbc uas ohci_platform ohci_hcd gpio_button_hotplug mii
[29838.279709] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 4.14.261 #0
[29838.285828] Hardware name: Marvell Armada 380/385 (Device Tree)
[29838.291781] [<c010ecf8>] (unwind_backtrace) from [<c010a9b8>] (show_stack+0x10/0x14)
[29838.299560] [<c010a9b8>] (show_stack) from [<c062f1f4>] (dump_stack+0x94/0xa8)
[29838.306816] [<c062f1f4>] (dump_stack) from [<c01287d4>] (__warn+0xe4/0x100)
[29838.313810] [<c01287d4>] (__warn) from [<c0128828>] (warn_slowpath_fmt+0x38/0x48)
[29838.321327] [<c0128828>] (warn_slowpath_fmt) from [<c04a9e1c>] (usb_submit_urb+0x68/0x4c4)
[29838.329650] [<c04a9e1c>] (usb_submit_urb) from [<bf7cb320>] (ipheth_tx+0x104/0x18c [ipheth])
[29838.338186] [<bf7cb320>] (ipheth_tx [ipheth]) from [<c0537a94>] (dev_hard_start_xmit+0xa0/0x114)
[29838.347014] [<c0537a94>] (dev_hard_start_xmit) from [<c055fbcc>] (sch_direct_xmit+0xb0/0x1b0)
[29838.355577] [<c055fbcc>] (sch_direct_xmit) from [<c055fd60>] (__qdisc_run+0x94/0xb4)
[29838.363355] [<c055fd60>] (__qdisc_run) from [<c0536d74>] (net_tx_action+0xf4/0x120)
[29838.371046] [<c0536d74>] (net_tx_action) from [<c0101628>] (__do_softirq+0xe0/0x240)
[29838.378827] [<c0101628>] (__do_softirq) from [<c012d46c>] (irq_exit+0xd4/0xe4)
[29838.386082] [<c012d46c>] (irq_exit) from [<c016662c>] (__handle_domain_irq+0x9c/0xac)
[29838.393947] [<c016662c>] (__handle_domain_irq) from [<c0101464>] (gic_handle_irq+0x5c/0x90)
[29838.402334] [<c0101464>] (gic_handle_irq) from [<c010b64c>] (__irq_svc+0x6c/0x90)
[29838.409849] Exception stack(0xc0a01f50 to 0xc0a01f98)
[29838.414922] 1f40:                                     00000001 00000000 00000000 c01145a0
[29838.423135] 1f60: ffffe000 c0a03cb8 c0a03c6c 00000000 00000000 414fc091 00000000 00000000
[29838.431349] 1f80: c0a01f98 c0a01fa0 c0107f68 c0107f6c 60000013 ffffffff
[29838.437995] [<c010b64c>] (__irq_svc) from [<c0107f6c>] (arch_cpu_idle+0x34/0x38)
[29838.445426] [<c0107f6c>] (arch_cpu_idle) from [<c015d318>] (do_idle+0xdc/0x19c)
[29838.452767] [<c015d318>] (do_idle) from [<c015d634>] (cpu_startup_entry+0x18/0x1c)
[29838.460371] [<c015d634>] (cpu_startup_entry) from [<c0900d3c>] (start_kernel+0x470/0x484)
[29838.468595] ---[ end trace 3b48202f08f7521f ]---

Old kernel, but the code diff of ipheth to master is minimal and
unrelated. Let's see if that reoccurs again. So far, I don't see how
that single tx urb could be resubmitted while in use.

Jan
Georgi Valkov Jan. 31, 2022, 9:43 p.m. UTC | #11
> On 2022-01-31, at 7:35 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
> 
> On 31.01.22 18:27, Jakub Kicinski wrote:
>> On Mon, 31 Jan 2022 10:45:23 +0100 Jan Kiszka wrote:
>>> On 20.07.21 15:12, Georgi Valkov wrote:
>>>> Thank you, Greg!
>>>> 
>>>> git send-email drivers/net/0001-ipheth-fix-EOVERFLOW-in-ipheth_rcvbulk_callback.patch
>>>> ...
>>>> Result: OK
>>>> 
>>>> I hope I got right. I added most of the e-mail addresses, and also tried adding Message-Id.
>>>> I have not received the e-mail yet, so I cannot confirm if it worked or not.
>>>>   
>>> 
>>> What happened here afterwards?
>>> 
>>> I just found out the hard way that this patch is still not in mainline
>>> but really needed.
>> I have not seen the repost :(
> 
> Would it help if I do that on behalf of Georgi? Meanwhile, I can add a tested-by to it, after almost a full working day with it applied.

Yes, please do it! The faster it gets mainline, the more people will benefit from the fix. As far as I recall, some months ago someone asked me to submit the patch using git mail or something like that, which I did for the first time. It command reported success, but I did not get any replays since then from anyone. I intended to resubmit it the following week, but got overwhelmed by tasks, and the time passed. Meanwhile I still keep the patch in GitHub/httpstorm/openwrt, brach gvalkov. No changes are required since the original e-mail, so it can be submitted to mainline.

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 unrelated issue also 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

The same happens if the phone reboots due to extreme cold temperatures while tethered. 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. Either way, running the commands mentioned above re-enable tethering and restore the communication instantly. It would be nice if a watchdog is integrated in ipheth to trigger recovery automatically.

Georgi Valkov


> Jan
> 
> -- 
> Siemens AG, Technology
> Competence Center Embedded Linux
>
diff mbox series

Patch

diff --git a/drivers/net/usb/ipheth.c b/drivers/net/usb/ipheth.c
index 207e59e74935..06d9f19ca142 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;