Re: [PATCH net v2 2/2] net: ethernet: oa_tc6: fix tx skb race condition between reference pointers

[Paolo Abeni <pabeni@redhat.com>]

On 11/22/24 11:21, Parthiban Veerasooran wrote:
> There are two skb pointers to manage tx skb's enqueued from n/w stack.
> waiting_tx_skb pointer points to the tx skb which needs to be processed
> and ongoing_tx_skb pointer points to the tx skb which is being processed.
> 
> SPI thread prepares the tx data chunks from the tx skb pointed by the
> ongoing_tx_skb pointer. When the tx skb pointed by the ongoing_tx_skb is
> processed, the tx skb pointed by the waiting_tx_skb is assigned to
> ongoing_tx_skb and the waiting_tx_skb pointer is assigned with NULL.
> Whenever there is a new tx skb from n/w stack, it will be assigned to
> waiting_tx_skb pointer if it is NULL. Enqueuing and processing of a tx skb
> handled in two different threads.
> 
> Consider a scenario where the SPI thread processed an ongoing_tx_skb and
> it moves next tx skb from waiting_tx_skb pointer to ongoing_tx_skb pointer
> without doing any NULL check. At this time, if the waiting_tx_skb pointer
> is NULL then ongoing_tx_skb pointer is also assigned with NULL. After
> that, if a new tx skb is assigned to waiting_tx_skb pointer by the n/w
> stack and there is a chance to overwrite the tx skb pointer with NULL in
> the SPI thread. Finally one of the tx skb will be left as unhandled,
> resulting packet missing and memory leak.
> To overcome the above issue, protect the moving of tx skb reference from
> waiting_tx_skb pointer to ongoing_tx_skb pointer so that the other thread
> can't access the waiting_tx_skb pointer until the current thread completes
> moving the tx skb reference safely.

A mutex looks overkill. Why don't you use a spinlock? why locking only
one side (the writer) would be enough?

Could you please report the exact sequence of events in a time diagram
leading to the bug, something alike the following?

CPU0					CPU1
oa_tc6_start_xmit
 ...
					oa_tc6_spi_thread_handler
					 ...

Thanks,

Paolo