On 11/04/2022 16:38, Joachim Wiberg wrote:
> The br_setlink() function extracts the struct net_bridge pointer a bit
> sloppy. It's easy to interpret the code wrong. This patch attempts to
> clear things up a bit.
>
> Signed-off-by: Joachim Wiberg <troglobit@xxxxxxxxx>
> ---
> net/bridge/br_netlink.c | 10 ++++++----
> 1 file changed, 6 insertions(+), 4 deletions(-)
>
I think you can make it more straight-forward, remove the first br = netdev_priv
and do something like (completely untested):
...
struct net_bridge_port *p = NULL;
...
if (netif_is_bridge_master(dev)) {
br = netdev_priv(dev);
} else {
p = br_port_get_rtnl(dev);
if (WARN_ON(!p))
return -EINVAL;
br = p->br;
}
So br is always and only set in this block.
> diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
> index 7fca8ff13ec7..8f4297287b32 100644
> --- a/net/bridge/br_netlink.c
> +++ b/net/bridge/br_netlink.c
> @@ -1040,6 +1040,8 @@ int br_setlink(struct net_device *dev, struct nlmsghdr *nlh, u16 flags,
> return 0;
>
> p = br_port_get_rtnl(dev);
> + if (p)
> + br = p->br;
> /* We want to accept dev as bridge itself if the AF_SPEC
> * is set to see if someone is setting vlan info on the bridge
> */
> @@ -1055,17 +1057,17 @@ int br_setlink(struct net_device *dev, struct nlmsghdr *nlh, u16 flags,
> if (err)
> return err;
>
> - spin_lock_bh(&p->br->lock);
> + spin_lock_bh(&br->lock);
> err = br_setport(p, tb, extack);
> - spin_unlock_bh(&p->br->lock);
> + spin_unlock_bh(&br->lock);
> } else {
> /* Binary compatibility with old RSTP */
> if (nla_len(protinfo) < sizeof(u8))
> return -EINVAL;
>
> - spin_lock_bh(&p->br->lock);
> + spin_lock_bh(&br->lock);
> err = br_set_port_state(p, nla_get_u8(protinfo));
> - spin_unlock_bh(&p->br->lock);
> + spin_unlock_bh(&br->lock);
> }
> if (err)
> goto out;
