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

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <ED104EF999716D4C84ED5EA6FAB8EEE128EF9725@SZXEMA509-MBX.china.huawei.com>
Date:	Tue, 5 Aug 2014 00:43:09 +0000
From:	Lichunhe <lichunhe@...wei.com>
To:	"Michael S. Tsirkin" <mst@...hat.com>
CC:	"vyasevic@...hat.com" <vyasevic@...hat.com>,
	"xiyou.wangcong@...il.com" <xiyou.wangcong@...il.com>,
	"makita.toshiaki@....ntt.co.jp" <makita.toshiaki@....ntt.co.jp>,
	"stephen@...workplumber.org" <stephen@...workplumber.org>,
	"ebiederm@...ssion.com" <ebiederm@...ssion.com>,
	"f.fainelli@...il.com" <f.fainelli@...il.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Wuyunfei <wuyunfei@...wei.com>,
	"Qianhuibin (Huibin QIAN, Euler)" <qianhuibin@...wei.com>
Subject: RE: [PATCH net/next] bridge:Add rcu read lock when delete br port

>-----Original Message-----
>From: Michael S. Tsirkin [mailto:mst@...hat.com]
>Sent: Monday, August 04, 2014 8:41 PM
>To: Lichunhe
>Cc: vyasevic@...hat.com; xiyou.wangcong@...il.com;
>makita.toshiaki@....ntt.co.jp; stephen@...workplumber.org;
>ebiederm@...ssion.com; f.fainelli@...il.com; linux-kernel@...r.kernel.org;
>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@...wei.com wrote:
>> From: Chunhe Li <lichunhe@...wei.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);
>

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@...wei.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.
>
>

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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/