net: process_rx_packet() work handler violates requirements of Workqueue Threads implementation

[mnkp]

Describe the bug

The Workqueue Threads implementation imposes the following requirement

A pending work item must not be altered until the item has been processed by the workqueue thread. This means a work item must not be re-initialized while it is busy. Furthermore, any additional information the work item’s handler function needs to perform its work must not be altered until the handler function has finished executing.

This requirement is violated by void process_rx_packet(struct k_work *work) work handler function implemented in subsys/net/ip/net_core.c.

In the RX path when data are received, e.g. from an Ethernet driver, the packet is submitted to a workqueue in an interrupt context and then processed by process_rx_packet(struct k_work *work) work handler function in a thread context. In case of an error path (when the packet is not passed onto higher layers but needs to be discarded) the process_rx_packet function will free the memory allocated for the packet and along the memory where the k_work item is stored. I.e. ultimately the k_work item is freed before process_rx_packet function returns. If the same memory block is obtained and then resubmitted by the Ethernet driver we could overwrite content of k_work item.

Impact
This is a race condition, may lead to data lose / internal data corruption of the networking stack.

Environment (please complete the following information):

• zephyr-v2.5.0

[jukkar]

Thanks @mnkp.

This issue was already discussed in slack with following summary:

• Because of this workqueue restriction, it is not easy to free the net_pkt that contains the used k_work, as we cannot unref it from workhandler (as that would overwrite the k_work which is still in use)
• One possible solution could be to use k_fifo instead of k_work in RX path, so the net_data_recv() would place the net_pkt (that contains the received network data) into a k_fifo instead of calling workqueue handler
• This way we could remove the k_work (at least in RX path) from net_pkt saving some bytes
• The changes would be quite minimal and mostly contained in net_core.c
• The performance impact of this is unknown atm
• TX path has also the same issue so would need tweaking too.

I will investigate if we could implement this still for 2.6

FYI cc: @rlubos @mniestroj

[jukkar]

The changes were actually quite easy to do, and we get 10% better performance for both RX and TX. I think is good candidate for 2.6.