* [PATCH net 0/2] devlink port attribute fixes
@ 2020-11-22 6:12 Parav Pandit
2020-11-22 6:12 ` [PATCH net 1/2] devlink: Hold rtnl lock while reading netdev attributes Parav Pandit
2020-11-22 6:12 ` [PATCH net 2/2] devlink: Make sure devlink instance and port are in same net namespace Parav Pandit
0 siblings, 2 replies; 13+ messages in thread
From: Parav Pandit @ 2020-11-22 6:12 UTC (permalink / raw)
To: kuba, davem, netdev, jiri; +Cc: 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
^ permalink raw reply [flat|nested] 13+ messages in thread* [PATCH net 1/2] devlink: Hold rtnl lock while reading netdev attributes 2020-11-22 6:12 [PATCH net 0/2] devlink port attribute fixes Parav Pandit @ 2020-11-22 6:12 ` Parav Pandit 2020-11-24 22:29 ` Jakub Kicinski 2020-11-22 6:12 ` [PATCH net 2/2] devlink: Make sure devlink instance and port are in same net namespace Parav Pandit 1 sibling, 1 reply; 13+ messages in thread From: Parav Pandit @ 2020-11-22 6:12 UTC (permalink / raw) To: kuba, davem, netdev, jiri; +Cc: 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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH net 1/2] devlink: Hold rtnl lock while reading netdev attributes 2020-11-22 6:12 ` [PATCH net 1/2] devlink: Hold rtnl lock while reading netdev attributes Parav Pandit @ 2020-11-24 22:29 ` Jakub Kicinski 2020-11-25 7:13 ` Parav Pandit 0 siblings, 1 reply; 13+ messages in thread From: Jakub Kicinski @ 2020-11-24 22:29 UTC (permalink / raw) To: Parav Pandit; +Cc: davem, netdev, jiri 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. ^ permalink raw reply [flat|nested] 13+ messages in thread
* RE: [PATCH net 1/2] devlink: Hold rtnl lock while reading netdev attributes 2020-11-24 22:29 ` Jakub Kicinski @ 2020-11-25 7:13 ` Parav Pandit 2020-11-25 16:30 ` Jakub Kicinski 0 siblings, 1 reply; 13+ messages in thread From: Parav Pandit @ 2020-11-25 7:13 UTC (permalink / raw) To: Jakub Kicinski; +Cc: davem, netdev, Jiri Pirko > 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. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH net 1/2] devlink: Hold rtnl lock while reading netdev attributes 2020-11-25 7:13 ` Parav Pandit @ 2020-11-25 16:30 ` Jakub Kicinski 2020-11-25 17:21 ` Parav Pandit 0 siblings, 1 reply; 13+ messages in thread From: Jakub Kicinski @ 2020-11-25 16:30 UTC (permalink / raw) To: Parav Pandit; +Cc: davem, netdev, Jiri Pirko 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. ^ permalink raw reply [flat|nested] 13+ messages in thread
* RE: [PATCH net 1/2] devlink: Hold rtnl lock while reading netdev attributes 2020-11-25 16:30 ` Jakub Kicinski @ 2020-11-25 17:21 ` Parav Pandit 2020-11-25 17:41 ` Jakub Kicinski 0 siblings, 1 reply; 13+ messages in thread From: Parav Pandit @ 2020-11-25 17:21 UTC (permalink / raw) To: Jakub Kicinski; +Cc: davem, netdev, Jiri Pirko > 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. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH net 1/2] devlink: Hold rtnl lock while reading netdev attributes 2020-11-25 17:21 ` Parav Pandit @ 2020-11-25 17:41 ` Jakub Kicinski 2020-11-25 18:17 ` Parav Pandit 0 siblings, 1 reply; 13+ messages in thread From: Jakub Kicinski @ 2020-11-25 17:41 UTC (permalink / raw) To: Parav Pandit; +Cc: davem, netdev, Jiri Pirko 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. ^ permalink raw reply [flat|nested] 13+ messages in thread
* RE: [PATCH net 1/2] devlink: Hold rtnl lock while reading netdev attributes 2020-11-25 17:41 ` Jakub Kicinski @ 2020-11-25 18:17 ` Parav Pandit 0 siblings, 0 replies; 13+ messages in thread From: Parav Pandit @ 2020-11-25 18:17 UTC (permalink / raw) To: Jakub Kicinski; +Cc: davem, netdev, Jiri Pirko > 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. ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH net 2/2] devlink: Make sure devlink instance and port are in same net namespace 2020-11-22 6:12 [PATCH net 0/2] devlink port attribute fixes Parav Pandit 2020-11-22 6:12 ` [PATCH net 1/2] devlink: Hold rtnl lock while reading netdev attributes Parav Pandit @ 2020-11-22 6:12 ` Parav Pandit 2020-11-25 9:16 ` [PATCH net v2 0/2] devlink port attribute fixes Parav Pandit 1 sibling, 1 reply; 13+ messages in thread From: Parav Pandit @ 2020-11-22 6:12 UTC (permalink / raw) To: kuba, davem, netdev, jiri; +Cc: 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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH net v2 0/2] devlink port attribute fixes 2020-11-22 6:12 ` [PATCH net 2/2] devlink: Make sure devlink instance and port are in same net namespace Parav Pandit @ 2020-11-25 9:16 ` Parav Pandit 2020-11-25 9:16 ` [PATCH net v2 1/2] devlink: Hold rtnl lock while reading netdev attributes Parav Pandit ` (2 more replies) 0 siblings, 3 replies; 13+ messages in thread From: Parav Pandit @ 2020-11-25 9:16 UTC (permalink / raw) To: kuba, davem, netdev, jiri; +Cc: 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH net v2 1/2] devlink: Hold rtnl lock while reading netdev attributes 2020-11-25 9:16 ` [PATCH net v2 0/2] devlink port attribute fixes Parav Pandit @ 2020-11-25 9:16 ` Parav Pandit 2020-11-25 9:16 ` [PATCH net v2 2/2] devlink: Make sure devlink instance and port are in same net namespace Parav Pandit 2020-11-26 1:40 ` [PATCH net v2 0/2] devlink port attribute fixes patchwork-bot+netdevbpf 2 siblings, 0 replies; 13+ messages in thread From: Parav Pandit @ 2020-11-25 9:16 UTC (permalink / raw) To: kuba, davem, netdev, jiri; +Cc: 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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH net v2 2/2] devlink: Make sure devlink instance and port are in same net namespace 2020-11-25 9:16 ` [PATCH net v2 0/2] devlink port attribute fixes Parav Pandit 2020-11-25 9:16 ` [PATCH net v2 1/2] devlink: Hold rtnl lock while reading netdev attributes Parav Pandit @ 2020-11-25 9:16 ` Parav Pandit 2020-11-26 1:40 ` [PATCH net v2 0/2] devlink port attribute fixes patchwork-bot+netdevbpf 2 siblings, 0 replies; 13+ messages in thread From: Parav Pandit @ 2020-11-25 9:16 UTC (permalink / raw) To: kuba, davem, netdev, jiri; +Cc: 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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH net v2 0/2] devlink port attribute fixes 2020-11-25 9:16 ` [PATCH net v2 0/2] devlink port attribute fixes Parav Pandit 2020-11-25 9:16 ` [PATCH net v2 1/2] devlink: Hold rtnl lock while reading netdev attributes Parav Pandit 2020-11-25 9:16 ` [PATCH net v2 2/2] devlink: Make sure devlink instance and port are in same net namespace Parav Pandit @ 2020-11-26 1:40 ` patchwork-bot+netdevbpf 2 siblings, 0 replies; 13+ messages in thread From: patchwork-bot+netdevbpf @ 2020-11-26 1:40 UTC (permalink / raw) To: Parav Pandit; +Cc: kuba, davem, netdev, jiri 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2020-11-26 1:40 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-11-22 6:12 [PATCH net 0/2] devlink port attribute fixes Parav Pandit 2020-11-22 6:12 ` [PATCH net 1/2] devlink: Hold rtnl lock while reading netdev attributes Parav Pandit 2020-11-24 22:29 ` Jakub Kicinski 2020-11-25 7:13 ` Parav Pandit 2020-11-25 16:30 ` Jakub Kicinski 2020-11-25 17:21 ` Parav Pandit 2020-11-25 17:41 ` Jakub Kicinski 2020-11-25 18:17 ` Parav Pandit 2020-11-22 6:12 ` [PATCH net 2/2] devlink: Make sure devlink instance and port are in same net namespace Parav Pandit 2020-11-25 9:16 ` [PATCH net v2 0/2] devlink port attribute fixes Parav Pandit 2020-11-25 9:16 ` [PATCH net v2 1/2] devlink: Hold rtnl lock while reading netdev attributes Parav Pandit 2020-11-25 9:16 ` [PATCH net v2 2/2] devlink: Make sure devlink instance and port are in same net namespace Parav Pandit 2020-11-26 1:40 ` [PATCH net v2 0/2] devlink port attribute fixes patchwork-bot+netdevbpf
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.