diff mbox

[net,v2,6/8] forcedeth: Fix a race during rmmod of forcedeth

Message ID 267015997eb594f2fd859acd572aa20dfc3e3e63.1320369398.git.david.decotigny@google.com
State Superseded, archived
Delegated to: David Miller
Headers show

Commit Message

david decotigny Nov. 4, 2011, 1:41 a.m. UTC
From: Salman Qazi <sqazi@google.com>

The race was between del_timer_sync and nv_do_stats_poll called through
nv_get_ethtool_stats.  To prevent this, we have to introduce mutual
exclusion between nv_get_ethtool_stats and del_timer_sync.  Notice
that we don't put the mutual exclusion in nv_do_stats_poll.  That's
because doing so would result in a deadlock, since it is a timer
callback and hence already waited for by timer deletion.



Signed-off-by: David Decotigny <david.decotigny@google.com>
---
 drivers/net/ethernet/nvidia/forcedeth.c |   11 ++++++++++-
 1 files changed, 10 insertions(+), 1 deletions(-)

Comments

Ben Hutchings Nov. 4, 2011, 3:46 a.m. UTC | #1
On Thu, 2011-11-03 at 18:41 -0700, David Decotigny wrote:
> From: Salman Qazi <sqazi@google.com>
> 
> The race was between del_timer_sync and nv_do_stats_poll called through
> nv_get_ethtool_stats.

I don't think so.  nv_close() and nv_get_ethtool_stats() are both called
with RTNL held.

Calling the timer function from nv_get_ethtool_stats is very likely part
of the problem though, so why don't you stop doing that?

[...]
> diff --git a/drivers/net/ethernet/nvidia/forcedeth.c b/drivers/net/ethernet/nvidia/forcedeth.c
> index 0af12a8..7996782 100644
> --- a/drivers/net/ethernet/nvidia/forcedeth.c
> +++ b/drivers/net/ethernet/nvidia/forcedeth.c
> @@ -3937,6 +3937,10 @@ static void nv_poll_controller(struct net_device *dev)
>  }
>  #endif
>  
> +/* No locking is needed as long as this is in the timer
> + * callback.  However, any other callers must call this
> + * function with np->lock held.
> + */

So long as this function is used by all of (1) the timer function (2)
the ndo_get_stats implementation (3) the ethtool get_stats
implementation, it can most certainly be called concurrently on multiple
processors.

You could have (2) and (3) return the last polled stats and not poll the
hardware themselves, but you would need to use the functions from
<linux/u64_stats_sync.h> to avoid word-tearing on 32-bit architectures.

>  static void nv_do_stats_poll(unsigned long data)
>  {
>         struct net_device *dev = (struct net_device *) data;
> @@ -4589,12 +4593,17 @@ static int nv_get_sset_count(struct net_device *dev, int sset)
>  
>  static void nv_get_ethtool_stats(struct net_device *dev, struct ethtool_stats *estats, u64 *buffer)
>  {
> +       unsigned long flags;
>         struct fe_priv *np = netdev_priv(dev);
>  
> +       spin_lock_irqsave(&np->lock, flags);
> +
>         /* update stats */
>         nv_do_stats_poll((unsigned long)dev);
>  
>         memcpy(buffer, &np->estats, nv_get_sset_count(dev, ETH_SS_STATS)*sizeof(u64));
> +
> +       spin_unlock_irqrestore(&np->lock, flags);

This function is not called from interrupt context.

>  }
>  
>  static int nv_link_test(struct net_device *dev)
> @@ -5189,13 +5198,13 @@ static int nv_close(struct net_device *dev)
>  
>         spin_lock_irq(&np->lock);
>         np->in_shutdown = 1;
> +       del_timer_sync(&np->stats_poll);
>         spin_unlock_irq(&np->lock);
>         nv_napi_disable(dev);
>         synchronize_irq(np->pci_dev->irq);
>  
>         del_timer_sync(&np->oom_kick);
>         del_timer_sync(&np->nic_poll);
> -       del_timer_sync(&np->stats_poll);
>  
>         netif_stop_queue(dev);
>         spin_lock_irq(&np->lock);

I don't believe this code movement is helpful.

Ben.
david decotigny Nov. 4, 2011, 5:22 p.m. UTC | #2
Ben,

Thank you for your comments. I understand this patch needs more work.
So I am going to remove it from this series for now and work on it in
isolation.

Regards,

On Thu, Nov 3, 2011 at 8:46 PM, Ben Hutchings <bhutchings@solarflare.com> wrote:
> On Thu, 2011-11-03 at 18:41 -0700, David Decotigny wrote:
>> From: Salman Qazi <sqazi@google.com>
>>
>> The race was between del_timer_sync and nv_do_stats_poll called through
>> nv_get_ethtool_stats.
>
> I don't think so.  nv_close() and nv_get_ethtool_stats() are both called
> with RTNL held.
>
> Calling the timer function from nv_get_ethtool_stats is very likely part
> of the problem though, so why don't you stop doing that?
>
> [...]
>> diff --git a/drivers/net/ethernet/nvidia/forcedeth.c b/drivers/net/ethernet/nvidia/forcedeth.c
>> index 0af12a8..7996782 100644
>> --- a/drivers/net/ethernet/nvidia/forcedeth.c
>> +++ b/drivers/net/ethernet/nvidia/forcedeth.c
>> @@ -3937,6 +3937,10 @@ static void nv_poll_controller(struct net_device *dev)
>>  }
>>  #endif
>>
>> +/* No locking is needed as long as this is in the timer
>> + * callback.  However, any other callers must call this
>> + * function with np->lock held.
>> + */
>
> So long as this function is used by all of (1) the timer function (2)
> the ndo_get_stats implementation (3) the ethtool get_stats
> implementation, it can most certainly be called concurrently on multiple
> processors.
>
> You could have (2) and (3) return the last polled stats and not poll the
> hardware themselves, but you would need to use the functions from
> <linux/u64_stats_sync.h> to avoid word-tearing on 32-bit architectures.
>
>>  static void nv_do_stats_poll(unsigned long data)
>>  {
>>         struct net_device *dev = (struct net_device *) data;
>> @@ -4589,12 +4593,17 @@ static int nv_get_sset_count(struct net_device *dev, int sset)
>>
>>  static void nv_get_ethtool_stats(struct net_device *dev, struct ethtool_stats *estats, u64 *buffer)
>>  {
>> +       unsigned long flags;
>>         struct fe_priv *np = netdev_priv(dev);
>>
>> +       spin_lock_irqsave(&np->lock, flags);
>> +
>>         /* update stats */
>>         nv_do_stats_poll((unsigned long)dev);
>>
>>         memcpy(buffer, &np->estats, nv_get_sset_count(dev, ETH_SS_STATS)*sizeof(u64));
>> +
>> +       spin_unlock_irqrestore(&np->lock, flags);
>
> This function is not called from interrupt context.
>
>>  }
>>
>>  static int nv_link_test(struct net_device *dev)
>> @@ -5189,13 +5198,13 @@ static int nv_close(struct net_device *dev)
>>
>>         spin_lock_irq(&np->lock);
>>         np->in_shutdown = 1;
>> +       del_timer_sync(&np->stats_poll);
>>         spin_unlock_irq(&np->lock);
>>         nv_napi_disable(dev);
>>         synchronize_irq(np->pci_dev->irq);
>>
>>         del_timer_sync(&np->oom_kick);
>>         del_timer_sync(&np->nic_poll);
>> -       del_timer_sync(&np->stats_poll);
>>
>>         netif_stop_queue(dev);
>>         spin_lock_irq(&np->lock);
>
> I don't believe this code movement is helpful.
>
> Ben.
>
> --
> Ben Hutchings, Staff Engineer, Solarflare
> Not speaking for my employer; that's the marketing department's job.
> They asked us to note that Solarflare product names are trademarked.
>
>
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
david decotigny Nov. 5, 2011, 10:16 p.m. UTC | #3
Hello,

Thank you for your feedback, Ben. I looked at this patch more carefully:

On Thu, Nov 3, 2011 at 8:46 PM, Ben Hutchings <bhutchings@solarflare.com> wrote:
> On Thu, 2011-11-03 at 18:41 -0700, David Decotigny wrote:
>> From: Salman Qazi <sqazi@google.com>
>>
>> The race was between del_timer_sync and nv_do_stats_poll called through
>> nv_get_ethtool_stats.
>
> I don't think so.  nv_close() and nv_get_ethtool_stats() are both called
> with RTNL held.
>
> Calling the timer function from nv_get_ethtool_stats is very likely part
> of the problem though, so why don't you stop doing that?

Right. As the initial author noted, the problem is presumably that
mod_timer was called after del_timer_sync, from a non-timer path
(which can only be via nv_get_ethtool_stats in our case). As you
noted, it's enough to ensure this path doesn't exist, which is easy to
do here and doesn't require synchro. I'll send an interim patch for
that to netdev (it should fix the race but will have the same
shortcomings as current code wrt 64b-correctness on 32b hosts).

When switching to the ndo_get_stats64 api, I will make sure
u64_stats_sync.h is used. This is for another patch series scheduled
later for net-next.

>> @@ -5189,13 +5198,13 @@ static int nv_close(struct net_device *dev)
>>
>>         spin_lock_irq(&np->lock);
>>         np->in_shutdown = 1;
>> +       del_timer_sync(&np->stats_poll);
>>         spin_unlock_irq(&np->lock);
>>         nv_napi_disable(dev);
>>         synchronize_irq(np->pci_dev->irq);
>>
>>         del_timer_sync(&np->oom_kick);
>>         del_timer_sync(&np->nic_poll);
>> -       del_timer_sync(&np->stats_poll);
>>
>>         netif_stop_queue(dev);
>>         spin_lock_irq(&np->lock);
>
> I don't believe this code movement is helpful.

I agree.

Regards,
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/net/ethernet/nvidia/forcedeth.c b/drivers/net/ethernet/nvidia/forcedeth.c
index 0af12a8..7996782 100644
--- a/drivers/net/ethernet/nvidia/forcedeth.c
+++ b/drivers/net/ethernet/nvidia/forcedeth.c
@@ -3937,6 +3937,10 @@  static void nv_poll_controller(struct net_device *dev)
 }
 #endif
 
+/* No locking is needed as long as this is in the timer
+ * callback.  However, any other callers must call this
+ * function with np->lock held.
+ */
 static void nv_do_stats_poll(unsigned long data)
 {
 	struct net_device *dev = (struct net_device *) data;
@@ -4589,12 +4593,17 @@  static int nv_get_sset_count(struct net_device *dev, int sset)
 
 static void nv_get_ethtool_stats(struct net_device *dev, struct ethtool_stats *estats, u64 *buffer)
 {
+	unsigned long flags;
 	struct fe_priv *np = netdev_priv(dev);
 
+	spin_lock_irqsave(&np->lock, flags);
+
 	/* update stats */
 	nv_do_stats_poll((unsigned long)dev);
 
 	memcpy(buffer, &np->estats, nv_get_sset_count(dev, ETH_SS_STATS)*sizeof(u64));
+
+	spin_unlock_irqrestore(&np->lock, flags);
 }
 
 static int nv_link_test(struct net_device *dev)
@@ -5189,13 +5198,13 @@  static int nv_close(struct net_device *dev)
 
 	spin_lock_irq(&np->lock);
 	np->in_shutdown = 1;
+	del_timer_sync(&np->stats_poll);
 	spin_unlock_irq(&np->lock);
 	nv_napi_disable(dev);
 	synchronize_irq(np->pci_dev->irq);
 
 	del_timer_sync(&np->oom_kick);
 	del_timer_sync(&np->nic_poll);
-	del_timer_sync(&np->stats_poll);
 
 	netif_stop_queue(dev);
 	spin_lock_irq(&np->lock);