Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Incorrect call to sys_timeout for real-life IPv6 lifetime of 2 weeks (CON-283) #217

Closed
HomeACcessoryKid opened this issue Feb 13, 2023 · 3 comments

Comments

@HomeACcessoryKid
Copy link

I use the docker image of esp-matter and found this in the version I loaded this weekend.
Onboarding the matter light example, I stranded with this error once the Wi-Fi was started.

I (69963) ROUTE_HOOK: prefix 2001:1C03:1C45:E00:: lifetime 1209600
assert failed: sys_timeout IDF/components/lwip/lwip/src/core/timeouts.c:316 (Timeout time too long, max is LWIP_UINT32_MAX/4 msecs)

The cause of this assert failure is because 1209600 s = 2 weeks is bigger than UINT32_MAX/4 msec

Considering that LWIP has decided that this is the MAX, esp-matter has to respect this (right?).
And consider how to deal with such a real-life situation. Set a timeout or not?

It is /opt/espressif/esp-matter/components/route_hook/src/esp_route_table.c that needs a fix.

esp_route_entry_t *esp_route_table_add_route_entry(const esp_route_entry_t *route_entry) {
    //...
    entry->lifetime_seconds = route_entry->lifetime_seconds;
    if (entry->lifetime_seconds != UINT32_MAX) {
        sys_timeout(entry->lifetime_seconds * 1000, route_timeout_handler, entry);
    }
    return entry;
}

There are two options in case the timeout is between UINT32_MAX and UINT32_MAX/4000:

call sys_timeout by adding this in the if (){}:
if (entry->lifetime_seconds>UINT32_MAX/4000) entry->lifetime_seconds=UINT32_MAX/4000;

not call sys_timeout and replace if (entry->lifetime_seconds != UINT32_MAX) with:
if (entry->lifetime_seconds<=UINT32_MAX/4000)

Since I cannot decide the proper behaviour (without deepdiving) I leave the choice for fixing this to the espressif team.

Thanks as always for a wonderful ECO system!

PS. please feed this up to the CHIP repo because it is also present in other code:

/opt/espressif/esp-matter/connectedhomeip/connectedhomeip/examples/platform/bouffalolab/bl602/route_hook/bl_route_table.c
/opt/espressif/esp-matter/connectedhomeip/connectedhomeip/examples/platform/esp32/route_hook/esp_route_table.c
/opt/espressif/esp-matter/connectedhomeip/connectedhomeip/examples/platform/ameba/route_hook/ameba_route_table.c
@github-actions github-actions bot changed the title Incorrect call to sys_timeout for real-life IPv6 lifetime of 2 weeks Incorrect call to sys_timeout for real-life IPv6 lifetime of 2 weeks (CON-283) Feb 13, 2023
@wqx6
Copy link
Contributor

wqx6 commented Feb 14, 2023

In fact, we would ignore the RA packets with a lifetime over LWIP_UINT32_MAX / (1000 * 4). https://github.com/project-chip/connectedhomeip/blob/master/src/platform/ESP32/route_hook/ESP32RouteTable.c#L68. Considering that the route hook has been moved to the platform directory in upstream repo. We will remove the route_hook component and use the codes of upstream repo in next submodule updating.

@HomeACcessoryKid
Copy link
Author

HomeACcessoryKid commented Feb 14, 2023

Hmm, ignoring a Route Advertised might actually break things that would use that route!
I think the more appropriate thing to do is to make a timer that can handle longer times.
Think at least 31 days since we saw an example in the field of a 30 day lifetime.
Please think about this deeply, since it will be hard to debug if ignoring creates a blind spot.

Of course, if this upstream repo is going to solve this properly, then we shall be happy ;-)

@HomeACcessoryKid
Copy link
Author

This was closed in CHIP based on a proper fix in PullRequest project-chip/connectedhomeip#25074

Please make sure you pick this one up in the next submodule update?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants