Messages in this thread | | | Subject | Re: [PATCH] net: 8390: Fix possible data races in __ei_get_stats | From | Jia-Ju Bai <> | Date | Tue, 8 May 2018 08:51:12 +0800 |
| |
On 2018/5/7 22:15, Eric Dumazet wrote: > > On 05/07/2018 07:08 AM, Jia-Ju Bai wrote: >> The write operations to "dev->stats" are protected by >> the spinlock on line 862-864, but the read operations to >> this data on line 858 and 867 are not protected by the spinlock. >> Thus, there may exist data races for "dev->stats". >> >> To fix the data races, the read operations to "dev->stats" are >> protected by the spinlock, and a local variable is used for return. >> >> Signed-off-by: Jia-Ju Bai <baijiaju1990@gmail.com> >> --- >> drivers/net/ethernet/8390/lib8390.c | 14 ++++++++++---- >> 1 file changed, 10 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/net/ethernet/8390/lib8390.c b/drivers/net/ethernet/8390/lib8390.c >> index c9c55c9eab9f..198952247d30 100644 >> --- a/drivers/net/ethernet/8390/lib8390.c >> +++ b/drivers/net/ethernet/8390/lib8390.c >> @@ -852,19 +852,25 @@ static struct net_device_stats *__ei_get_stats(struct net_device *dev) >> unsigned long ioaddr = dev->base_addr; >> struct ei_device *ei_local = netdev_priv(dev); >> unsigned long flags; >> + struct net_device_stats *stats; >> + >> + spin_lock_irqsave(&ei_local->page_lock, flags); >> >> /* If the card is stopped, just return the present stats. */ >> - if (!netif_running(dev)) >> - return &dev->stats; >> + if (!netif_running(dev)) { >> + stats = &dev->stats; >> + spin_unlock_irqrestore(&ei_local->page_lock, flags); >> + return stats; >> + } >> >> - spin_lock_irqsave(&ei_local->page_lock, flags); >> /* Read the counter registers, assuming we are in page 0. */ >> dev->stats.rx_frame_errors += ei_inb_p(ioaddr + EN0_COUNTER0); >> dev->stats.rx_crc_errors += ei_inb_p(ioaddr + EN0_COUNTER1); >> dev->stats.rx_missed_errors += ei_inb_p(ioaddr + EN0_COUNTER2); >> + stats = &dev->stats; >> spin_unlock_irqrestore(&ei_local->page_lock, flags); >> >> - return &dev->stats; >> + return stats; >> } >> >> /* >> > dev->stats is not a pointer, it is an array embedded in the > struct net_device > > So this patch is not needed, since dev->stats can not change.
Thanks for your reply :)
I do not understand that why "dev->stats can not change". Its data is indeed changed by the code: dev->stats.rx_frame_errors += ei_inb_p(ioaddr + EN0_COUNTER0); dev->stats.rx_crc_errors += ei_inb_p(ioaddr + EN0_COUNTER1); dev->stats.rx_missed_errors += ei_inb_p(ioaddr + EN0_COUNTER2);
So I think a data race may occur when returning "dev->stats" without lock protection.
By the way, I find this possible data race is similar to the previous commit 7b31b4deda76 for the tg3 driver.
Best wishes, Jia-Ju Bai
| |