Re: [PATCH net-next] net: ethernet: oa_tc6: fix race condition on ongoing_tx_skb

[<Parthiban.Veerasooran@microchip.com>]

Hi,

On 19/12/24 12:29 pm, Dheeraj Reddy Jonnalagadda wrote:
> [You don't often get email from dheeraj.linuxdev@gmail.com. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
> 
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> A race condition exists in function oa_tc6_prepare_spi_tx_buf_for_tx_skbs
> due to an unsynchronized access to shared variable tc6->ongoing_tx_skb.
> 
> The issue arises because the condition (!tc6->ongoing_tx_skb) is checked
> outside the critical section. Two or more threads can simultaneously
> evaluate this condition as true before acquiring the lock. This results
> in both threads entering the critical section and modifying
> tc6->ongoing_tx_skb, causing inconsistent state updates or overwriting
> each other's changes.
> 
> Consider the following scenario. A race window exists in the sequence:
> 
>     Thread1                       Thread2
>     ------------------------      ------------------------
>     - if ongoing_tx_skb is NULL
>                                     - if ongoing_tx_skb is NULL
>                                     - spin_lock_bh()
>                                     - ongoing_tx_skb = waiting_tx_skb
>                                     - waiting_tx_skb = NULL
>                                     - spin_unlock_bh()
>     - spin_lock_bh()
>     - ongoing_tx_skb = waiting_tx_skb
>     - waiting_tx_skb = NULL
>     - spin_unlock_bh()
I don't think this scenario/sequence exists as the ongoing_tx_skb is not 
shared between two threads. waiting_tx_skb alone is shared between 
oa_tc6_start_xmit() and oa_tc6_spi_thread_handler(). ongoing_tx_skb is 
used only in the oa_tc6_spi_thread_handler() thread and it is 
sequential. So in my opinion, as done before, protecting waiting_tx_skb 
alone is enough and no need to protect ongoing_tx_skb.

Best regards,
Parthiban V
> 
> This leads to lost updates between ongoing_tx_skb and waiting_tx_skb
> fields. Moving the NULL check inside the critical section ensures both
> the NULL check and the assignment are protected by the same lock,
> maintaining atomic check-and-set operations.
> 
> Fixes: e592b5110b3e ("net: ethernet: oa_tc6: fix tx skb race condition between reference pointers")
> Closes: https://scan7.scan.coverity.com/#/project-view/52337/11354?selectedIssue=1602611
> Signed-off-by: Dheeraj Reddy Jonnalagadda <dheeraj.linuxdev@gmail.com>
> ---
>   drivers/net/ethernet/oa_tc6.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/oa_tc6.c b/drivers/net/ethernet/oa_tc6.c
> index db200e4ec284..66d55ec9bc88 100644
> --- a/drivers/net/ethernet/oa_tc6.c
> +++ b/drivers/net/ethernet/oa_tc6.c
> @@ -1004,12 +1004,12 @@ 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++) {
> +               spin_lock_bh(&tc6->tx_skb_lock);
>                  if (!tc6->ongoing_tx_skb) {
> -                       spin_lock_bh(&tc6->tx_skb_lock);
>                          tc6->ongoing_tx_skb = tc6->waiting_tx_skb;
>                          tc6->waiting_tx_skb = NULL;
> -                       spin_unlock_bh(&tc6->tx_skb_lock);
>                  }
> +               spin_unlock_bh(&tc6->tx_skb_lock);
>                  if (!tc6->ongoing_tx_skb)
>                          break;
>                  oa_tc6_add_tx_skb_to_spi_buf(tc6);
> --
> 2.34.1
>