[PATCH 0/2] Fix network commands w/ USB Eth gadget

[Miquel Raynal]

Hi Tom,

trini at konsulko.com wrote on Sat, 22 Jul 2023 10:43:37 -0400:

> On Sat, Jul 22, 2023 at 12:25:35AM +0200, Miquel Raynal wrote:
> 
> > Hello,
> > 
> > I recently came across serious issues using U-Boot on Beagle Bone
> > Black. The USB Ethernet gadget is behaving in a way that is not
> > compliant with the uclass expectations, leading to use-after-free
> > accesses often producing data aborts. All network commands are
> > affected.
> > 
> > There are two problems:
> > * Any network command after completion could produce a data abort
> > * A tftp retrieval with a wrong file name would produce a data abort
> > 
> > Here is how the major issue (the former one) looks like:
> >   
> > => tftp 0x81000000 zImage  
> > using musb-hdrc, OUT ep1out IN ep1in STATUS ep2in
> > MAC f8:dc:7a:00:00:02
> > HOST MAC f8:dc:7a:00:00:01
> > RNDIS ready
> > musb-hdrc: peripheral reset irq lost!
> > high speed config #2: 2 mA, Ethernet Gadget, using RNDIS
> > USB RNDIS network up!
> > Using usb_ether device
> > TFTP from server 192.168.0.1; our IP address is 192.168.0.100
> > Filename 'zImage'.
> > Load address: 0x81000000
> > Loading: ##################################################  13 MiB
> >          4.2 MiB/s
> > done
> > Bytes transferred = 13634360 (d00b38 hex)
> > data abort
> > pc : [<9ff80fba>]          lr : [<9ff7abd9>]
> > reloc pc : [<8081bfba>]    lr : [<80815bd9>]
> > sp : 9df2f9f8  ip : 00000020     fp : 00000003
> > r10: 00000200  r9 : 9df44ea0     r8 : 9df2fa68
> > r7 : 9df2fa68  r6 : 9ffdbabc     r5 : 9ffcdbcd  r4 : 00000018
> > r3 : 00000018  r2 : 9ffdba00     r1 : 00000001  r0 : 9df4d348
> > Flags: Nzcv  IRQs off  FIQs on  Mode SVC_32 (T)
> > Code: 68c2 6881 f023 0303 (60ca) 4403 
> > Resetting CPU ...
> > 
> > While debugging this issue, I came across Qianfan's bug report which
> > raised this issue one year ago. Qianfan nicely pointed at two of his
> > patches sent on the mailing list following his investigations, which
> > IMHO got refused for a wrong reason.
> > 
> > Link: https://lore.kernel.org/all/7536b9e1-de7a-a492-6951-485d4eb75df1@163.com/
> > Link: https://patchwork.ozlabs.org/project/uboot/patch/20220402025836.19374-1-qianfanguijin@163.com/
> > Link: https://patchwork.ozlabs.org/project/uboot/patch/20220402025836.19374-2-qianfanguijin@163.com/
> > 
> > I've taken over Qianfan's two patches, I took the liberty to explain a
> > bit more what these issues were about and why they were serious,
> > rewording his first patch, and trying to fix the second issue
> > differently, because I believe the second issue should be avoided rather
> > than workarounded.
> > 
> > Once ready to send this series, I noticed that two other people already
> > tried to fix this:
> > Link: https://lore.kernel.org/all/20221212204411.2247170-1-bero@baylibre.com/
> > Link: https://lists.denx.de/pipermail/u-boot/2022-December/502055.html
> > 
> > I have no idea why this is still an open issue, I hope the "code
> > reorganization" reason that was mentioned in one of the above threads
> > does not stand anymore given how serious these issues are, so whatever
> > solution is preferred, I hope one will soon be picked-up :-)  
> 
> So, the original series was also the wrong direction for the problem,

I don't know if it was taking the wrong direction, nothing was
strongly bothering me. As Qianfan's proposal was not fitting the
maintainer's whishes I tried to think again about it in order to
propose something slightly different.
 
> overall.  Fortunately Marek was able to address the problem quite
> recently:
> https://patchwork.ozlabs.org/project/uboot/list/?series=364209&state=*
> 
> Please test that series, thanks.

I don't understand why the different approaches have been so quickly
discarded. They participate to make U-Boot more stable, now. Already 4
major releases have been tagged since these bugs were reported.

To my eyes we should all focus on getting issues fixed. Of course
discussing the fixes is part of the game but this has been "under
discussion" for a year, and yet this patchset is immediately rejected.
Marek's proposal, besides making total sense, has a real impact, it
needs to be discussed and tested (sorry, but it breaks user habits, SPL
in recovery modes, and all network commands), so why not taking trivial
patches with little-to-no side effects like these and revert them when
they are no longer relevant?

Thanks,
Miquèl