Re: [PATCH] net: 8390: Fix possible data races in __ei_get_stats

[Jia-Ju Bai]

On 2018/5/8 13:04, Eric Dumazet wrote:


On 05/07/2018 07:16 PM, Jia-Ju Bai wrote:


Yes, ">stats" will not change, because it is a fixed address.
But the field data in "dev->stats" is changed (rx_frame_errors, rx_crc_errors 
and rx_missed_errors).
So if the driver returns ">stats" without lock protection (like on line 
858), the field data value of this return value can be the changed field data value or unchanged 
field data value.


We do not care.

This function can be called by multiple cpus at the same time.

As soon as one cpu returns from it, another cpu can happily modify 
dev->stats.ANYFIELD.

Your patch fixes nothing at all.



Okay, thanks.
I also find that my patch does not work...


Best wishes,
Jia-Ju Bai

Re: [PATCH] net: 8390: Fix possible data races in __ei_get_stats

[Eric Dumazet]

On 05/07/2018 07:16 PM, Jia-Ju Bai wrote:

> Yes, ">stats" will not change, because it is a fixed address.
> But the field data in "dev->stats" is changed (rx_frame_errors, rx_crc_errors 
> and rx_missed_errors).
> So if the driver returns ">stats" without lock protection (like on line 
> 858), the field data value of this return value can be the changed field data 
> value or unchanged field data value.


We do not care.

This function can be called by multiple cpus at the same time.

As soon as one cpu returns from it, another cpu can happily modify 
dev->stats.ANYFIELD.

Your patch fixes nothing at all.

Re: [PATCH] net: 8390: Fix possible data races in __ei_get_stats

[Jia-Ju Bai]

On 2018/5/8 9:56, Eric Dumazet wrote:


On 05/07/2018 05:51 PM, Jia-Ju Bai wrote:


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 
---
   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(_local->page_lock, flags);
 /* If the card is stopped, just return the present stats. */
-if (!netif_running(dev))
-return >stats;
+if (!netif_running(dev)) {
+stats = >stats;
+spin_unlock_irqrestore(_local->page_lock, flags);
+return stats;
+}
   -spin_lock_irqsave(_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 = >stats;
   spin_unlock_irqrestore(_local->page_lock, flags);
   -return >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 ?


So I think a data race may occur when returning "dev->stats" without lock 
protection.

>stats is a stable value.

It wont change over the lifetime of net_device object.

Adding a barrier before or after getting >stats is useless, confusing and 
really not needed.



Yes, ">stats" will not change, because it is a fixed address.
But the field data in "dev->stats" is changed (rx_frame_errors, 
rx_crc_errors and rx_missed_errors).
So if the driver returns ">stats" without lock protection (like on 
line 858), the field data value of this return value can be the changed 
field data value or unchanged field data value.



Best wishes,
Jia-Ju Bai

Re: [PATCH] net: 8390: Fix possible data races in __ei_get_stats

[Eric Dumazet]

On 05/07/2018 05:51 PM, Jia-Ju Bai wrote:
> 
> 
> 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 
>>> ---
>>>   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(_local->page_lock, flags);
>>>     /* If the card is stopped, just return the present stats. */
>>> -    if (!netif_running(dev))
>>> -    return >stats;
>>> +    if (!netif_running(dev)) {
>>> +    stats = >stats;
>>> +    spin_unlock_irqrestore(_local->page_lock, flags);
>>> +    return stats;
>>> +    }
>>>   -    spin_lock_irqsave(_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 = >stats;
>>>   spin_unlock_irqrestore(_local->page_lock, flags);
>>>   -    return >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 ?

> 
> So I think a data race may occur when returning "dev->stats" without lock 
> protection.

>stats is a stable value.

It wont change over the lifetime of net_device object.

Adding a barrier before or after getting >stats is useless, confusing and 
really not needed.



> 
> By the way, I find this possible data race is similar to the previous commit 
> 7b31b4deda76 for the tg3 driver.

Very different things really.

This does a copy of the whole stats, not the pointer :

*stats = tp->net_stats_prev;


I guess you are confusing simple C semantics about returning the address of a 
structure,
instead of returning a whole structure.

If __ei_get_stats(struct net_device *dev) prototype was :

struct net_device_stats __ei_get_stats(struct net_device *dev) 

instead of :

struct net_device_stats *__ei_get_stats(struct net_device *dev) 

Then sure, your patch might been needed.

Re: [PATCH] net: 8390: Fix possible data races in __ei_get_stats

[Jia-Ju Bai]

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 
---
  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(_local->page_lock, flags);
  
  	/* If the card is stopped, just return the present stats. */

-   if (!netif_running(dev))
-   return >stats;
+   if (!netif_running(dev)) {
+   stats = >stats;
+   spin_unlock_irqrestore(_local->page_lock, flags);
+   return stats;
+   }
  
-	spin_lock_irqsave(_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 = >stats;
spin_unlock_irqrestore(_local->page_lock, flags);
  
-	return >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

Re: [PATCH] net: 8390: Fix possible data races in __ei_get_stats

[Eric Dumazet]

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 
> ---
>  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(_local->page_lock, flags);
>  
>   /* If the card is stopped, just return the present stats. */
> - if (!netif_running(dev))
> - return >stats;
> + if (!netif_running(dev)) {
> + stats = >stats;
> + spin_unlock_irqrestore(_local->page_lock, flags);
> + return stats;
> + }
>  
> - spin_lock_irqsave(_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 = >stats;
>   spin_unlock_irqrestore(_local->page_lock, flags);
>  
> - return >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.