netdev - Re: [PATCH 01/20] net: NETDEV_UNREGISTER_PERNET -> NETDEV_UNREGISTER

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 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <m1d42zoa0a.fsf@fess.ebiederm.org>
Date:	Mon, 30 Nov 2009 15:42:13 -0800
From:	ebiederm@...ssion.com (Eric W. Biederman)
To:	David Miller <davem@...emloft.net>
Cc:	netdev@...r.kernel.org, hadi@...erus.ca, dlezcano@...ibm.com,
	adobriyan@...il.com, kaber@...sh.net, opurdila@...acom.com
Subject: Re: [PATCH 01/20] net: NETDEV_UNREGISTER_PERNET -> NETDEV_UNREGISTER_BATCH

David Miller <davem@...emloft.net> writes:

> From: "Eric W. Biederman" <ebiederm@...ssion.com>
> Date: Sun, 29 Nov 2009 17:45:58 -0800
>
>> Additionally it appears that we moved the route cache flush after
>> the final synchronize_net, which seems wrong and there was no
>> explanation.  So I have restored the original location of the final
>> synchronize_net.
>
> I think the idea was that it's important for all pending stale
> RCU references to the device to be gone before we send out the
> notifier.  And that's why the synchronize_net() comes first.
>
> This needs more thought.

The only case the affects (today) is the invalidation of the route/flow cache.

The routing table until the addition of NETDEV_UNREGISTER_PERNET happened
in the notifier NETDEV_UNREGISTER, which comes before the final synchronize_net.

The routing cache has rcu references to the network device.

So it appears to me that with the current pernet batched routing code cleanup we
can end up calling kfree before all of the call_rcu functions triggered by
rt_do_flush will be run.

    netdev_run_todo()
       kobject_put()
         netdev_release()
           kfree()

NETDEV_UNREGISTER_PERCPU
    rt_cache_flush(xxx, 0)
       rt_do_flush()
         rt_free
           call_rcu

So it appears to me that moving the final syncrhonize_net() back where it was before
the NETDEV_UNREGISTER_PERNET changes (as I did) is necessary for correctness.

Eric
--
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