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

[Tom Rini]

On Sun, Jul 23, 2023 at 07:14:17PM +0200, Miquel Raynal wrote:
> 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?

If I'm remembering things correctly there were other threads about this
and the approach here breaks other things (handling all of the network
stack in that way breaks netconsole I think was one thing), which is why
they weren't taken. So please try the other recent series here which
should address the problem as well, thanks.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20230723/f4d94ad7/attachment.sig>