On Mon, Aug 04, 2014 at 11:37:56AM +0800, lichu...@huawei.com wrote:
> From: Chunhe Li <lichu...@huawei.com>
>
> In the br_hanle_frame function has a bug, when the bridge receive packets
> which go througth the br_handle_frame, get the net_bridge_port pointer "p",
> but don't check NULL pointer to use it. If somebody delete the bridge port
> at the same time, will call a NULL pointer, trigger kernel panic. I see the
> del_nbp comments, call del_nbp should via RCU, but the caller don't do this.
I don't see such a comment there.
Are you talking about this line:
p = br_port_get_rcu(skb->dev);
this is actually rx_handler_data.
The reason it should not be NULL is
explained here:
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);
> following steps will make bug happened
> 1.start vm and add the vm interface to a bridge br0,for example,
> brctl addbr br0 tap0
>
> 2.configuer vm interface and br0 same ip subnet, vm ping br0.
>
> 3.add and delete the vm interface port for endless loop.
>
> Signed-off-by: Chunhe Li <lichu...@huawei.com>
OK but apparently something else triggered the bug here.
It might be a good idea to enable lockdep and rcu checks
see if anything suspicious is reported.
> ---
> net/bridge/br_if.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
> index 3eca3fd..91c611d 100644
> --- a/net/bridge/br_if.c
> +++ b/net/bridge/br_if.c
> @@ -274,9 +274,11 @@ void br_dev_delete(struct net_device *dev, struct
> list_head *head)
> struct net_bridge *br = netdev_priv(dev);
> struct net_bridge_port *p, *n;
>
> + rcu_read_lock();
> list_for_each_entry_safe(p, n, &br->port_list, list) {
> del_nbp(p);
> }
> + rcu_read_unlock();
>
> br_fdb_delete_by_port(br, NULL, 1);
>
> @@ -550,7 +552,9 @@ int br_del_if(struct net_bridge *br, struct net_device
> *dev)
> * there still maybe an alternate path for netconsole to use;
> * therefore there is no reason for a NETDEV_RELEASE event.
> */
> + rcu_read_lock();
> del_nbp(p);
> + rcu_read_unlock();
>
> spin_lock_bh(&br->lock);
> changed_addr = br_stp_recalculate_bridge_id(br);
Does the problem disappear with this applied?
I don't see how this would help. rcu locks do not synchronize
against other readers.
> --
> 1.9.2.0
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
