Riot: cpu/esp32: esp_eth is not working correctly for packet size > 255 octets

[gschorcht]

Description

ping6 from an ESP32 board does not work as expected when different payload lengths are used. Following behavior could be observed:

ping6 fe80::345b:6598:1369:5bf4
ping6 fe80::345b:6598:1369:5bf4 189
ping6 2003:c1:e72f:6c40:203b:b247:a518:3c7b

work as expected.

But

ping6 fe80::345b:6598:1369:5bf4 190

gives a timeout while

ping6 2003:c1:e72f:6c40:203b:b247:a518:3c7b 1

gives the error message error: malformed address.

It seems that there is a buffer problem that may also be related to the length of the destination address.

Steps to reproduce the issue

Use an ESP32 board and compile it with esp_eth netdev, e.g.:

USEMODULE=esp_eth make BOARD=esp32-olimex-evb -C examples/gnrc_networking flash

Execute ping commands to another host in the same LAN with the commands shown above.

Expected results

ping6 works for different address lengths and different payload sizes.

Actual results

Timeout or malformed address errors when ping6 is used for global unicast addresses used with the size argument or link local addresses are used with more than 189 byte payload length.

[gschorcht]

It seems that the error message error: malformed address when executing

ping6 2003:c1:e72f:6c40:203b:b247:a518:3c7b 1

has nothing to do with esp_eth since the error message is thrown before esp_eth is involved.

Furthermore, when executing

ping6 fe80::345b:6598:1369:5bf4 190

the ICMPv6 Echo request is received and replied by fe80::345b:6598:1369:5bf4. That is the reply is not received correctly.

[gschorcht]

@TimoRoth What I could figure out is that ESP Ethernet MAC layer (EMAC) is able to receive frames with a length of up to 1458 octets here:
https://github.com/RIOT-OS/RIOT/blob/6886ca54c2008813c469f508ac56db513c0b3f4d/cpu/esp32/esp-eth/esp_eth_netdev.c#L93
When the new packet is received the ISR calls:
https://github.com/RIOT-OS/RIOT/blob/6886ca54c2008813c469f508ac56db513c0b3f4d/cpu/esp32/esp-eth/esp_eth_netdev.c#L105
However the netdev isr callback
https://github.com/RIOT-OS/RIOT/blob/6886ca54c2008813c469f508ac56db513c0b3f4d/cpu/esp32/esp-eth/esp_eth_netdev.c#L328-L337
function is then called only for packets with a length of up to 255 octets. This corresponds excatly to ICMP echo replies produced with ping6 <address> 189.

@miri64 maybe, you have an idea from where the limitation comes. Since esp_eth netdev is simply a standard Ethernet netdev it just uses netdev_eth_get to return NETOPTs here:
https://github.com/RIOT-OS/RIOT/blob/6886ca54c2008813c469f508ac56db513c0b3f4d/cpu/esp32/esp-eth/esp_eth_netdev.c#L306

[miri64]

Can you clarify? Is the _esp_eth_isr() function or the _esp_eth_dev.netdev.event_callback() callback only called for packets >255. If it is the first it would be indeed weird, as there is no length check in gnrc_netif going on for the isr() call:

https://github.com/RIOT-OS/RIOT/blob/6886ca54c2008813c469f508ac56db513c0b3f4d/sys/net/gnrc/netif/gnrc_netif.c#L1281-L1289

Maybe here the wrong number is returned?

https://github.com/RIOT-OS/RIOT/blob/6886ca54c2008813c469f508ac56db513c0b3f4d/sys/net/gnrc/netif/ethernet/gnrc_netif_ethernet.c#L164

[gschorcht]

@miri64 Sorry, if I didn't express myself precisely enough.

_esp_eth_dev.netdev.event_callback(&_esp_eth_dev.netdev, NETDEV_EVENT_ISR); 

is called for frames of any size. But the netdev_driver_t::isr callback function _esp_eth_isr is only called for packets < 256 octets. netdev_driver_t::recv function _esp_eth_recv is also called only for packets < 256 octets whatever the parameters are. Returning only the size or dropping packets is implemented in _esp_eth_recv as specified.

[gschorcht]

The related modules used in the test are:

#define MODULE_ESP_ETH 1
#define MODULE_GNRC_NETIF 1
#define MODULE_GNRC_NETIF_ETHERNET 1
#define MODULE_NETDEV_ETH 1

[miri64]

is called for frames of any size. But the netdev_driver_t::isr callback function _esp_eth_isr is only called for packets < 256 octets.

That's really weird and I don't know how this behavior would come. As you can see the event callback just sends the message which is handled in the code snippet I quoted above:

https://github.com/RIOT-OS/RIOT/blob/30b39dcf44845e7bef996d4b3b03feb211d6ad20/sys/net/gnrc/netif/gnrc_netif.c#L1355-L1366

So there is no size-check happening there.

netdev_driver_t::recv function _esp_eth_recv is also called only for packets < 256 octets whatever the parameters are.

Least surprising, as recv is triggered by the isr callback (which should trigger the RX_COMPLETE event ;-):

https://github.com/RIOT-OS/RIOT/blob/30b39dcf44845e7bef996d4b3b03feb211d6ad20/sys/net/gnrc/netif/gnrc_netif.c#L1369-L1377

[gschorcht]

In principle the procedure netdev->event_callback(NETDEV_EVENT_ISR) -> netdev->driver->isr() -> netdev->event_callback(NETDEV_EVENT_RX_COMPLETE) -> netdev->driver->recv() seems to work as described in https://riot-os.org/api/group__drivers__netdev__api.html. Packets with a size of up to 255 octets can be received without any problems.

I'm still investigating, I will try to do more printf debugging later today.

[gschorcht]

Problem found. The length information is cut down to 8 bit by esp_eth
https://github.com/RIOT-OS/RIOT/blob/30b39dcf44845e7bef996d4b3b03feb211d6ad20/cpu/esp32/esp-eth/esp_eth_netdev.h#L40

[gschorcht]

:sunglasses:

[gschorcht]

BTW, it's the same for tx_len. Obviously, I had copied & pasted too much from esp_now.

[gschorcht]

@miri64 One further question. Is fragmentation supported by GNRC IPv6? ICMPv6 echo request works now for a data size of up to 1392 bytes. Starting from a data size of 1393 bytes the peer fragments its replies.

[miri64]

Nope, that's what project 24 is trying to finally make possible (IPv6 extension header handling used do be quite chaotic, but we are slowly going into a direction where new extension should be able to be added rather easy).

[TimoRoth]

I also wonder if it's correct for _recv in esp-eth to return -EINVAL when there is no packet to read. Shouldn't it just return 0 in that case?

[gschorcht]

Hm, https://riot-os.org/api/structnetdev__driver.html#ae2c8cad80067e3b1f9979931ddb3cc8b specifies 3 possible values:

Return value | Condition
-----------------|---------------
-ENOBUFS | if supplied buffer is too small
number of bytes read | if buf != NULL
packet size | if buf == NULL

Code is

    if (!buf && !len) {
        ...
        return size;
    }

    if (!buf && len) {
        ...
        return size;
    }

    if (buf && len && dev->rx_len) {
        if (dev->rx_len > len) {
            ...
            return -ENOBUFS;
        }
        ...
        return size;
    }

Thus all specified cases are covered. In any other case there is something invalid (arguments, ...). I would say any value less than 0 indicates an error. The only case where -EINVAL is returned is when buf != NULL and len or dev->rx_len is 0. This should not happen.

[gschorcht]

The only case where -EINVAL is returned is when buf != NULL and len or dev->rx_len is 0. This should not happen.

Furthermore, the combination buf != NULL and len == 0 is not a valid combination. The case where len !=0 and dev->rx_len == 0 while buf != NULL should be not possible.

[gschorcht]

BTW, when I was looking to other netdev drivers, I realized that cc110x returns a value that is not specified in case the buffer is too small.
https://github.com/RIOT-OS/RIOT/blob/30b39dcf44845e7bef996d4b3b03feb211d6ad20/drivers/cc110x/cc110x-netdev.c#L57-L59

[TimoRoth]

Yeah, "buf && len && !dev->rx_len" is the case I was thinking of, if that can never happen, that's indeed not an issue.

I changed the rx/tx_len to uint16_t for testing, and I now get a lot of this:

2018-12-12 13:15:15,537 - INFO # E (34967) [ esp_eth]: Not enough space in receive buffer for 478 byte
2018-12-12 13:15:26,966 - INFO # E (46393) [ esp_eth]: Not enough space in receive buffer for 310 byte
2018-12-12 13:15:31,674 - INFO # E (51102) [ esp_eth]: Not enough space in receive buffer for 346 byte
2018-12-12 13:16:15,561 - INFO # E (94988) [ esp_eth]: Not enough space in receive buffer for 478 byte
2018-12-12 13:17:04,785 - INFO # E (144208) [ esp_eth]: Not enough space in receive buffer for 310 byte

Maybe that should be a DEBUG(), and not LOG_TAG_ERROR(), as it does not seem to cause any issues.

[gschorcht]

enc28j60 netdev drivers sets the returns 0 in any error case, also if the buffer is too small :worried:

[gschorcht]

I have made the changes too and I have also seen this message sporadically I'm not sure whether this is an error or normal. I will investigate it later today. After that I will provide a PR including your suggestion to replace it by DEBUG.

[miri64]

I think both enc28j60 and cc110x should not be used as a template in this particular case. Both were implemented when the API was quite young, I believe and there are a few known issues in those implementations. The case of "no packet on receive" is an error case and should be handled by returning an appropriate error code (and documented if we can find a common ground). For too small buffers -ENOBUFS as return value is defined.

[gschorcht]

The case of "no packet on receive" is an error case and should be handled by returning an appropriate error code (and documented if we can find a common ground).

-EINVAL seems to me a quite reasonable error code in case the combination of buf and len parameters together with dev->rx_len is invalid.

[gschorcht]

As I remember issue #10410 is tracking a number of PRs that are dealing with the dropping implementation. cc110x driver is completly rewritten from scratch in PR #10340. I will take a look there.

[miri64]

EINVAL is typically used for invalid parameters [ref] not invalid state (which is the case here). I'd rather go with EIO, but maybe there is an even better error number.

[gschorcht]

I have changed _esp_eth_recv netdev according to new cc110x netdev driver.

[miri64]

I have changed _esp_eth_recv netdev according to new cc110x netdev driver.

Is there a PR for that?

[miri64]

EINVAL is typically used for invalid _parameters_ [ref] not invalid state (which is the case here). I'd rather go with EIO, but maybe there is an even better error number.

Maybe ENOMSG?

[miri64]

Or in conjunction with *_sock_recv(): EAGAIN.

[gschorcht]

Is there a PR for that?

For cc110x? Yes, PR #10340.

[miri64]

No, for your esp_eth fixes ;-).

[gschorcht]

There will be later today when I'm back in my office.

[miri64]

Ok.