RE: [PATCH net/next] bridge:Add rcu read lock when delete br port

[Lichunhe]

>-----Original Message-----
>From: Michael S. Tsirkin [mailto:mst@xxxxxxxxxx]
>Sent: Monday, August 04, 2014 8:41 PM
>To: Lichunhe
>Cc: vyasevic@xxxxxxxxxx; xiyou.wangcong@xxxxxxxxx;
>makita.toshiaki@xxxxxxxxxxxxx; stephen@xxxxxxxxxxxxxxxxxx;
>ebiederm@xxxxxxxxxxxx; f.fainelli@xxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
>Wuyunfei; Qianhuibin (Huibin QIAN, Euler)
>Subject: Re: [PATCH net/next] bridge:Add rcu read lock when delete br port
>
>On Mon, Aug 04, 2014 at 11:37:56AM +0800, lichunhe@xxxxxxxxxx wrote:
>> From: Chunhe Li <lichunhe@xxxxxxxxxx>
>>
>> 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);
>

Yes, this var "p" is NULL when the bug happened.

>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 <lichunhe@xxxxxxxxxx>
>
>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.
>
>

Maybe I understand wrong, please ignore this patch, do you have better way to solve this problem?

>> --
>> 1.9.2.0
>>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/