rp2/mpnetworkport: Deregister all sys timeouts when netif is removed. #17624
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #17624 +/- ##
=======================================
Coverage 98.44% 98.44%
=======================================
Files 171 171
Lines 22208 22208
=======================================
Hits 21863 21863
Misses 345 345 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Code size report: |
|
A possible alternative to this PR would be to never remove netif's only add them once. I'm not sure how easy that would be to do, but it could simplify things quite a bit, to be able to delete all the code that removes netifs. |
projectgus
left a comment
projectgus
left a comment
There was a problem hiding this comment.
LGTM to fix the bug for now. The idea of never unregistering interfaces from lwip seems like a good long-term goal, though (less code size, etc).
|
Yes I agree that never de-registering the netif is a worthwhile future change; this is a change I've made in #17613 (review) for just the stm32 eth driver. Doing this has a few benefits; avoiding issues like this one certainly but also it means you can set / adjust the network interface config regardless of whether the interface is active. If/when the stm32 pr there is finalised and the architecture agreed on I'd be happy to refactor all the other network interfaces in the same way to make them all consistent. |
Really lwIP should provide this, to deregister all callbacks on the given netif. Signed-off-by: Damien George <damien@micropython.org>
When mDNS is active on a netif it registers a lot of timeouts, namely:
mdns_probe_and_announce
mdns_handle_tc_question
mdns_multicast_probe_timeout_reset_ipv4
mdns_multicast_timeout_25ttl_reset_ipv4
mdns_multicast_timeout_reset_ipv4
mdns_send_multicast_msg_delayed_ipv4
mdns_send_unicast_msg_delayed_ipv4
mdns_multicast_probe_timeout_reset_ipv6
mdns_multicast_timeout_25ttl_reset_ipv6
mdns_multicast_timeout_reset_ipv6
mdns_send_multicast_msg_delayed_ipv6
mdns_send_unicast_msg_delayed_ipv6
These may still be active after a netif is removed, and if they are called
they will find that the mDNS state pointer in the netif is NULL and they
will crash.
These functions could be explicitly removed using `sys_untimeout()`, but
`mdns_handle_tc_question()` is static so it's not possible to access it.
Instead use the new `sys_untimeout_all_with_arg()` helper to deregister all
timeout callbacks when a netif is removed.
Fixes issue micropython#17621.
Signed-off-by: Damien George <damien@micropython.org>
dpgeorge
force-pushed
the
rp2-fix-lwip-mdns-crash-on-netif-remove
branch
from
c409b3c to
0b698b8
Compare
3 participants
Summary
When mDNS is active on a netif it registers a lot of timeouts, namely:
These may still be active after a netif is removed, and if they are called they will find that the mDNS state pointer in the netif is NULL and they will crash.
These functions could be explicitly removed using
sys_untimeout(), butmdns_handle_tc_question()is static so it's not possible to access it. Instead use the newsys_untimeout_all_with_arg()helper to deregister all timeout callbacks when a netif is removed.Fixes issue #17621.
Testing
Tested with RPI_PICO_W firmware, running the test from #17621. It's been running for over 100 cycles without any issue (prior to the fix it would crash within 10 cycles).
Trade-offs and Alternatives
There's also
LWIP_NETIF_REMOVE_CALLBACKwhich is a more lightweight way to do something when a netif is removed. ButLWIP_NETIF_EXT_STATUS_CALLBACKis already enabled so easier to use that.This fix needs to be generalised to all ports that use lwIP, but for now I just wanted something minimal that works and is easy to review / reason about.
Note that we could probably use
LWIP_NETIF_EXT_STATUS_CALLBACKinstead of the hooksCYW43_CB_TCPIP_INIT_EXTRAandCYW43_CB_TCPIP_DEINIT_EXTRA, but that's also for follow-up work.