br_port_get_rtnl() can return NULL
Signed-off-by: Laszlo Toth <laszlth at gmail.com>
---
net/bridge/br_netlink.c | 12 ++++++++++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
index 015f465c..cbec11f 100644
--- a/net/bridge/br_netlink.c
+++ b/net/bridge/br_netlink.c
@@ -939,14 +939,17 @@ static int br_port_slave_changelink(struct net_device
*brdev,
struct nlattr *data[],
struct netlink_ext_ack *extack)
{
+ struct net_bridge_port *port = br_port_get_rtnl(dev);
struct net_bridge *br = netdev_priv(brdev);
int ret;
if (!data)
return 0;
+ if (!port)
+ return -EINVAL;
spin_lock_bh(&br->lock);
- ret = br_setport(br_port_get_rtnl(dev), data);
+ ret = br_setport(port, data);
spin_unlock_bh(&br->lock);
return ret;
@@ -956,7 +959,12 @@ static int br_port_fill_slave_info(struct sk_buff *skb,
const struct net_device *brdev,
const struct net_device *dev)
{
- return br_port_fill_attrs(skb, br_port_get_rtnl(dev));
+ struct net_bridge_port *port = br_port_get_rtnl(dev);
+
+ if (!port)
+ return -EINVAL;
+
+ return br_port_fill_attrs(skb, port);
}
static size_t br_port_get_slave_size(const struct net_device *brdev,
--
2.7.4
Nikolay Aleksandrov
2018-Apr-08 22:25 UTC
[Bridge] [PATCH] net: bridge: add missing NULL checks
On 08/04/18 20:49, Laszlo Toth wrote:> br_port_get_rtnl() can return NULL > > Signed-off-by: Laszlo Toth <laszlth at gmail.com> > --- > net/bridge/br_netlink.c | 12 ++++++++++-- > 1 file changed, 10 insertions(+), 2 deletions(-) >Nacked-by: Nikolay Aleksandrov <nikolay at cumulusnetworks.com> More below.> diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c > index 015f465c..cbec11f 100644 > --- a/net/bridge/br_netlink.c > +++ b/net/bridge/br_netlink.c > @@ -939,14 +939,17 @@ static int br_port_slave_changelink(struct net_device *brdev, > struct nlattr *data[], > struct netlink_ext_ack *extack) > { > + struct net_bridge_port *port = br_port_get_rtnl(dev); > struct net_bridge *br = netdev_priv(brdev); > int ret; > > if (!data) > return 0; > + if (!port) > + return -EINVAL; >If we're here, it means the master device of dev is a bridge => dev is a bridge port, since we're running with RTNL that cannot change, so this check is unnecessary. Have you actually hit a bug with this code ?> spin_lock_bh(&br->lock); > - ret = br_setport(br_port_get_rtnl(dev), data); > + ret = br_setport(port, data); > spin_unlock_bh(&br->lock); > > return ret; > @@ -956,7 +959,12 @@ static int br_port_fill_slave_info(struct sk_buff *skb, > const struct net_device *brdev, > const struct net_device *dev) > { > - return br_port_fill_attrs(skb, br_port_get_rtnl(dev)); > + struct net_bridge_port *port = br_port_get_rtnl(dev); > + > + if (!port) > + return -EINVAL; > + > + return br_port_fill_attrs(skb, port);Same rationale here, fill_slave_info is called via a master device's ops under RTNL, which means dev is a bridge port and that also cannot change. If you have hit a bug with this code, can we see the trace ? The problem might be elsewhere. Thanks, Nik> } > > static size_t br_port_get_slave_size(const struct net_device *brdev, >