* [PATCH net-next v7 0/2] Add support to offload macsec using netlink update @ 2023-01-09 8:55 ehakim 2023-01-09 8:55 ` [PATCH net-next v7 1/2] macsec: add support for IFLA_MACSEC_OFFLOAD in macsec_changelink ehakim 2023-01-09 8:55 ` [PATCH net-next v7 2/2] macsec: dump IFLA_MACSEC_OFFLOAD attribute as part of macsec dump ehakim 0 siblings, 2 replies; 10+ messages in thread From: ehakim @ 2023-01-09 8:55 UTC (permalink / raw) To: netdev; +Cc: raeds, davem, edumazet, kuba, pabeni, sd, atenart, Emeel Hakim From: Emeel Hakim <ehakim@nvidia.com> This series adds support for offloading macsec as part of the netlink update routine, command example: ip link set link eth2 macsec0 type macsec offload mac The above is done using the IFLA_MACSEC_OFFLOAD attribute hence the second patch of dumping this attribute as part of the macsec dump. Emeel Hakim (2): macsec: add support for IFLA_MACSEC_OFFLOAD in macsec_changelink macsec: dump IFLA_MACSEC_OFFLOAD attribute as part of macsec dump drivers/net/macsec.c | 122 +++++++++++++++++++++++-------------------- 1 file changed, 65 insertions(+), 57 deletions(-) -- 2.21.3 ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH net-next v7 1/2] macsec: add support for IFLA_MACSEC_OFFLOAD in macsec_changelink 2023-01-09 8:55 [PATCH net-next v7 0/2] Add support to offload macsec using netlink update ehakim @ 2023-01-09 8:55 ` ehakim 2023-01-09 15:14 ` Sabrina Dubroca 2023-01-09 8:55 ` [PATCH net-next v7 2/2] macsec: dump IFLA_MACSEC_OFFLOAD attribute as part of macsec dump ehakim 1 sibling, 1 reply; 10+ messages in thread From: ehakim @ 2023-01-09 8:55 UTC (permalink / raw) To: netdev; +Cc: raeds, davem, edumazet, kuba, pabeni, sd, atenart, Emeel Hakim 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 Signed-off-by: Emeel Hakim <ehakim@nvidia.com> --- v6 -> v7: - Dont change rtnl_lock position after commit f3b4a00f0f62 ("net: macsec: fix net device access prior to holding a lock"). v5 -> v6: - Locking issue got fixed in a separate patch so rebase V4 -> V5: - Fail immediately if macsec ops does not exist 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 | 111 ++++++++++++++++++++++--------------------- 1 file changed, 56 insertions(+), 55 deletions(-) diff --git a/drivers/net/macsec.c b/drivers/net/macsec.c index bf8ac7a3ded7..687d4480b7b3 100644 --- a/drivers/net/macsec.c +++ b/drivers/net/macsec.c @@ -2583,18 +2583,61 @@ 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 = 0; + macsec = macsec_priv(dev); + + if (macsec->offload == offload) + return 0; + + /* Check if the offloading mode is supported by the underlying layers */ + if (offload != MACSEC_OFFLOAD_OFF && + !macsec_check_offload(offload, macsec)) { + return -EOPNOTSUPP; + } + + /* Check if the net device is busy. */ + if (netif_running(dev)) + return -EBUSY; + + /* Check if the device already has rules configured: we do not support + * rules migration. + */ + 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) + return -EOPNOTSUPP; + + macsec->offload = offload; + + ctx.secy = &macsec->secy; + ret = offload == MACSEC_OFFLOAD_OFF ? macsec_offload(ops->mdo_del_secy, &ctx) + : macsec_offload(ops->mdo_add_secy, &ctx); + if (ret) + macsec->offload = prev_offload; + + 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; @@ -2613,7 +2656,6 @@ static int macsec_upd_offload(struct sk_buff *skb, struct genl_info *info) ret = PTR_ERR(dev); goto out; } - macsec = macsec_priv(dev); if (!tb_offload[MACSEC_OFFLOAD_ATTR_TYPE]) { ret = -EINVAL; @@ -2621,55 +2663,8 @@ static int macsec_upd_offload(struct sk_buff *skb, struct genl_info *info) } offload = nla_get_u8(tb_offload[MACSEC_OFFLOAD_ATTR_TYPE]); - if (macsec->offload == offload) - goto out; - /* Check if the offloading mode is supported by the underlying layers */ - if (offload != MACSEC_OFFLOAD_OFF && - !macsec_check_offload(offload, macsec)) { - ret = -EOPNOTSUPP; - goto out; - } - - /* Check if the net device is busy. */ - if (netif_running(dev)) { - ret = -EBUSY; - goto out; - } - - 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; - } - - ops = __macsec_get_ops(offload == MACSEC_OFFLOAD_OFF ? prev_offload : offload, - macsec, &ctx); - if (!ops) { - ret = -EOPNOTSUPP; - goto rollback; - } - - if (prev_offload == MACSEC_OFFLOAD_OFF) - func = ops->mdo_add_secy; - else - func = ops->mdo_del_secy; - - ctx.secy = &macsec->secy; - ret = macsec_offload(func, &ctx); - if (ret) - goto rollback; - - rtnl_unlock(); - return 0; - -rollback: - macsec->offload = prev_offload; + ret = macsec_update_offload(dev, offload); out: rtnl_unlock(); return ret; @@ -3840,6 +3835,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; -- 2.21.3 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH net-next v7 1/2] macsec: add support for IFLA_MACSEC_OFFLOAD in macsec_changelink 2023-01-09 8:55 ` [PATCH net-next v7 1/2] macsec: add support for IFLA_MACSEC_OFFLOAD in macsec_changelink ehakim @ 2023-01-09 15:14 ` Sabrina Dubroca 2023-01-10 8:43 ` Antoine Tenart 0 siblings, 1 reply; 10+ messages in thread From: Sabrina Dubroca @ 2023-01-09 15:14 UTC (permalink / raw) To: ehakim, atenart; +Cc: netdev, raeds, davem, edumazet, kuba, pabeni 2023-01-09, 10:55:56 +0200, ehakim@nvidia.com wrote: > @@ -3840,6 +3835,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; There's a missing rollback of the offloading status in the (probably quite unlikely) case that mdo_upd_secy fails, no? We can't fail macsec_get_ops because macsec_update_offload would have failed already, but I guess the driver could fail in mdo_upd_secy, and then "goto cleanup" doesn't restore the offloading state. Sorry I didn't notice this earlier. In case the IFLA_MACSEC_OFFLOAD attribute is provided and we're enabling offload, we also end up calling the driver's mdo_add_secy, and then immediately afterwards mdo_upd_secy, which probably doesn't make much sense. Maybe we could turn that into: if (data[IFLA_MACSEC_OFFLOAD]) { ... macsec_update_offload } else if (macsec_is_offloaded(macsec)) { /* If h/w offloading is available, propagate to the device */ ... mdo_upd_secy } Antoine, does that look reasonable to you? -- Sabrina ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net-next v7 1/2] macsec: add support for IFLA_MACSEC_OFFLOAD in macsec_changelink 2023-01-09 15:14 ` Sabrina Dubroca @ 2023-01-10 8:43 ` Antoine Tenart 2023-01-10 9:05 ` Emeel Hakim 2023-01-10 10:44 ` Sabrina Dubroca 0 siblings, 2 replies; 10+ messages in thread From: Antoine Tenart @ 2023-01-10 8:43 UTC (permalink / raw) To: Sabrina Dubroca, ehakim; +Cc: netdev, raeds, davem, edumazet, kuba, pabeni Quoting Sabrina Dubroca (2023-01-09 16:14:32) > 2023-01-09, 10:55:56 +0200, ehakim@nvidia.com wrote: > > @@ -3840,6 +3835,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; > > There's a missing rollback of the offloading status in the (probably > quite unlikely) case that mdo_upd_secy fails, no? We can't fail > macsec_get_ops because macsec_update_offload would have failed > already, but I guess the driver could fail in mdo_upd_secy, and then > "goto cleanup" doesn't restore the offloading state. Sorry I didn't > notice this earlier. > > In case the IFLA_MACSEC_OFFLOAD attribute is provided and we're > enabling offload, we also end up calling the driver's mdo_add_secy, > and then immediately afterwards mdo_upd_secy, which probably doesn't > make much sense. > > Maybe we could turn that into: > > if (data[IFLA_MACSEC_OFFLOAD]) { If data[IFLA_MACSEC_OFFLOAD] is provided but doesn't change the offloading state, then macsec_update_offload will return early and mdo_upd_secy won't be called. > ... macsec_update_offload > } else if (macsec_is_offloaded(macsec)) { > /* If h/w offloading is available, propagate to the device */ > ... mdo_upd_secy > } > > Antoine, does that look reasonable to you? But yes I agree we can improve the logic. Maybe something like: prev_offload = macsec->offload; offload = data[IFLA_MACSEC_OFFLOAD]; if (prev_offload != offload) { macsec_update_offload(...) } else if (macsec_is_offloaded(macsec)) { ... prev_offload can be used to restore the offloading state on failure here. } Thanks, Antoine ^ permalink raw reply [flat|nested] 10+ messages in thread
* RE: [PATCH net-next v7 1/2] macsec: add support for IFLA_MACSEC_OFFLOAD in macsec_changelink 2023-01-10 8:43 ` Antoine Tenart @ 2023-01-10 9:05 ` Emeel Hakim [not found] ` <167334656781.17820.3219445403317381657@kwain.local> 2023-01-10 10:44 ` Sabrina Dubroca 1 sibling, 1 reply; 10+ messages in thread From: Emeel Hakim @ 2023-01-10 9:05 UTC (permalink / raw) To: Antoine Tenart, Sabrina Dubroca Cc: netdev, Raed Salem, davem, edumazet, kuba, pabeni > -----Original Message----- > From: Antoine Tenart <atenart@kernel.org> > Sent: Tuesday, 10 January 2023 10:44 > To: Sabrina Dubroca <sd@queasysnail.net>; Emeel Hakim <ehakim@nvidia.com> > Cc: netdev@vger.kernel.org; Raed Salem <raeds@nvidia.com>; > davem@davemloft.net; edumazet@google.com; kuba@kernel.org; > pabeni@redhat.com > Subject: Re: [PATCH net-next v7 1/2] macsec: add support for > IFLA_MACSEC_OFFLOAD in macsec_changelink > > External email: Use caution opening links or attachments > > > Quoting Sabrina Dubroca (2023-01-09 16:14:32) > > 2023-01-09, 10:55:56 +0200, ehakim@nvidia.com wrote: > > > @@ -3840,6 +3835,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; > > > > There's a missing rollback of the offloading status in the (probably > > quite unlikely) case that mdo_upd_secy fails, no? We can't fail > > macsec_get_ops because macsec_update_offload would have failed > > already, but I guess the driver could fail in mdo_upd_secy, and then > > "goto cleanup" doesn't restore the offloading state. Sorry I didn't > > notice this earlier. > > > > In case the IFLA_MACSEC_OFFLOAD attribute is provided and we're > > enabling offload, we also end up calling the driver's mdo_add_secy, > > and then immediately afterwards mdo_upd_secy, which probably doesn't > > make much sense. > > > > Maybe we could turn that into: > > > > if (data[IFLA_MACSEC_OFFLOAD]) { > > If data[IFLA_MACSEC_OFFLOAD] is provided but doesn't change the offloading > state, then macsec_update_offload will return early and mdo_upd_secy won't be > called. > > > ... macsec_update_offload > > } else if (macsec_is_offloaded(macsec)) { > > /* If h/w offloading is available, propagate to the device */ > > ... mdo_upd_secy > > } > > > > Antoine, does that look reasonable to you? > > But yes I agree we can improve the logic. Maybe something like: Ack , I can do the change > prev_offload = macsec->offload; > offload = data[IFLA_MACSEC_OFFLOAD]; > > if (prev_offload != offload) { > macsec_update_offload(...) > } else if (macsec_is_offloaded(macsec)) { > ... > prev_offload can be used to restore the offloading state on > failure here. why do we need to restore offloading state here in case of failure? we get to this case when prev_offload == offload. > } > > Thanks, > Antoine ^ permalink raw reply [flat|nested] 10+ messages in thread
[parent not found: <167334656781.17820.3219445403317381657@kwain.local>]
* Re: [PATCH net-next v7 1/2] macsec: add support for IFLA_MACSEC_OFFLOAD in macsec_changelink [not found] ` <167334656781.17820.3219445403317381657@kwain.local> @ 2023-01-10 11:46 ` Sabrina Dubroca 0 siblings, 0 replies; 10+ messages in thread From: Sabrina Dubroca @ 2023-01-10 11:46 UTC (permalink / raw) To: Antoine Tenart Cc: Emeel Hakim, netdev, Raed Salem, davem, edumazet, kuba, pabeni 2023-01-10, 11:29:27 +0100, Antoine Tenart wrote: > Quoting Emeel Hakim (2023-01-10 10:05:36) > > > Quoting Sabrina Dubroca (2023-01-09 16:14:32) > > > > 2023-01-09, 10:55:56 +0200, ehakim@nvidia.com wrote: > > > > > @@ -3840,6 +3835,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; > > > > > > > > There's a missing rollback of the offloading status in the (probably > > > > quite unlikely) case that mdo_upd_secy fails, no? We can't fail > > > > macsec_get_ops because macsec_update_offload would have failed > > > > already, but I guess the driver could fail in mdo_upd_secy, and then > > > > "goto cleanup" doesn't restore the offloading state. Sorry I didn't > > > > notice this earlier. > > > > > > > > In case the IFLA_MACSEC_OFFLOAD attribute is provided and we're > > > > enabling offload, we also end up calling the driver's mdo_add_secy, > > > > and then immediately afterwards mdo_upd_secy, which probably doesn't > > > > make much sense. > > > > > > > > Maybe we could turn that into: > > > > > > > > if (data[IFLA_MACSEC_OFFLOAD]) { > > > > > > If data[IFLA_MACSEC_OFFLOAD] is provided but doesn't change the offloading > > > state, then macsec_update_offload will return early and mdo_upd_secy won't be > > > called. > > > > > > > ... macsec_update_offload > > > > } else if (macsec_is_offloaded(macsec)) { > > > > /* If h/w offloading is available, propagate to the device */ > > > > ... mdo_upd_secy > > > > } > > > > > > > > Antoine, does that look reasonable to you? > > > > > > But yes I agree we can improve the logic. Maybe something like: > > > > Ack , I can do the change > > > > > prev_offload = macsec->offload; > > > offload = data[IFLA_MACSEC_OFFLOAD]; > > > > > > if (prev_offload != offload) { > > > macsec_update_offload(...) > > > } else if (macsec_is_offloaded(macsec)) { > > > ... > > > prev_offload can be used to restore the offloading state on > > > failure here. > > > > why do we need to restore offloading state here in case of failure? > > we get to this case when prev_offload == offload. > > Right, not restoring. The general question is: what to do with > offloading on and an hw in an unknown state (upd failed). Right, but I don't think that's introduced by this patch. I don't want to block Emeel's patches because of an issue that was present before. Do we need a way to distinguish - update failed but the HW is still offloading the old state, just roll back - update failed, this macsec device can't be offloaded anymore (or at least not until $unclear_condition) and maybe some other variants (destroy and recreate the macsec device? reload the NIC driver?)? Would that help? Is that a useful distinction for admins and management software? -- Sabrina ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net-next v7 1/2] macsec: add support for IFLA_MACSEC_OFFLOAD in macsec_changelink 2023-01-10 8:43 ` Antoine Tenart 2023-01-10 9:05 ` Emeel Hakim @ 2023-01-10 10:44 ` Sabrina Dubroca 2023-01-10 13:55 ` Antoine Tenart 1 sibling, 1 reply; 10+ messages in thread From: Sabrina Dubroca @ 2023-01-10 10:44 UTC (permalink / raw) To: Antoine Tenart; +Cc: ehakim, netdev, raeds, davem, edumazet, kuba, pabeni 2023-01-10, 09:43:37 +0100, Antoine Tenart wrote: > Quoting Sabrina Dubroca (2023-01-09 16:14:32) > > 2023-01-09, 10:55:56 +0200, ehakim@nvidia.com wrote: > > > @@ -3840,6 +3835,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; > > > > There's a missing rollback of the offloading status in the (probably > > quite unlikely) case that mdo_upd_secy fails, no? We can't fail > > macsec_get_ops because macsec_update_offload would have failed > > already, but I guess the driver could fail in mdo_upd_secy, and then > > "goto cleanup" doesn't restore the offloading state. Sorry I didn't > > notice this earlier. > > > > In case the IFLA_MACSEC_OFFLOAD attribute is provided and we're > > enabling offload, we also end up calling the driver's mdo_add_secy, > > and then immediately afterwards mdo_upd_secy, which probably doesn't > > make much sense. > > > > Maybe we could turn that into: > > > > if (data[IFLA_MACSEC_OFFLOAD]) { > > If data[IFLA_MACSEC_OFFLOAD] is provided but doesn't change the > offloading state, then macsec_update_offload will return early and > mdo_upd_secy won't be called. Ouch, thanks for catching this. > > > ... macsec_update_offload > > } else if (macsec_is_offloaded(macsec)) { > > /* If h/w offloading is available, propagate to the device */ > > ... mdo_upd_secy > > } > > > > Antoine, does that look reasonable to you? > > But yes I agree we can improve the logic. Maybe something like: > > prev_offload = macsec->offload; > offload = data[IFLA_MACSEC_OFFLOAD]; That needs to be under if (data[IFLA_MACSEC_OFFLOAD]) and then the rest gets a bit messy. > > if (prev_offload != offload) { > macsec_update_offload(...) > } else if (macsec_is_offloaded(macsec)) { > ... > prev_offload can be used to restore the offloading state on > failure here. > } We also have a prev != new test at the start of macsec_update_offload, the duplication is a bit ugly. We could move it out and then only call macsec_update_offload when there is a change to do, both from macsec_changelink and macsec_upd_offload. Since we don't need to restore in the second branch, and we can only fetch IFLA_MACSEC_OFFLOAD when it's present, maybe: change = false; if (data[IFLA_MACSEC_OFFLOAD]) { offload = nla_get_u8(data[IFLA_MACSEC_OFFLOAD]); if (macsec->offload != offload) { change = true; macsec_update_offload ...cleanup } } if (!change && macsec_is_offloaded(macsec)) { ... } Or let macsec_update_offload do the macsec->offload != offload test and pass &change so that changelink can know what to do next. -- Sabrina ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net-next v7 1/2] macsec: add support for IFLA_MACSEC_OFFLOAD in macsec_changelink 2023-01-10 10:44 ` Sabrina Dubroca @ 2023-01-10 13:55 ` Antoine Tenart 2023-01-10 16:16 ` Emeel Hakim 0 siblings, 1 reply; 10+ messages in thread From: Antoine Tenart @ 2023-01-10 13:55 UTC (permalink / raw) To: Sabrina Dubroca; +Cc: ehakim, netdev, raeds, davem, edumazet, kuba, pabeni Quoting Sabrina Dubroca (2023-01-10 11:44:13) > 2023-01-10, 09:43:37 +0100, Antoine Tenart wrote: > > Quoting Sabrina Dubroca (2023-01-09 16:14:32) > > > 2023-01-09, 10:55:56 +0200, ehakim@nvidia.com wrote: > > > > @@ -3840,6 +3835,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; > > > > > > There's a missing rollback of the offloading status in the (probably > > > quite unlikely) case that mdo_upd_secy fails, no? We can't fail > > > macsec_get_ops because macsec_update_offload would have failed > > > already, but I guess the driver could fail in mdo_upd_secy, and then > > > "goto cleanup" doesn't restore the offloading state. Sorry I didn't > > > notice this earlier. > > > > > > In case the IFLA_MACSEC_OFFLOAD attribute is provided and we're > > > enabling offload, we also end up calling the driver's mdo_add_secy, > > > and then immediately afterwards mdo_upd_secy, which probably doesn't > > > make much sense. > > > > > > Maybe we could turn that into: > > > > > > if (data[IFLA_MACSEC_OFFLOAD]) { > > > > If data[IFLA_MACSEC_OFFLOAD] is provided but doesn't change the > > offloading state, then macsec_update_offload will return early and > > mdo_upd_secy won't be called. > > Ouch, thanks for catching this. > > > > > > ... macsec_update_offload > > > } else if (macsec_is_offloaded(macsec)) { > > > /* If h/w offloading is available, propagate to the device */ > > > ... mdo_upd_secy > > > } > > > > > > Antoine, does that look reasonable to you? > > > > But yes I agree we can improve the logic. Maybe something like: > > > > prev_offload = macsec->offload; > > offload = data[IFLA_MACSEC_OFFLOAD]; > > That needs to be under if (data[IFLA_MACSEC_OFFLOAD]) and then the > rest gets a bit messy. > > > > > if (prev_offload != offload) { > > macsec_update_offload(...) > > } else if (macsec_is_offloaded(macsec)) { > > ... > > prev_offload can be used to restore the offloading state on > > failure here. > > } > > We also have a prev != new test at the start of macsec_update_offload, > the duplication is a bit ugly. We could move it out and then only call > macsec_update_offload when there is a change to do, both from > macsec_changelink and macsec_upd_offload. Agreed. > Since we don't need to restore in the second branch, and we can only > fetch IFLA_MACSEC_OFFLOAD when it's present, maybe: > > change = false; > if (data[IFLA_MACSEC_OFFLOAD]) { > offload = nla_get_u8(data[IFLA_MACSEC_OFFLOAD]); > if (macsec->offload != offload) { > change = true; > macsec_update_offload ...cleanup > } > } > > if (!change && macsec_is_offloaded(macsec)) { > ... > } > > Or let macsec_update_offload do the macsec->offload != offload test > and pass &change so that changelink can know what to do next. Either solutions work for me. Thanks! Antoine ^ permalink raw reply [flat|nested] 10+ messages in thread
* RE: [PATCH net-next v7 1/2] macsec: add support for IFLA_MACSEC_OFFLOAD in macsec_changelink 2023-01-10 13:55 ` Antoine Tenart @ 2023-01-10 16:16 ` Emeel Hakim 0 siblings, 0 replies; 10+ messages in thread From: Emeel Hakim @ 2023-01-10 16:16 UTC (permalink / raw) To: Antoine Tenart, Sabrina Dubroca Cc: netdev, Raed Salem, davem, edumazet, kuba, pabeni > -----Original Message----- > From: Antoine Tenart <atenart@kernel.org> > Sent: Tuesday, 10 January 2023 15:55 > To: Sabrina Dubroca <sd@queasysnail.net> > Cc: Emeel Hakim <ehakim@nvidia.com>; netdev@vger.kernel.org; Raed Salem > <raeds@nvidia.com>; davem@davemloft.net; edumazet@google.com; > kuba@kernel.org; pabeni@redhat.com > Subject: Re: [PATCH net-next v7 1/2] macsec: add support for > IFLA_MACSEC_OFFLOAD in macsec_changelink > > External email: Use caution opening links or attachments > > > Quoting Sabrina Dubroca (2023-01-10 11:44:13) > > 2023-01-10, 09:43:37 +0100, Antoine Tenart wrote: > > > Quoting Sabrina Dubroca (2023-01-09 16:14:32) > > > > 2023-01-09, 10:55:56 +0200, ehakim@nvidia.com wrote: > > > > > @@ -3840,6 +3835,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; > > > > > > > > There's a missing rollback of the offloading status in the > > > > (probably quite unlikely) case that mdo_upd_secy fails, no? We > > > > can't fail macsec_get_ops because macsec_update_offload would have > > > > failed already, but I guess the driver could fail in mdo_upd_secy, > > > > and then "goto cleanup" doesn't restore the offloading state. > > > > Sorry I didn't notice this earlier. > > > > > > > > In case the IFLA_MACSEC_OFFLOAD attribute is provided and we're > > > > enabling offload, we also end up calling the driver's > > > > mdo_add_secy, and then immediately afterwards mdo_upd_secy, which > > > > probably doesn't make much sense. > > > > > > > > Maybe we could turn that into: > > > > > > > > if (data[IFLA_MACSEC_OFFLOAD]) { > > > > > > If data[IFLA_MACSEC_OFFLOAD] is provided but doesn't change the > > > offloading state, then macsec_update_offload will return early and > > > mdo_upd_secy won't be called. > > > > Ouch, thanks for catching this. > > > > > > > > > ... macsec_update_offload > > > > } else if (macsec_is_offloaded(macsec)) { > > > > /* If h/w offloading is available, propagate to the device */ > > > > ... mdo_upd_secy > > > > } > > > > > > > > Antoine, does that look reasonable to you? > > > > > > But yes I agree we can improve the logic. Maybe something like: > > > > > > prev_offload = macsec->offload; > > > offload = data[IFLA_MACSEC_OFFLOAD]; > > > > That needs to be under if (data[IFLA_MACSEC_OFFLOAD]) and then the > > rest gets a bit messy. > > > > > > > > if (prev_offload != offload) { > > > macsec_update_offload(...) > > > } else if (macsec_is_offloaded(macsec)) { > > > ... > > > prev_offload can be used to restore the offloading state on > > > failure here. > > > } > > > > We also have a prev != new test at the start of macsec_update_offload, > > the duplication is a bit ugly. We could move it out and then only call > > macsec_update_offload when there is a change to do, both from > > macsec_changelink and macsec_upd_offload. > > Agreed. > > > Since we don't need to restore in the second branch, and we can only > > fetch IFLA_MACSEC_OFFLOAD when it's present, maybe: > > > > change = false; > > if (data[IFLA_MACSEC_OFFLOAD]) { > > offload = nla_get_u8(data[IFLA_MACSEC_OFFLOAD]); > > if (macsec->offload != offload) { > > change = true; > > macsec_update_offload ...cleanup > > } > > } > > > > if (!change && macsec_is_offloaded(macsec)) { > > ... > > } > > > > Or let macsec_update_offload do the macsec->offload != offload test > > and pass &change so that changelink can know what to do next. > > Either solutions work for me. Ack, I will send a v8 with Sabrina's approach of change = false ... > Thanks! > Antoine Thanks, Emeel ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH net-next v7 2/2] macsec: dump IFLA_MACSEC_OFFLOAD attribute as part of macsec dump 2023-01-09 8:55 [PATCH net-next v7 0/2] Add support to offload macsec using netlink update ehakim 2023-01-09 8:55 ` [PATCH net-next v7 1/2] macsec: add support for IFLA_MACSEC_OFFLOAD in macsec_changelink ehakim @ 2023-01-09 8:55 ` ehakim 1 sibling, 0 replies; 10+ messages in thread From: ehakim @ 2023-01-09 8:55 UTC (permalink / raw) To: netdev; +Cc: raeds, davem, edumazet, kuba, pabeni, sd, atenart, Emeel Hakim From: Emeel Hakim <ehakim@nvidia.com> Support dumping offload netlink attribute in macsec's device attributes dump. Change macsec_get_size to consider the offload attribute in the calculations of the required room for dumping the device netlink attributes. Signed-off-by: Emeel Hakim <ehakim@nvidia.com> --- V1 -> V2: Update commit message drivers/net/macsec.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/drivers/net/macsec.c b/drivers/net/macsec.c index 687d4480b7b3..18dabb4840cb 100644 --- a/drivers/net/macsec.c +++ b/drivers/net/macsec.c @@ -4241,16 +4241,22 @@ static size_t macsec_get_size(const struct net_device *dev) nla_total_size(1) + /* IFLA_MACSEC_SCB */ nla_total_size(1) + /* IFLA_MACSEC_REPLAY_PROTECT */ nla_total_size(1) + /* IFLA_MACSEC_VALIDATION */ + nla_total_size(1) + /* IFLA_MACSEC_OFFLOAD */ 0; } static int macsec_fill_info(struct sk_buff *skb, const struct net_device *dev) { - struct macsec_secy *secy = &macsec_priv(dev)->secy; - struct macsec_tx_sc *tx_sc = &secy->tx_sc; + struct macsec_tx_sc *tx_sc; + struct macsec_dev *macsec; + struct macsec_secy *secy; u64 csid; + macsec = macsec_priv(dev); + secy = &macsec->secy; + tx_sc = &secy->tx_sc; + switch (secy->key_len) { case MACSEC_GCM_AES_128_SAK_LEN: csid = secy->xpn ? MACSEC_CIPHER_ID_GCM_AES_XPN_128 : MACSEC_DEFAULT_CIPHER_ID; @@ -4275,6 +4281,7 @@ static int macsec_fill_info(struct sk_buff *skb, nla_put_u8(skb, IFLA_MACSEC_SCB, tx_sc->scb) || nla_put_u8(skb, IFLA_MACSEC_REPLAY_PROTECT, secy->replay_protect) || nla_put_u8(skb, IFLA_MACSEC_VALIDATION, secy->validate_frames) || + nla_put_u8(skb, IFLA_MACSEC_OFFLOAD, macsec->offload) || 0) goto nla_put_failure; -- 2.21.3 ^ permalink raw reply related [flat|nested] 10+ messages in thread
end of thread, other threads:[~2023-01-10 16:16 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-01-09 8:55 [PATCH net-next v7 0/2] Add support to offload macsec using netlink update ehakim 2023-01-09 8:55 ` [PATCH net-next v7 1/2] macsec: add support for IFLA_MACSEC_OFFLOAD in macsec_changelink ehakim 2023-01-09 15:14 ` Sabrina Dubroca 2023-01-10 8:43 ` Antoine Tenart 2023-01-10 9:05 ` Emeel Hakim [not found] ` <167334656781.17820.3219445403317381657@kwain.local> 2023-01-10 11:46 ` Sabrina Dubroca 2023-01-10 10:44 ` Sabrina Dubroca 2023-01-10 13:55 ` Antoine Tenart 2023-01-10 16:16 ` Emeel Hakim 2023-01-09 8:55 ` [PATCH net-next v7 2/2] macsec: dump IFLA_MACSEC_OFFLOAD attribute as part of macsec dump ehakim
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).