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

[Jacob Keller <jacob.e.keller@intel.com>]

On 11/20/2024 5:51 AM, 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 assigns 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, check waiting_tx_skb pointer is not NULL
> along with ongoing_tx_skb pointer's NULL check before proceeding to assign
> the tx skb from waiting_tx_skb pointer to ongoing_tx_skb pointer.
> 
> Fixes: 53fbde8ab21e ("net: ethernet: oa_tc6: implement transmit path to transfer tx ethernet frames")
> Signed-off-by: Parthiban Veerasooran <parthiban.veerasooran@microchip.com>
> ---
>  drivers/net/ethernet/oa_tc6.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/oa_tc6.c b/drivers/net/ethernet/oa_tc6.c
> index 4c8b0ca922b7..e1e7c6e07966 100644
> --- a/drivers/net/ethernet/oa_tc6.c
> +++ b/drivers/net/ethernet/oa_tc6.c
> @@ -1003,7 +1003,7 @@ static u16 oa_tc6_prepare_spi_tx_buf_for_tx_skbs(struct oa_tc6 *tc6)
>  	 */
>  	for (used_tx_credits = 0; used_tx_credits < tc6->tx_credits;
>  	     used_tx_credits++) {
> -		if (!tc6->ongoing_tx_skb) {
> +		if (!tc6->ongoing_tx_skb && tc6->waiting_tx_skb) {
>  			tc6->ongoing_tx_skb = tc6->waiting_tx_skb;
>  			tc6->waiting_tx_skb = NULL;

It is unclear to me how this additional check completely resolves race
conditions? Is there some other locking or synchronization such the
second thread cannot have updated waiting_tx_skb either prior or after
this check?

This feels like you want some sort of atomic exchange operation...

>  		}