[Bridge] [PATCH net] bridge: shutdown bridge device before removing it

Nikolay Aleksandrov nikolay at cumulusnetworks.com
Mon Apr 24 11:01:10 UTC 2017

On 24/04/17 10:25, Xin Long wrote:
> During removing a bridge device, if the bridge is still up, a new mdb entry
> still can be added in br_multicast_add_group() after all mdb entries are
> removed in br_multicast_dev_del(). Like the path:
>   mld_ifc_timer_expire ->
>     mld_sendpack -> ...
>       br_multicast_rcv ->
>         br_multicast_add_group
> The new mp's timer will be set up. If the timer expires after the bridge
> is freed, it may cause use-after-free panic in br_multicast_group_expired.
> This can happen when ip link remove a bridge or destroy a netns with a
> bridge device inside.
> As we can see in br_del_bridge, brctl is also supposed to remove a bridge
> device after it's shutdown.
> This patch is to call dev_close at the beginning of br_dev_delete so that
> netif_running check in br_multicast_add_group can avoid this issue. But
> to keep consistent with before, it will not remove the IFF_UP check in
> br_del_bridge for brctl.
> Reported-by: Jianwen Ji <jiji at redhat.com>
> Signed-off-by: Xin Long <lucien.xin at gmail.com>
> ---
>  net/bridge/br_if.c | 2 ++
>  1 file changed, 2 insertions(+)

+CC bridge maintainers

I can see how this could happen, could you also provide the traceback ?

The patch looks good to me, actually I think it fixes another issue with
mcast stats where the percpu pointer can be accessed after it's freed if
an mcast packet can get sent via br->dev after the br_multicast_dev_del() call.
This is definitely stable material, if I'm not mistaken the issue is there since
the introduction of br_dev_delete:
commit e10177abf842
Author: Satish Ashok <sashok at cumulusnetworks.com>
Date:   Wed Jul 15 07:16:51 2015 -0700

    bridge: multicast: fix handling of temp and perm entries

Acked-by: Nikolay Aleksandrov <nikolay at cumulusnetworks.com>

More information about the Bridge mailing list