lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Date:	Sat, 5 Nov 2011 15:16:44 -0700
From:	David Decotigny <david.decotigny@...gle.com>
To:	Ben Hutchings <bhutchings@...arflare.com>,
	Salman Qazi <sqazi@...gle.com>
Cc:	netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
	"David S. Miller" <davem@...emloft.net>,
	Ian Campbell <ian.campbell@...rix.com>,
	Eric Dumazet <eric.dumazet@...il.com>,
	Jeff Kirsher <jeffrey.t.kirsher@...el.com>,
	Jiri Pirko <jpirko@...hat.com>, Joe Perches <joe@...ches.com>,
	Szymon Janc <szymon@...c.net.pl>
Subject: Re: [PATCH net v2 6/8] forcedeth: Fix a race during rmmod of forcedeth

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@...arflare.com> wrote:
> On Thu, 2011-11-03 at 18:41 -0700, David Decotigny wrote:
>> From: Salman Qazi <sqazi@...gle.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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists