netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Florian Westphal <fw@strlen.de>
To: <netdev@vger.kernel.org>
Cc: peterz@infradead.org
Subject: rtnetlink: fix dump+module unload races, take 2
Date: Mon,  6 Nov 2017 11:51:05 +0100	[thread overview]
Message-ID: <20171106105113.20476-1-fw@strlen.de> (raw)

Peter Zijlstra reported:
----------
  I just ran across commit:
  019a316992ee ("rtnetlink: add reference counting to prevent module unload while dump is in progress")
  And that commit is _completely_ broken.

  1) it not in fact a refcount, so using refcount_t is silly
  2) there is a distinct lack of memory barriers, so we can easily
     observe the decrement while the msg_handler is still in progress.
  3) waiting with a schedule()/yield() loop is complete crap and subject
     life-locks, imagine doing that rtnl_unregister_all() from a RT task.
----------

Moreover, the commit doesn't work.

Problem is that netlink keeps the dump function address in the control
block to resume dumps, i.e. when netlink_dump_start() returns then
we may not be done with the dump, it can be restarted on next recv.

netlink already has a mechanism to prevent module unload while
dumps are in progress: netlink_dump_control struct contains a pointer
to struct module, and netlink will take care of module_get/put as needed
if that is set.

To make use of this however we need to store pointer to module that
registered the particular dump function.  This series does that.

First, revert the broken commit to start from scratch.
Second patch adds new rtnl_register_module() that takes pointer to
the owning module.

Rest of patches convert affected modules.
IPv6 is not converted as ipv6 module cannot be unloaded.

One alternative to this series would be to decrement
rtnl_msg_handlers_ref count via netlink_dump_control->done(), but that
doesn't address any of Peters points.

I am submitting this for -next for 2 reasons:
1. This problem has existed for so long its evidently not a big deal
2. I think its a bad idea to cram this in at last second in current
development cycle.

ps: I will be travelling, replies will be delayed.

             reply	other threads:[~2017-11-06 10:51 UTC|newest]

Thread overview: 20+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-11-06 10:51 Florian Westphal [this message]
2017-11-06 10:51 ` [PATCH net-next 1/8] rtnetlink: Revert "rtnetlink: add reference counting to prevent module unload while dump is in progress" Florian Westphal
2017-11-06 10:51 ` [PATCH net-next 2/8] rtnetlink: add rtnl_register_module Florian Westphal
2017-11-06 12:44   ` Peter Zijlstra
2017-11-07  6:11     ` Florian Westphal
2017-11-07  9:10       ` Peter Zijlstra
2017-11-07  9:24         ` Peter Zijlstra
2017-11-07  9:47           ` Florian Westphal
2017-11-07 14:57             ` Peter Zijlstra
2017-11-08  1:27               ` Stephen Hemminger
2017-11-13  7:21               ` Florian Westphal
2017-11-13  7:55                 ` Peter Zijlstra
2017-11-13  7:59                   ` Florian Westphal
2017-11-07  9:43         ` Florian Westphal
2017-11-06 10:51 ` [PATCH net-next 3/8] qtr: use rtnl_register_module Florian Westphal
2017-11-06 10:51 ` [PATCH net-next 4/8] bridge: " Florian Westphal
2017-11-06 10:51 ` [PATCH net-next 5/8] can: " Florian Westphal
2017-11-06 10:51 ` [PATCH net-next 6/8] decnet: " Florian Westphal
2017-11-06 10:51 ` [PATCH net-next 7/8] phonet: " Florian Westphal
2017-11-06 10:51 ` [PATCH net-next 8/8] mpls: " Florian Westphal

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20171106105113.20476-1-fw@strlen.de \
    --to=fw@strlen.de \
    --cc=netdev@vger.kernel.org \
    --cc=peterz@infradead.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).