| Message ID | 20221208115517.14951-1-ehakim@nvidia.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next,v4,1/2] macsec: add support for IFLA_MACSEC_OFFLOAD in macsec_changelink | expand |
Thu, Dec 08, 2022 at 12:55:16PM CET, ehakim@nvidia.com wrote: >From: Emeel Hakim <ehakim@nvidia.com> > >Add support for changing Macsec offload selection through the >netlink layer by implementing the relevant changes in >macsec_changelink. > >Since the handling in macsec_changelink is similar to macsec_upd_offload, >update macsec_upd_offload to use a common helper function to avoid >duplication. > >Example for setting offload for a macsec device: > ip link set macsec0 type macsec offload mac > >Reviewed-by: Raed Salem <raeds@nvidia.com> >Signed-off-by: Emeel Hakim <ehakim@nvidia.com> Looks fine to me. Reviewed-by: Jiri Pirko <jiri@nvidia.com>
2022-12-08, 13:55:16 +0200, ehakim@nvidia.com wrote: > From: Emeel Hakim <ehakim@nvidia.com> > > Add support for changing Macsec offload selection through the > netlink layer by implementing the relevant changes in > macsec_changelink. > > Since the handling in macsec_changelink is similar to macsec_upd_offload, > update macsec_upd_offload to use a common helper function to avoid > duplication. > > Example for setting offload for a macsec device: > ip link set macsec0 type macsec offload mac > > Reviewed-by: Raed Salem <raeds@nvidia.com> > Signed-off-by: Emeel Hakim <ehakim@nvidia.com> > --- > V3 -> V4: - Dont pass whole attributes data to macsec_update_offload, just pass relevant attribute. > - Fix code style. > - Remove macsec_changelink_upd_offload > V2 -> V3: - Split the original patch into 3 patches, the macsec_rtnl_policy related change (separate patch) > to be sent to "net" branch as a fix. > - Change the original patch title to make it clear that it's only adding IFLA_MACSEC_OFFLOAD > to changelink > V1 -> V2: Add common helper to avoid duplicating code > > drivers/net/macsec.c | 100 +++++++++++++++++++++++-------------------- > 1 file changed, 53 insertions(+), 47 deletions(-) > > diff --git a/drivers/net/macsec.c b/drivers/net/macsec.c > index d73b9d535b7a..abfe4a612a2d 100644 > --- a/drivers/net/macsec.c > +++ b/drivers/net/macsec.c > @@ -2583,38 +2583,16 @@ static bool macsec_is_configured(struct macsec_dev *macsec) > return false; > } > > -static int macsec_upd_offload(struct sk_buff *skb, struct genl_info *info) > +static int macsec_update_offload(struct net_device *dev, enum macsec_offload offload) > { > - struct nlattr *tb_offload[MACSEC_OFFLOAD_ATTR_MAX + 1]; > - enum macsec_offload offload, prev_offload; > - int (*func)(struct macsec_context *ctx); > - struct nlattr **attrs = info->attrs; > - struct net_device *dev; > + enum macsec_offload prev_offload; > const struct macsec_ops *ops; > struct macsec_context ctx; > struct macsec_dev *macsec; > - int ret; > - > - if (!attrs[MACSEC_ATTR_IFINDEX]) > - return -EINVAL; > - > - if (!attrs[MACSEC_ATTR_OFFLOAD]) > - return -EINVAL; > - > - if (nla_parse_nested_deprecated(tb_offload, MACSEC_OFFLOAD_ATTR_MAX, > - attrs[MACSEC_ATTR_OFFLOAD], > - macsec_genl_offload_policy, NULL)) > - return -EINVAL; > + int ret = 0; > > - dev = get_dev_from_nl(genl_info_net(info), attrs); > - if (IS_ERR(dev)) > - return PTR_ERR(dev); > macsec = macsec_priv(dev); > > - if (!tb_offload[MACSEC_OFFLOAD_ATTR_TYPE]) > - return -EINVAL; > - > - offload = nla_get_u8(tb_offload[MACSEC_OFFLOAD_ATTR_TYPE]); > if (macsec->offload == offload) > return 0; > > @@ -2627,43 +2605,65 @@ static int macsec_upd_offload(struct sk_buff *skb, struct genl_info *info) > if (netif_running(dev)) > return -EBUSY; > > - rtnl_lock(); > - > - prev_offload = macsec->offload; > - macsec->offload = offload; > - > /* Check if the device already has rules configured: we do not support > * rules migration. > */ > - if (macsec_is_configured(macsec)) { > - ret = -EBUSY; > - goto rollback; > - } > + if (macsec_is_configured(macsec)) > + return -EBUSY; > + > + prev_offload = macsec->offload; > > ops = __macsec_get_ops(offload == MACSEC_OFFLOAD_OFF ? prev_offload : offload, > macsec, &ctx); > - if (!ops) { > + if (!ops) > ret = -EOPNOTSUPP; return -EOPNOTSUPP; It's probably not a problem because macsec_check_offload should catch that, but it looks wrong. > - goto rollback; > - } > > - if (prev_offload == MACSEC_OFFLOAD_OFF) > - func = ops->mdo_add_secy; > - else > - func = ops->mdo_del_secy; > + macsec->offload = offload; > > ctx.secy = &macsec->secy; > - ret = macsec_offload(func, &ctx); > + ret = offload == MACSEC_OFFLOAD_OFF ? macsec_offload(ops->mdo_del_secy, &ctx) > + : macsec_offload(ops->mdo_add_secy, &ctx); > + nit: unnecessary blank line (sorry, I should have spotted that earlier) > if (ret) > - goto rollback; > + macsec->offload = prev_offload; > > - rtnl_unlock(); > - return 0; > + return ret; > +} > + > +static int macsec_upd_offload(struct sk_buff *skb, struct genl_info *info) > +{ > + struct nlattr *tb_offload[MACSEC_OFFLOAD_ATTR_MAX + 1]; > + struct nlattr **attrs = info->attrs; > + enum macsec_offload offload; > + struct net_device *dev; > + int ret; > + > + if (!attrs[MACSEC_ATTR_IFINDEX]) > + return -EINVAL; > + > + if (!attrs[MACSEC_ATTR_OFFLOAD]) > + return -EINVAL; > + > + if (nla_parse_nested_deprecated(tb_offload, MACSEC_OFFLOAD_ATTR_MAX, > + attrs[MACSEC_ATTR_OFFLOAD], > + macsec_genl_offload_policy, NULL)) > + return -EINVAL; > + > + dev = get_dev_from_nl(genl_info_net(info), attrs); > + if (IS_ERR(dev)) > + return PTR_ERR(dev); > > -rollback: > - macsec->offload = prev_offload; > + if (!tb_offload[MACSEC_OFFLOAD_ATTR_TYPE]) > + return -EINVAL; > + > + offload = nla_get_u8(tb_offload[MACSEC_OFFLOAD_ATTR_TYPE]); > + > + rtnl_lock(); > + > + ret = macsec_update_offload(dev, offload); > > rtnl_unlock(); > + > return ret; > } > > @@ -3831,6 +3831,12 @@ static int macsec_changelink(struct net_device *dev, struct nlattr *tb[], > if (ret) > goto cleanup; > > + if (data[IFLA_MACSEC_OFFLOAD]) { > + ret = macsec_update_offload(dev, nla_get_u8(data[IFLA_MACSEC_OFFLOAD])); > + if (ret) > + goto cleanup; > + } > + > /* If h/w offloading is available, propagate to the device */ > if (macsec_is_offloaded(macsec)) { > const struct macsec_ops *ops;
On Thu, 8 Dec 2022 13:55:16 +0200 ehakim@nvidia.com wrote: > + dev = get_dev_from_nl(genl_info_net(info), attrs); What prevents this dev from disappearing before... > + if (IS_ERR(dev)) > + return PTR_ERR(dev); > + if (!tb_offload[MACSEC_OFFLOAD_ATTR_TYPE]) > + return -EINVAL; > + > + offload = nla_get_u8(tb_offload[MACSEC_OFFLOAD_ATTR_TYPE]); > + > + rtnl_lock(); ... we finally take the lock? I think you're just moving this code, but still. > + ret = macsec_update_offload(dev, offload); > > rtnl_unlock();
On Thu, 8 Dec 2022 18:32:44 -0800 Jakub Kicinski wrote:
> I think you're just moving this code, but still.
And by "by still" I mean - it's still a bug, so it needs to be fixed
first.
> -----Original Message----- > From: Jakub Kicinski <kuba@kernel.org> > Sent: Friday, 9 December 2022 4:33 > To: Emeel Hakim <ehakim@nvidia.com> > Cc: linux-kernel@vger.kernel.org; Raed Salem <raeds@nvidia.com>; > davem@davemloft.net; edumazet@google.com; pabeni@redhat.com; > netdev@vger.kernel.org; sd@queasysnail.net; atenart@kernel.org; jiri@resnulli.us > Subject: Re: [PATCH net-next v4 1/2] macsec: add support for > IFLA_MACSEC_OFFLOAD in macsec_changelink > > External email: Use caution opening links or attachments > > > On Thu, 8 Dec 2022 18:32:44 -0800 Jakub Kicinski wrote: > > I think you're just moving this code, but still. > > And by "by still" I mean - it's still a bug, so it needs to be fixed first. The code added by those patches does not use the rtnl_lock, the lock is just getting moved as part of sharing similar code, but the new code is still not using it, I don’t think those patches need to wait until a fix of an existing locking issue as long as the new code is not inserting any bugs.
> -----Original Message----- > From: Emeel Hakim > Sent: Friday, 9 December 2022 12:19 > To: Jakub Kicinski <kuba@kernel.org> > Cc: linux-kernel@vger.kernel.org; Raed Salem <raeds@nvidia.com>; > davem@davemloft.net; edumazet@google.com; pabeni@redhat.com; > netdev@vger.kernel.org; sd@queasysnail.net; atenart@kernel.org; jiri@resnulli.us > Subject: RE: [PATCH net-next v4 1/2] macsec: add support for > IFLA_MACSEC_OFFLOAD in macsec_changelink > > > > > -----Original Message----- > > From: Jakub Kicinski <kuba@kernel.org> > > Sent: Friday, 9 December 2022 4:33 > > To: Emeel Hakim <ehakim@nvidia.com> > > Cc: linux-kernel@vger.kernel.org; Raed Salem <raeds@nvidia.com>; > > davem@davemloft.net; edumazet@google.com; pabeni@redhat.com; > > netdev@vger.kernel.org; sd@queasysnail.net; atenart@kernel.org; > > jiri@resnulli.us > > Subject: Re: [PATCH net-next v4 1/2] macsec: add support for > > IFLA_MACSEC_OFFLOAD in macsec_changelink > > > > External email: Use caution opening links or attachments > > > > > > On Thu, 8 Dec 2022 18:32:44 -0800 Jakub Kicinski wrote: > > > I think you're just moving this code, but still. > > > > And by "by still" I mean - it's still a bug, so it needs to be fixed first. > > The code added by those patches does not use the rtnl_lock, the lock is just getting > moved as part of sharing similar code, but the new code is still not using it, I don’t > think those patches need to wait until a fix of an existing locking issue as long as > the new code is not inserting any bugs. I will send a fix patch shortly as requested.
diff --git a/drivers/net/macsec.c b/drivers/net/macsec.c index d73b9d535b7a..abfe4a612a2d 100644 --- a/drivers/net/macsec.c +++ b/drivers/net/macsec.c @@ -2583,38 +2583,16 @@ static bool macsec_is_configured(struct macsec_dev *macsec) return false; } -static int macsec_upd_offload(struct sk_buff *skb, struct genl_info *info) +static int macsec_update_offload(struct net_device *dev, enum macsec_offload offload) { - struct nlattr *tb_offload[MACSEC_OFFLOAD_ATTR_MAX + 1]; - enum macsec_offload offload, prev_offload; - int (*func)(struct macsec_context *ctx); - struct nlattr **attrs = info->attrs; - struct net_device *dev; + enum macsec_offload prev_offload; const struct macsec_ops *ops; struct macsec_context ctx; struct macsec_dev *macsec; - int ret; - - if (!attrs[MACSEC_ATTR_IFINDEX]) - return -EINVAL; - - if (!attrs[MACSEC_ATTR_OFFLOAD]) - return -EINVAL; - - if (nla_parse_nested_deprecated(tb_offload, MACSEC_OFFLOAD_ATTR_MAX, - attrs[MACSEC_ATTR_OFFLOAD], - macsec_genl_offload_policy, NULL)) - return -EINVAL; + int ret = 0; - dev = get_dev_from_nl(genl_info_net(info), attrs); - if (IS_ERR(dev)) - return PTR_ERR(dev); macsec = macsec_priv(dev); - if (!tb_offload[MACSEC_OFFLOAD_ATTR_TYPE]) - return -EINVAL; - - offload = nla_get_u8(tb_offload[MACSEC_OFFLOAD_ATTR_TYPE]); if (macsec->offload == offload) return 0; @@ -2627,43 +2605,65 @@ static int macsec_upd_offload(struct sk_buff *skb, struct genl_info *info) if (netif_running(dev)) return -EBUSY; - rtnl_lock(); - - prev_offload = macsec->offload; - macsec->offload = offload; - /* Check if the device already has rules configured: we do not support * rules migration. */ - if (macsec_is_configured(macsec)) { - ret = -EBUSY; - goto rollback; - } + if (macsec_is_configured(macsec)) + return -EBUSY; + + prev_offload = macsec->offload; ops = __macsec_get_ops(offload == MACSEC_OFFLOAD_OFF ? prev_offload : offload, macsec, &ctx); - if (!ops) { + if (!ops) ret = -EOPNOTSUPP; - goto rollback; - } - if (prev_offload == MACSEC_OFFLOAD_OFF) - func = ops->mdo_add_secy; - else - func = ops->mdo_del_secy; + macsec->offload = offload; ctx.secy = &macsec->secy; - ret = macsec_offload(func, &ctx); + ret = offload == MACSEC_OFFLOAD_OFF ? macsec_offload(ops->mdo_del_secy, &ctx) + : macsec_offload(ops->mdo_add_secy, &ctx); + if (ret) - goto rollback; + macsec->offload = prev_offload; - rtnl_unlock(); - return 0; + return ret; +} + +static int macsec_upd_offload(struct sk_buff *skb, struct genl_info *info) +{ + struct nlattr *tb_offload[MACSEC_OFFLOAD_ATTR_MAX + 1]; + struct nlattr **attrs = info->attrs; + enum macsec_offload offload; + struct net_device *dev; + int ret; + + if (!attrs[MACSEC_ATTR_IFINDEX]) + return -EINVAL; + + if (!attrs[MACSEC_ATTR_OFFLOAD]) + return -EINVAL; + + if (nla_parse_nested_deprecated(tb_offload, MACSEC_OFFLOAD_ATTR_MAX, + attrs[MACSEC_ATTR_OFFLOAD], + macsec_genl_offload_policy, NULL)) + return -EINVAL; + + dev = get_dev_from_nl(genl_info_net(info), attrs); + if (IS_ERR(dev)) + return PTR_ERR(dev); -rollback: - macsec->offload = prev_offload; + if (!tb_offload[MACSEC_OFFLOAD_ATTR_TYPE]) + return -EINVAL; + + offload = nla_get_u8(tb_offload[MACSEC_OFFLOAD_ATTR_TYPE]); + + rtnl_lock(); + + ret = macsec_update_offload(dev, offload); rtnl_unlock(); + return ret; } @@ -3831,6 +3831,12 @@ static int macsec_changelink(struct net_device *dev, struct nlattr *tb[], if (ret) goto cleanup; + if (data[IFLA_MACSEC_OFFLOAD]) { + ret = macsec_update_offload(dev, nla_get_u8(data[IFLA_MACSEC_OFFLOAD])); + if (ret) + goto cleanup; + } + /* If h/w offloading is available, propagate to the device */ if (macsec_is_offloaded(macsec)) { const struct macsec_ops *ops;