Re: [PATCH net-next v2] macsec: Add support for IFLA_MACSEC_OFFLOAD in the netlink layer

From: Jiri Pirko
Date: Tue Dec 06 2022 - 08:35:40 EST

Next message: Athira Rajeev: "Re: [PATCH 18/20] perf stat: Display event stats using aggr counts"
Previous message: Fabiano Rosas: "Re: [PATCH v10 2/9] KVM: Introduce per-page memory attributes"
In reply to: Emeel Hakim: "RE: [PATCH net-next v2] macsec: Add support for IFLA_MACSEC_OFFLOAD in the netlink layer"
Next in thread: Sabrina Dubroca: "Re: [PATCH net-next v2] macsec: Add support for IFLA_MACSEC_OFFLOAD in the netlink layer"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Tue, Dec 06, 2022 at 01:31:54PM CET, ehakim@xxxxxxxxxx wrote:
>
>
>> -----Original Message-----
>> From: Jiri Pirko <jiri@xxxxxxxxxxx>
>> Sent: Tuesday, 6 December 2022 11:16
>> To: Emeel Hakim <ehakim@xxxxxxxxxx>
>> Cc: linux-kernel@xxxxxxxxxxxxxxx; Raed Salem <raeds@xxxxxxxxxx>;
>> davem@xxxxxxxxxxxxx; edumazet@xxxxxxxxxx; kuba@xxxxxxxxxx;
>> pabeni@xxxxxxxxxx; netdev@xxxxxxxxxxxxxxx; sd@xxxxxxxxxxxxxxx
>> Subject: Re: [PATCH net-next v2] macsec: Add support for IFLA_MACSEC_OFFLOAD
>> in the netlink layer
>>
>> External email: Use caution opening links or attachments
>>
>>
>> Tue, Dec 06, 2022 at 09:57:57AM CET, ehakim@xxxxxxxxxx wrote:
>> >From: Emeel Hakim <ehakim@xxxxxxxxxx>
>> >
>> >This adds support for configuring Macsec offload through the
>>
>> Tell the codebase what to do. Be imperative in your patch descriptions so it is clear
>> what are the intensions of the patch.
>
>Ack
>
>>
>>
>> >netlink layer by:
>> >- Considering IFLA_MACSEC_OFFLOAD in macsec_fill_info.
>> >- Handling IFLA_MACSEC_OFFLOAD in macsec_changelink.
>> >- Adding IFLA_MACSEC_OFFLOAD to the netlink policy.
>> >- Adjusting macsec_get_size.
>>
>> 4 patches then?
>
>Ack, I will change the commit message to be imperative and will replace the list with a good description.
>I still believe it should be a one patch since splitting this could break a bisect process.

Well, when you split, you have to make sure you don't break bisection,
always. Please try to figure that out.

>
>> I mean really, not a macsec person, but I should be able to follow what are your
>> intensions looking and description&code right away.
>>
>> >
>> >The handling in macsec_changlink is similar to
>>
>> s/macsec_changlink/macsec_changelink/
>
>Ack
>
>> >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@xxxxxxxxxx>
>> >Signed-off-by: Emeel Hakim <ehakim@xxxxxxxxxx>
>> >---
>> >V1 -> V2: Add common helper to avoid duplicating code
>> >drivers/net/macsec.c | 114 ++++++++++++++++++++++++++++---------------
>> > 1 file changed, 74 insertions(+), 40 deletions(-)
>> >
>> >diff --git a/drivers/net/macsec.c b/drivers/net/macsec.c index
>> >d73b9d535b7a..afd6ff47ba56 100644
>> >--- a/drivers/net/macsec.c
>> >+++ b/drivers/net/macsec.c
>> >@@ -2583,16 +2583,45 @@ static bool macsec_is_configured(struct macsec_dev
>> *macsec)
>> > return false;
>> > }
>> >
>> >+static int macsec_update_offload(struct macsec_dev *macsec, enum
>> >+macsec_offload offload) {
>> >+ enum macsec_offload prev_offload;
>> >+ const struct macsec_ops *ops;
>> >+ struct macsec_context ctx;
>> >+ int ret = 0;
>> >+
>> >+ prev_offload = macsec->offload;
>> >+
>> >+ /* Check if the device already has rules configured: we do not support
>> >+ * rules migration.
>> >+ */
>> >+ if (macsec_is_configured(macsec))
>> >+ return -EBUSY;
>> >+
>> >+ 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];
>> >- enum macsec_offload offload, prev_offload;
>> >- int (*func)(struct macsec_context *ctx);
>> > struct nlattr **attrs = info->attrs;
>> >- struct net_device *dev;
>> >- const struct macsec_ops *ops;
>> >- struct macsec_context ctx;
>> >+ enum macsec_offload offload;
>> > struct macsec_dev *macsec;
>> >+ struct net_device *dev;
>> > int ret;
>> >
>> > if (!attrs[MACSEC_ATTR_IFINDEX]) @@ -2629,39 +2658,7 @@ static
>> >int macsec_upd_offload(struct sk_buff *skb, struct genl_info *info)
>> >
>> > 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;
>> >- }
>> >-
>> >- 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(macsec, offload);
>> >
>> > rtnl_unlock();
>> > return ret;
>> >@@ -3698,6 +3695,7 @@ static const struct nla_policy
>> macsec_rtnl_policy[IFLA_MACSEC_MAX + 1] = {
>> > [IFLA_MACSEC_SCB] = { .type = NLA_U8 },
>> > [IFLA_MACSEC_REPLAY_PROTECT] = { .type = NLA_U8 },
>> > [IFLA_MACSEC_VALIDATION] = { .type = NLA_U8 },
>> >+ [IFLA_MACSEC_OFFLOAD] = { .type = NLA_U8 },
>> > };
>> >
>> > static void macsec_free_netdev(struct net_device *dev) @@ -3803,6
>> >+3801,29 @@ static int macsec_changelink_common(struct net_device *dev,
>> > return 0;
>> > }
>> >
>> >+static int macsec_changelink_upd_offload(struct net_device *dev,
>> >+struct nlattr *data[]) {
>> >+ enum macsec_offload offload;
>> >+ struct macsec_dev *macsec;
>> >+
>> >+ macsec = macsec_priv(dev);
>> >+ offload = nla_get_u8(data[IFLA_MACSEC_OFFLOAD]);
>> >+
>> >+ 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;
>> >+
>> >+ return macsec_update_offload(macsec, offload); }
>> >+
>> > static int macsec_changelink(struct net_device *dev, struct nlattr *tb[],
>> > struct nlattr *data[],
>> > struct netlink_ext_ack *extack) @@ -3831,6
>> >+3852,12 @@ static int macsec_changelink(struct net_device *dev, struct nlattr
>> *tb[],
>> > if (ret)
>> > goto cleanup;
>> >
>> >+ if (data[IFLA_MACSEC_OFFLOAD]) {
>> >+ ret = macsec_changelink_upd_offload(dev, data);
>> >+ if (ret)
>> >+ goto cleanup;
>> >+ }
>> >+
>> > /* If h/w offloading is available, propagate to the device */
>> > if (macsec_is_offloaded(macsec)) {
>> > const struct macsec_ops *ops; @@ -4231,16 +4258,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; @@ -4265,6 +4298,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
>> >

Next message: Athira Rajeev: "Re: [PATCH 18/20] perf stat: Display event stats using aggr counts"
Previous message: Fabiano Rosas: "Re: [PATCH v10 2/9] KVM: Introduce per-page memory attributes"
In reply to: Emeel Hakim: "RE: [PATCH net-next v2] macsec: Add support for IFLA_MACSEC_OFFLOAD in the netlink layer"
Next in thread: Sabrina Dubroca: "Re: [PATCH net-next v2] macsec: Add support for IFLA_MACSEC_OFFLOAD in the netlink layer"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]