| Message ID | 20241120135142.586845-3-parthiban.veerasooran@microchip.com (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Fixes on the OPEN Alliance TC6 10BASE-T1x MAC-PHY support generic lib | expand |
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... > }
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 > >> } >
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; }
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(-)