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

[<Parthiban.Veerasooran@microchip.com>]

Hi Jacob Keller,

On 21/11/24 1:27 am, Jacob Keller wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> 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...
Ok, thanks for your input. I think protecting this code section with a 
mutex lock will prevent this race condition. So the code will become 
like below,

if (!tc6->ongoing_tx_skb) {
	mutex_lock(&tx_skb_lock);
	tc6->ongoing_tx_skb = tc6->waiting_tx_skb;
	tc6->waiting_tx_skb = NULL;
	mutex_unlock(&tx_skb_lock);
}

Hope this is what you expected right?

Best regards,
Parthiban V
> 
>>                }
>