net: Inconsistent net_if TX locking

[krish2718]

We have a per-interface lock net_if_lock/unlock which synchronizes all transmissions on that interface amongst themselves and also with control path e.g, net_if_enable(true/false), but the locking itself inconsistent, few APIs take lock but few directly invoke TX. (Focusing on TX for now).

This can cause issues with de-initialization path (deinit is seldom used, so, probably disable/stopping the interface), and we do see an occasional crash after a few hours of interface disconnect and connect tests, I don't have a specific test case to exemplify the issue, so, analysis based on code walkthrough. Below is a call trace that I did and only see net_if_send_data taking the lock. Even the docs are unclear about the locking requirements.

net_if_queue_tx
	net_if_tx
	net_tc_submit_to_tx_queue
	
net_process_tx_packet
	net_if_tx
	
net_if_send_data
	lock:
		net_if_queue_tx
		
net_arp_input/arp_update/net_eth_bridge_update/gptp_*
	net_if_queue_tx

We should document the locking requirements and fix the inconsistent locking. Below patch fixes by making the net_if_tx as the entry point and it takes the lock, I have only done basic tests, but as this is critical raising an RFC than the patch.

diff --git a/subsys/net/ip/net_if.c b/subsys/net/ip/net_if.c
index b77c3ad3269..5de14e10339 100644
--- a/subsys/net/ip/net_if.c
+++ b/subsys/net/ip/net_if.c
@@ -222,6 +222,8 @@ static bool net_if_tx(struct net_if *iface, struct net_pkt *pkt)
                return false;
        }
 
+       net_if_lock(iface);
+
        create_time = net_pkt_create_time(pkt);
 
        debug_check_packet(pkt);
@@ -310,6 +312,8 @@ static bool net_if_tx(struct net_if *iface, struct net_pkt *pkt)
                net_if_call_link_cb(iface, &ll_dst, status);
        }
 
+       net_if_unlock(iface);
+
        return true;
 }
 
@@ -518,6 +522,7 @@ done:
                }
        } else if (verdict == NET_OK) {
                /* Packet is ready to be sent by L2, let's queue */
+               net_if_unlock(iface);
                net_if_queue_tx(iface, pkt);
        }

[rlubos]

To me it seems we should add lock to net_if_queue_tx() and net_process_tx_packet(), they both call net_if_tx(), and beside calling it additionally modify the interface (stats etc.). net_if_tx() will then be called only when the iface mutex is locked.

[krish2718]

See #64922 (comment)

[krish2718]

Closing this due to performance impact, leaving it for the L2 drivers to handle the data vs. control path races.