[PATCH net 0/2] devlink port attribute fixes

[PATCH net 0/2] devlink port attribute fixes

[Parav Pandit]

This patchset contains 2 small fixes for devlink port attributes.

Patch summary:
Patch-1 synchronize the devlink port attribute reader
        with net namespace change operation
Patch-2 Ensure to return devlink port's netdevice attributes
        when netdev and devlink instance belong to same net namespace

Parav Pandit (2):
  devlink: Hold rtnl lock while reading netdev attributes
  devlink: Make sure devlink instance and port are in same net namespace

 net/core/devlink.c | 33 +++++++++++++++++++++++++++------
 1 file changed, 27 insertions(+), 6 deletions(-)

-- 
2.26.2

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

[Parav Pandit]

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@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);
+	if (err)
+		goto done;
+	err = nla_put_string(msg, DEVLINK_ATTR_PORT_NETDEV_NAME, netdev->name);
+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;
 
-		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)
 			goto nla_put_failure_type_locked;
 	}
 	if (devlink_port->type == DEVLINK_PORT_TYPE_IB) {
@@ -818,6 +834,7 @@ static int devlink_nl_port_fill(struct sk_buff *msg, struct devlink *devlink,
 			goto nla_put_failure_type_locked;
 	}
 	spin_unlock_bh(&devlink_port->type_lock);
+	rtnl_unlock();
 	if (devlink_nl_port_attrs_put(msg, devlink_port))
 		goto nla_put_failure;
 	if (devlink_nl_port_function_attrs_put(msg, devlink_port, extack))
@@ -828,6 +845,7 @@ static int devlink_nl_port_fill(struct sk_buff *msg, struct devlink *devlink,
 
 nla_put_failure_type_locked:
 	spin_unlock_bh(&devlink_port->type_lock);
+	rtnl_unlock();
 nla_put_failure:
 	genlmsg_cancel(msg, hdr);
 	return -EMSGSIZE;
-- 
2.26.2

[PATCH net 2/2] devlink: Make sure devlink instance and port are in same net namespace

[Parav Pandit]

When devlink reload operation is not used, netdev of an Ethernet port
may be present in different net namespace than the net namespace of the
devlink instance.

Ensure that both the devlink instance and devlink port netdev are
located in same net namespace.

Fixes: 070c63f20f6c ("net: devlink: allow to change namespaces during reload")
Signed-off-by: Parav Pandit <parav@nvidia.com>
---
 net/core/devlink.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/net/core/devlink.c b/net/core/devlink.c
index 6135ef5972ce..2e1b0e82a102 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -778,12 +778,15 @@ devlink_nl_port_function_attrs_put(struct sk_buff *msg, struct devlink_port *por
 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;
+	int err = 0;
 
 	ASSERT_RTNL();
 	if (!netdev)
 		return 0;
 
+	if (!net_eq(devlink_net(devlink_port->devlink), dev_net(netdev)))
+		goto done;
+
 	err = nla_put_u32(msg, DEVLINK_ATTR_PORT_NETDEV_IFINDEX, netdev->ifindex);
 	if (err)
 		goto done;
-- 
2.26.2

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

[Jakub Kicinski]

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@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.

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

[Parav Pandit]

> From: Jakub Kicinski <kuba@kernel.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@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?
> 
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.

[PATCH net v2 0/2] devlink port attribute fixes

[Parav Pandit]

This patchset contains 2 small fixes for devlink port attributes.

Patch summary:
Patch-1 synchronize the devlink port attribute reader
        with net namespace change operation
Patch-2 Ensure to return devlink port's netdevice attributes
        when netdev and devlink instance belong to same net namespace

---
Changelog:
v1->v2:
 - avoided refactoring the port fill attribute routine


Parav Pandit (2):
  devlink: Hold rtnl lock while reading netdev attributes
  devlink: Make sure devlink instance and port are in same net namespace

 net/core/devlink.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

-- 
2.26.2

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

[Parav Pandit]

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.

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@nvidia.com>
---
Changelog:
v1->v2:
 - avoided refactoring the code
 - updated commit log

---
 net/core/devlink.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/net/core/devlink.c b/net/core/devlink.c
index acc29d5157f4..04ae67bbe19c 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -792,6 +792,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;
@@ -818,6 +820,7 @@ static int devlink_nl_port_fill(struct sk_buff *msg, struct devlink *devlink,
 			goto nla_put_failure_type_locked;
 	}
 	spin_unlock_bh(&devlink_port->type_lock);
+	rtnl_unlock();
 	if (devlink_nl_port_attrs_put(msg, devlink_port))
 		goto nla_put_failure;
 	if (devlink_nl_port_function_attrs_put(msg, devlink_port, extack))
@@ -828,6 +831,7 @@ static int devlink_nl_port_fill(struct sk_buff *msg, struct devlink *devlink,
 
 nla_put_failure_type_locked:
 	spin_unlock_bh(&devlink_port->type_lock);
+	rtnl_unlock();
 nla_put_failure:
 	genlmsg_cancel(msg, hdr);
 	return -EMSGSIZE;
-- 
2.26.2

[PATCH net v2 2/2] devlink: Make sure devlink instance and port are in same net namespace

[Parav Pandit]

When devlink reload operation is not used, netdev of an Ethernet port may
be present in different net namespace than the net namespace of the
devlink instance.

Ensure that both the devlink instance and devlink port netdev are located
in same net namespace.

Fixes: 070c63f20f6c ("net: devlink: allow to change namespaces during reload")
Signed-off-by: Parav Pandit <parav@nvidia.com>
---
Changelog:
v1->v2:
 - avoided refactor code from previous patch

---
 net/core/devlink.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/core/devlink.c b/net/core/devlink.c
index 04ae67bbe19c..28c45be7f1bf 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -802,9 +802,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 *net = devlink_net(devlink_port->devlink);
 		struct net_device *netdev = devlink_port->type_dev;
 
-		if (netdev &&
+		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,
-- 
2.26.2

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

[Jakub Kicinski]

On Wed, 25 Nov 2020 07:13:40 +0000 Parav Pandit wrote:
> > 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.

Ah, right, that will not work because of the shenanigans you guys play
with the uplink port. If all reprs are NETNS_LOCAL it'd not be an issue.

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

[Parav Pandit]

> From: Jakub Kicinski <kuba@kernel.org>
> Sent: Wednesday, November 25, 2020 10:00 PM
> 
> On Wed, 25 Nov 2020 07:13:40 +0000 Parav Pandit wrote:
> > > 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.
> 
> Ah, right, that will not work because of the shenanigans you guys play with
> the uplink port. If all reprs are NETNS_LOCAL it'd not be an issue.
I am not sure what secret are you talking about with uplink.
I am taking about the SF netdevice to have the NETNS_LOCAL not the SF rep.
SF rep anyway has NETNS_LOCAL set.

I do not follow your comment - 'that will not work'. Can you please explain?
Do you mean I should take care for SF's netdevice to have NETNS_LOCAL in first patchset or you mean setting NETNS_LOCAL for VF's Netdev will not work?
If its later, sure it will break the backward compatibility, so will not do as default.
But yes, SF I want to subsequently.

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

[Jakub Kicinski]

On Wed, 25 Nov 2020 17:21:41 +0000 Parav Pandit wrote:
> > From: Jakub Kicinski <kuba@kernel.org>
> > Sent: Wednesday, November 25, 2020 10:00 PM
> > 
> > On Wed, 25 Nov 2020 07:13:40 +0000 Parav Pandit wrote:  
> > > > 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.  
> > 
> > Ah, right, that will not work because of the shenanigans you guys play with
> > the uplink port. If all reprs are NETNS_LOCAL it'd not be an issue.  
> I am not sure what secret are you talking about with uplink.

I'm referring to Mellanox conflating PF with uplink. It's not a secret,
we argued about it in the past.

> I am taking about the SF netdevice to have the NETNS_LOCAL not the SF rep.
> SF rep anyway has NETNS_LOCAL set.

All reps build by mlx5e_build_rep_netdev() have NETNS_LOCAL.

> I do not follow your comment - 'that will not work'. Can you please explain?

My half-baked suggestion was to basically add a:

	WARN_ON(ops->reload_down && ops->reload_up &&
		!(netdev->priv & NETIF_F_NETNS_LOCAL));

to devlink_port_type_netdev_checks(). Given if device has a reload
callback devlink is the way to change netns. But yeah, we can't break
existing behavior so your uplink has to be movable and can't have
NETNS_LOCAL. IOW adding the WARN_ON() won't work.

Hope this is crystal clear now.

> Do you mean I should take care for SF's netdevice to have NETNS_LOCAL in first patchset or you mean setting NETNS_LOCAL for VF's Netdev will not work?
> If its later, sure it will break the backward compatibility, so will not do as default.

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

[Parav Pandit]

> From: Jakub Kicinski <kuba@kernel.org>
> Sent: Wednesday, November 25, 2020 11:12 PM
> 
> On Wed, 25 Nov 2020 17:21:41 +0000 Parav Pandit wrote:
> > > From: Jakub Kicinski <kuba@kernel.org>
> > > Sent: Wednesday, November 25, 2020 10:00 PM
> > >
> > > On Wed, 25 Nov 2020 07:13:40 +0000 Parav Pandit wrote:
> > > > > 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.
> > >
> > > Ah, right, that will not work because of the shenanigans you guys
> > > play with the uplink port. If all reprs are NETNS_LOCAL it'd not be an
> issue.
> > I am not sure what secret are you talking about with uplink.
> 
> I'm referring to Mellanox conflating PF with uplink. It's not a secret,
Ok.
> we argued about it in the past.
> 
> > I am taking about the SF netdevice to have the NETNS_LOCAL not the SF
> rep.
> > SF rep anyway has NETNS_LOCAL set.
> 
> All reps build by mlx5e_build_rep_netdev() have NETNS_LOCAL.
> 
Yes. this is clear to me and we are good here. 😊

> > I do not follow your comment - 'that will not work'. Can you please explain?
> 
> My half-baked suggestion was to basically add a:
> 
> 	WARN_ON(ops->reload_down && ops->reload_up &&
> 		!(netdev->priv & NETIF_F_NETNS_LOCAL));
> 
> to devlink_port_type_netdev_checks(). Given if device has a reload
> callback devlink is the way to change netns. But yeah, we can't break
> existing behavior so your uplink has to be movable and can't have
> NETNS_LOCAL. IOW adding the WARN_ON() won't work.
> 
Right.

> Hope this is crystal clear now.
Yes, its clear. Thanks.

I addressed your comments and cut down both the fixes to merely 7 lines change. Sent v2.

Re: [PATCH net v2 0/2] devlink port attribute fixes

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net.git (refs/heads/master):

On Wed, 25 Nov 2020 11:16:18 +0200 you wrote:
> This patchset contains 2 small fixes for devlink port attributes.
> 
> Patch summary:
> Patch-1 synchronize the devlink port attribute reader
>         with net namespace change operation
> Patch-2 Ensure to return devlink port's netdevice attributes
>         when netdev and devlink instance belong to same net namespace
> 
> [...]

Here is the summary with links:
  - [net,v2,1/2] devlink: Hold rtnl lock while reading netdev attributes
    https://git.kernel.org/netdev/net/c/b187c9b4178b
  - [net,v2,2/2] devlink: Make sure devlink instance and port are in same net namespace
    https://git.kernel.org/netdev/net/c/a7b43649507d

You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html