netdev - RE: [PATCH net 1/2] devlink: Hold rtnl lock while reading netdev attributes

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: <BY5PR12MB43224995BFBAE791FE75552ADCFA0@BY5PR12MB4322.namprd12.prod.outlook.com>
Date:   Wed, 25 Nov 2020 07:13:40 +0000
From:   Parav Pandit <parav@...dia.com>
To:     Jakub Kicinski <kuba@...nel.org>
CC:     "davem@...emloft.net" <davem@...emloft.net>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Jiri Pirko <jiri@...dia.com>
Subject: RE: [PATCH net 1/2] devlink: Hold rtnl lock while reading netdev
 attributes



> From: Jakub Kicinski <kuba@...nel.org>
> Sent: Wednesday, November 25, 2020 3:59 AM
> 
> On Sun, 22 Nov 2020 08:12:56 +0200 Parav Pandit wrote:
> > A netdevice of a devlink port can be moved to different net namespace
> > than its parent devlink instance.
> > This scenario occurs when devlink reload is not used for maintaining
> > backward compatibility.
> >
> > When netdevice is undergoing migration to net namespace, its ifindex
> > and name may change.
> >
> > In such use case, devlink port query may read stale netdev attributes.
> >
> > Fix it by reading them under rtnl lock.
> >
> > Fixes: bfcd3a466172 ("Introduce devlink infrastructure")
> > Signed-off-by: Parav Pandit <parav@...dia.com>
> > ---
> >  net/core/devlink.c | 30 ++++++++++++++++++++++++------
> >  1 file changed, 24 insertions(+), 6 deletions(-)
> >
> > diff --git a/net/core/devlink.c b/net/core/devlink.c index
> > acc29d5157f4..6135ef5972ce 100644
> > --- a/net/core/devlink.c
> > +++ b/net/core/devlink.c
> > @@ -775,6 +775,23 @@ devlink_nl_port_function_attrs_put(struct sk_buff
> *msg, struct devlink_port *por
> >  	return err;
> >  }
> >
> > +static int devlink_nl_port_netdev_fill(struct sk_buff *msg, struct
> > +devlink_port *devlink_port) {
> > +	struct net_device *netdev = devlink_port->type_dev;
> > +	int err;
> > +
> > +	ASSERT_RTNL();
> > +	if (!netdev)
> > +		return 0;
> > +
> > +	err = nla_put_u32(msg, DEVLINK_ATTR_PORT_NETDEV_IFINDEX,
> > +netdev->ifindex);
> 
> The line wrapping was correct, please keep in under 80. Please tell your colleges
> at Mellanox.
> 
> > +	if (err)
> > +		goto done;
> 
> 	return err;
> 
> > +	err = nla_put_string(msg, DEVLINK_ATTR_PORT_NETDEV_NAME,
> > +netdev->name);
> 
> 	return nla_put_...
> 
> > +done:
> > +	return err;
> > +}
> > +
> >  static int devlink_nl_port_fill(struct sk_buff *msg, struct devlink *devlink,
> >  				struct devlink_port *devlink_port,
> >  				enum devlink_command cmd, u32 portid, @@ -
> 792,6 +809,8 @@ static
> > int devlink_nl_port_fill(struct sk_buff *msg, struct devlink *devlink,
> >  	if (nla_put_u32(msg, DEVLINK_ATTR_PORT_INDEX, devlink_port-
> >index))
> >  		goto nla_put_failure;
> >
> > +	/* Hold rtnl lock while accessing port's netdev attributes. */
> > +	rtnl_lock();
> >  	spin_lock_bh(&devlink_port->type_lock);
> >  	if (nla_put_u16(msg, DEVLINK_ATTR_PORT_TYPE, devlink_port->type))
> >  		goto nla_put_failure_type_locked;
> > @@ -800,13 +819,10 @@ static int devlink_nl_port_fill(struct sk_buff *msg,
> struct devlink *devlink,
> >  			devlink_port->desired_type))
> >  		goto nla_put_failure_type_locked;
> >  	if (devlink_port->type == DEVLINK_PORT_TYPE_ETH) {
> > -		struct net_device *netdev = devlink_port->type_dev;
> > +		int err;
> 
> What's the point of this local variable?
> 
I will avoid refactor for now, so above comment doesn't need to be addressed.
> > -		if (netdev &&
> > -		    (nla_put_u32(msg, DEVLINK_ATTR_PORT_NETDEV_IFINDEX,
> > -				 netdev->ifindex) ||
> > -		     nla_put_string(msg, DEVLINK_ATTR_PORT_NETDEV_NAME,
> > -				    netdev->name)))
> > +		err = devlink_nl_port_netdev_fill(msg, devlink_port);
> > +		if (err)
> 
> just put the call in the if ()
Ok.
> 
> >  			goto nla_put_failure_type_locked;
> >  	}
> >  	if (devlink_port->type == DEVLINK_PORT_TYPE_IB) {
> 
> 
> Honestly this patch is doing too much for a fix.
> 
> All you need is the RTNL lock and then add:
> 
Ok. I will differ the refactor to later point.

> +               struct net *net = devlink_net(devlink_port->devlink);
>                 struct net_device *netdev = devlink_port->type_dev;
> 
>                 if (netdev &&
> +                   net_eq(net, dev_net(netdev)) &&
>                     (nla_put_u32(msg, DEVLINK_ATTR_PORT_NETDEV_IFINDEX,
>                                  netdev->ifindex) ||
>                      nla_put_string(msg, DEVLINK_ATTR_PORT_NETDEV_NAME,
> 
> 
> You can do refactoring later in net-next. Maybe even add a check that drivers
> which support reload set namespace local on their netdevs.
This will break the backward compatibility as orchestration for VFs are not using devlink reload, which is supported very recently.
But yes, for SF who doesn't have backward compatibility issue, as soon as initial series is merged, I will mark it as local, so that orchestration doesn't start on wrong foot.