[PATCH 07/21] Revert "net: wget: Support retransmission a dropped packet"

[Simon Glass]

Hi Tom,

On Mon, 12 Aug 2024 at 12:46, Tom Rini <trini at konsulko.com> wrote:
>
> On Sun, Aug 11, 2024 at 08:50:18AM -0600, Simon Glass wrote:
> > +Ying-Chun Liu (PaulLiu) <paul.liu at linaro.org>
> >
> > Hi Yasuharu,
> >
> > On Sat, 10 Aug 2024 at 20:48, Yasuharu Shibata
> > <yasuharu.shibata at gmail.com> wrote:
> > >
> > > Dear Simon,
> > >
> > > Could you inform me how the wget test was broken?
> > > As I explained in the commit log, I fixed the bug in specific conditions.
> > > Without the details of how the break happened,
> > > it is difficult to find a proper patch.
> > >
> > > Best regards,
> > > Yasuharu Shibata
> >
> > Yes, I should have mentioned that. It is test/cmd/wget.c - see also [1].
> >
> > This is a good run:
> > Test: net_test_wget: wget.c
> > HTTP/1.1 200 OK
> > Packets received 5, Transfer Successful
> > Bytes transferred = 32 (20 hex)
> > md5 for 00020000 ... 0002001f ==> 234af48e94b0085060249ecb5942ab57
> > Failures: 0
> >
> > With your patch it hangs:
> >
> > Test: net_test_wget: wget.c
> > HTTP/1.1 200 OKT
>
> Can you please figure out what's going on with the sandbox test then?
> The change in question fixes wget support on real hardware, and also the
> other wget tests (as part of the lwIP series) work fine as well. This
> seems like something specific to that test.

The sandbox test is pretty simple, but it seems that something is very
odd in the wget code. I'm really not sure what it is supposed to be
doing.

Here is a good run, with both of these reverts:
(1) 1fd9476ec7d Revert "net: wget: Support retransmission a dropped packet"
(2) 3ebd131f7af Revert "net: wget: fix TCP sequence number wrap around issue"


Test: net_test_wget: wget.c
wget: Transfer HTTP Server 1.1.2.2; our IP 192.0.2.1
URL '/index.html'

wget:Load address: 0x20000
Loading: wget: send SYN
wget: Connecting In len=0, Seq=0, Ack=1
wget: Cting, send, len=0
wget: Connected seq=1, len=47
wget: Connected HTTP Header 0000560ee4323456
HTTP/1.1 200 OKwget: Connctd pkt 0000560ee4323456  hlen 27
wget: Connected Len 0
wget: offset 0
wget: Connected Pkt 0000560ee4323456 hlen 27
wget: Transferring, seq=1, ack=2, init_seq=28, len=47
wget: Transferring, seq=48, ack=3, init_seq=28, len=0
wget: offset 20

Packets received 5, Transfer Successful
Bytes transferred = 32 (20 hex)
md5 for 00020000 ... 0002001f ==> 234af48e94b0085060249ecb5942ab57
Failures: 0

Here is a bad run, with only (1) reverted:

Test: net_test_wget: wget.c
wget: Transfer HTTP Server 1.1.2.2; our IP 192.0.2.1
URL '/index.html'

wget:Load address: 0x20000
Loading: wget: send SYN
wget: Connecting In len=0, Seq=0, Ack=1
wget: Cting, send, len=0
wget: Connected seq=1, len=47
wget: Connected HTTP Header 000055d24b26e456
HTTP/1.1 200 OKwget: Connctd pkt 000055d24b26e456  hlen 27
wget: Connected Len 0
wget: offset 0
wget: Connected Pkt 000055d24b26e456 hlen 27
wget: Transferring, seq=1, ack=2, init_seq=28, len=47
wget: offset ffffffd9

wget error: trying to overwrite reserved memory...
wget: Transfer Fail - wget: store error

wget: Transferring, seq=48, ack=3, init_seq=28, len=0
wget: offset 20

Packets received 5, Transfer Successful
T T

You can see that seq is lower than init_seq so it tries to write to
memory before the buffer. I suppose in real hardware this would
normally be OK, but it isn't in sandbox.

With just (2) reverted I see this:

Test: net_test_wget: wget.c
wget: Transfer HTTP Server 1.1.2.2; our IP 192.0.2.1
URL '/index.html'

wget:Load address: 0x20000
Loading: wget: send SYN
wget: Connecting In len=0, Seq=0, Ack=1
wget: Cting, send, len=0
wget: Connected seq=1, len=47
wget: Connected HTTP Header 000055a5e9a25456
HTTP/1.1 200 OKwget: Connctd pkt 000055a5e9a25456  hlen 27
wget: Connected Len 0
wget: offset 0
wget: Connected Pkt 000055a5e9a25456 hlen 27
wget: Transferring, seq=1, ack=2, init_seq=28, len=47
wget: seq=48 packet was lost
wget: Transferring, seq=48, ack=3, init_seq=28, len=0
wget: offset 20
T T

At least in this case it doesn't write to bad memory, but it still doesn't work.

Regards,
Simon