ESP32: persistent PPP needed
Currently PPP tries to connect during some time, around 10 seconds
and then it gives up. After the timeout, PPP's serial traffic which looks
like !}!}!} }4}"}&} } } } }%}&y is no longer active at serial line and other
end (pppd linux) can't establish connection after timeout.
In application I would need PPP to continuously be active
because if I restart PPP by deleting its instance and creating again
after ESP32 has connected to WiFi, then PPP will spoil WiFi routing
and I don't want this to happen.
Additionaly - is this bug? This won't manually restart PPP after timeout:
ppp.active(False)
ppp.active(True)
Does nothing, while I think it should reactivate PPP traffic to
attempt connection on serial line, the chars !}!}!} }4}"}&} } } } }%}&y
should appear again
esp32/network_ppp: Bugfixes for deadlocks and crashes when disconnecting.
Summary
A follow-up to #17138 in which I reworked the ESP32 PPP implementation to be closer to the extmod implementation.
In that PR it was mentioned that:
PPP is quite hard to get right and modems can be very finicky. So it's great now that the code running on esp32 boards is also (indirectly) testing the extmod code, and vice versa
Well, I found three issues on the ESP32. Two of them are specific to the ESP32 and it's use of a thread-safe API, and I think one of them (the PCB cleanup) also is probably also wrong in the extmod version.
The three fixes (each a separate commit):
Use thread-safe API for PPPoS input
The ESP32 port uses the thread-safe API, but in the previous PR the PPP input function was accidentally changed to use the non-safe API. It happens to work fine, but the correct way is to use the thread-safe API as we do elsewhere in the implementation (and did before this change was accidentally introduced).
(extmod doesn't use the thread-safe API so isn't affected.)
Use non-thread-safe API inside status callback
The status callback runs on the lwIP tcpip_thread, and thus on the ESP32 we must use the non-thread-safe API because the thread-safe API would cause a deadlock (because it would wait on that same tcpip_thread to first finish executing the status callback).
(extmod doesn't use the thread-safe API so isn't affected, other than a change that doesn't change any functionally but keeps the two files as similar as possible when diffing them.)
Correctly clean up PPP PCB after close
If PPP is still connected, freeing the PCB will fail (see lwIP code here) and thus instead we should trigger a disconnect and wait for the lwIP callback to actually free the PCB.
When PPP is not connected we should check if the freeing failed, warn the user if so, and only mark the connection as inactive if not.
When all this happens during garbage collection the best case is that the PPP connection is already dead, which means the callback will be called immediately and cleanup will happen correctly. The worst case is that the connection is still alive, thus we are unable to free the PCB (lwIP won't let us) and it remains referenced in the netif_list, meaning a use-after-free happens later when lwIP traverses that linked list.
While this change does not fully fix the garbage collection case, on the ESP32 port specifically it does improve how the PPP.active(False) method behaves: It no longer immediately tries to free (and fails), but instead triggers a disconnect and lets the cleanup happen correctly through the status callback. (extmod doesn't have the .active() method.)
Testing
I've so far only tested this on the ESP32 port, by repeatedly connecting/disconnecting/deleting, and checking via GDB that the netif_list did not get corrupted anymore. As for other ports: I did not test them, but am fairly confident the change makes sense; as the linked code from the exact lwIP submodule used by extmod shows the ppp_free function indeed can fail and thus the change to handle it in the callback seems correct.