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 <pa...@nvidia.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?
> - 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 ()
> 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:
+ 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.
