| Message ID | 20241219065926.1051732-1-dheeraj.linuxdev@gmail.com (mailing list archive) |
|---|---|
| State | Rejected |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] net: ethernet: oa_tc6: fix race condition on ongoing_tx_skb | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/apply | fail | Patch does not apply to net-next-0 |
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 >
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);
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(-)