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

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

[Dheeraj Reddy Jonnalagadda]

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()

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

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

[Parthiban.Veerasooran]

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
>