lkml.org 
[lkml]   [2013]   [Apr]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    From
    Subject[PATCH 079/102] net: add a synchronize_net() in netdev_rx_handler_unregister()
    Date
    3.5.7.10 -stable review patch.  If anyone has any objections, please let me know.

    ------------------

    From: Eric Dumazet <edumazet@google.com>

    commit 00cfec37484761a44a3b6f4675a54caa618210ae upstream.

    commit 35d48903e97819 (bonding: fix rx_handler locking) added a race
    in bonding driver, reported by Steven Rostedt who did a very good
    diagnosis :

    <quoting Steven>

    I'm currently debugging a crash in an old 3.0-rt kernel that one of our
    customers is seeing. The bug happens with a stress test that loads and
    unloads the bonding module in a loop (I don't know all the details as
    I'm not the one that is directly interacting with the customer). But the
    bug looks to be something that may still be present and possibly present
    in mainline too. It will just be much harder to trigger it in mainline.

    In -rt, interrupts are threads, and can schedule in and out just like
    any other thread. Note, mainline now supports interrupt threads so this
    may be easily reproducible in mainline as well. I don't have the ability
    to tell the customer to try mainline or other kernels, so my hands are
    somewhat tied to what I can do.

    But according to a core dump, I tracked down that the eth irq thread
    crashed in bond_handle_frame() here:

    slave = bond_slave_get_rcu(skb->dev);
    bond = slave->bond; <--- BUG

    the slave returned was NULL and accessing slave->bond caused a NULL
    pointer dereference.

    Looking at the code that unregisters the handler:

    void netdev_rx_handler_unregister(struct net_device *dev)
    {

    ASSERT_RTNL();
    RCU_INIT_POINTER(dev->rx_handler, NULL);
    RCU_INIT_POINTER(dev->rx_handler_data, NULL);
    }

    Which is basically:
    dev->rx_handler = NULL;
    dev->rx_handler_data = NULL;

    And looking at __netif_receive_skb() we have:

    rx_handler = rcu_dereference(skb->dev->rx_handler);
    if (rx_handler) {
    if (pt_prev) {
    ret = deliver_skb(skb, pt_prev, orig_dev);
    pt_prev = NULL;
    }
    switch (rx_handler(&skb)) {

    My question to all of you is, what stops this interrupt from happening
    while the bonding module is unloading? What happens if the interrupt
    triggers and we have this:

    CPU0 CPU1
    ---- ----
    rx_handler = skb->dev->rx_handler

    netdev_rx_handler_unregister() {
    dev->rx_handler = NULL;
    dev->rx_handler_data = NULL;

    rx_handler()
    bond_handle_frame() {
    slave = skb->dev->rx_handler;
    bond = slave->bond; <-- NULL pointer dereference!!!

    What protection am I missing in the bond release handler that would
    prevent the above from happening?

    </quoting Steven>

    We can fix bug this in two ways. First is adding a test in
    bond_handle_frame() and others to check if rx_handler_data is NULL.

    A second way is adding a synchronize_net() in
    netdev_rx_handler_unregister() to make sure that a rcu protected reader
    has the guarantee to see a non NULL rx_handler_data.

    The second way is better as it avoids an extra test in fast path.

    Reported-by: Steven Rostedt <rostedt@goodmis.org>
    Signed-off-by: Eric Dumazet <edumazet@google.com>
    Cc: Jiri Pirko <jpirko@redhat.com>
    Cc: Paul E. McKenney <paulmck@us.ibm.com>
    Acked-by: Steven Rostedt <rostedt@goodmis.org>
    Reviewed-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
    Signed-off-by: David S. Miller <davem@davemloft.net>
    Signed-off-by: Luis Henriques <luis.henriques@canonical.com>
    ---
    net/core/dev.c | 6 ++++++
    1 file changed, 6 insertions(+)

    diff --git a/net/core/dev.c b/net/core/dev.c
    index 39aa9e6..845a83a 100644
    --- a/net/core/dev.c
    +++ b/net/core/dev.c
    @@ -3121,6 +3121,7 @@ int netdev_rx_handler_register(struct net_device *dev,
    if (dev->rx_handler)
    return -EBUSY;

    + /* Note: rx_handler_data must be set before rx_handler */
    rcu_assign_pointer(dev->rx_handler_data, rx_handler_data);
    rcu_assign_pointer(dev->rx_handler, rx_handler);

    @@ -3141,6 +3142,11 @@ void netdev_rx_handler_unregister(struct net_device *dev)

    ASSERT_RTNL();
    RCU_INIT_POINTER(dev->rx_handler, NULL);
    + /* a reader seeing a non NULL rx_handler in a rcu_read_lock()
    + * section has a guarantee to see a non NULL rx_handler_data
    + * as well.
    + */
    + synchronize_net();
    RCU_INIT_POINTER(dev->rx_handler_data, NULL);
    }
    EXPORT_SYMBOL_GPL(netdev_rx_handler_unregister);
    --
    1.8.1.2


    \
     
     \ /
      Last update: 2013-04-08 14:43    [W:4.991 / U:0.268 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site