From mboxrd@z Thu Jan 1 00:00:00 1970 From: Nikolay Aleksandrov Subject: Re: [PATCH] net: bridge: add missing NULL checks Date: Mon, 9 Apr 2018 01:25:41 +0300 Message-ID: <3db78abd-bc13-528f-9da3-cb9a0ce0f617@cumulusnetworks.com> References: <20180408174934.GA3895@laszlth> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Cc: netdev@vger.kernel.org, bridge@lists.linux-foundation.org To: Laszlo Toth , Stephen Hemminger , "David S. Miller" Return-path: In-Reply-To: <20180408174934.GA3895@laszlth> Content-Language: en-US List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: bridge-bounces@lists.linux-foundation.org Errors-To: bridge-bounces@lists.linux-foundation.org List-Id: netdev.vger.kernel.org On 08/04/18 20:49, Laszlo Toth wrote: > br_port_get_rtnl() can return NULL > > Signed-off-by: Laszlo Toth > --- > net/bridge/br_netlink.c | 12 ++++++++++-- > 1 file changed, 10 insertions(+), 2 deletions(-) > Nacked-by: Nikolay Aleksandrov 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, >