Offloading MACsec in PHYs requires inserting the SecTAG and the ICV in
the ethernet frame. This operation will increase the frame size with 32
bytes. If the frames are sent at line rate, the PHY will not have enough
room to insert the SecTAG and the ICV.
To mitigate this scenario, the PHY offer to use require a specific
ethertype with some padding bytes present in the ethernet frame. This
ethertype and its associated bytes will be replaced by the SecTAG and ICV.
Signed-off-by: Radu Pirea (NXP OSS) <radu-nicolae.pirea@oss.nxp.com>
---
drivers/net/macsec.c | 79 ++++++++++++++++++++++++++++++++++++++++++++
include/net/macsec.h | 5 +++
2 files changed, 84 insertions(+)
diff --git a/drivers/net/macsec.c b/drivers/net/macsec.c
index 144ec756c796..32ea1fd5f5ab 100644
--- a/drivers/net/macsec.c
+++ b/drivers/net/macsec.c
@@ -2633,6 +2633,18 @@ static int macsec_update_offload(struct net_device *dev, enum macsec_offload off
if (ret)
macsec->offload = prev_offload;
+ if (macsec->offload == MACSEC_OFFLOAD_OFF) {
+ dev->needed_headroom -= ops->needed_headroom;
+ dev->needed_headroom += MACSEC_NEEDED_HEADROOM;
+ dev->needed_tailroom -= ops->needed_tailroom;
+ dev->needed_tailroom += MACSEC_NEEDED_TAILROOM;
+ } else {
+ dev->needed_headroom -= MACSEC_NEEDED_HEADROOM;
+ dev->needed_headroom += ops->needed_headroom;
+ dev->needed_tailroom -= MACSEC_NEEDED_TAILROOM;
+ dev->needed_tailroom += ops->needed_tailroom;
+ }
+
return ret;
}
@@ -3389,6 +3401,61 @@ static struct genl_family macsec_fam __ro_after_init = {
.resv_start_op = MACSEC_CMD_UPD_OFFLOAD + 1,
};
+static struct sk_buff *insert_tx_tag(struct sk_buff *skb,
+ struct net_device *dev)
+{
+ struct macsec_dev *macsec = macsec_priv(dev);
+ const struct macsec_ops *ops;
+ struct macsec_context ctx;
+ int err;
+
+ if (!macsec_is_offloaded(macsec))
+ return ERR_PTR(-EINVAL);
+
+ ops = macsec_get_ops(macsec, &ctx);
+ if (!ops)
+ return ERR_PTR(-EINVAL);
+
+ if (!ops->mdo_insert_tx_tag)
+ return skb;
+
+ if (unlikely(skb_headroom(skb) < ops->needed_headroom ||
+ skb_tailroom(skb) < ops->needed_tailroom)) {
+ struct sk_buff *nskb = skb_copy_expand(skb,
+ ops->needed_headroom,
+ ops->needed_tailroom,
+ GFP_ATOMIC);
+ if (likely(nskb)) {
+ consume_skb(skb);
+ skb = nskb;
+ } else {
+ err = -ENOMEM;
+ goto cleanup;
+ }
+ } else {
+ skb = skb_unshare(skb, GFP_ATOMIC);
+ if (!skb)
+ return ERR_PTR(-ENOMEM);
+ }
+
+ if (skb->len - ETH_HLEN > macsec_priv(dev)->real_dev->mtu) {
+ err = -EINVAL;
+ goto cleanup;
+ }
+
+ ctx.secy = &macsec->secy;
+ ctx.skb = skb;
+
+ err = ops->mdo_insert_tx_tag(&ctx);
+ if (err)
+ goto cleanup;
+
+ return skb;
+cleanup:
+ kfree_skb(skb);
+ return ERR_PTR(err);
+}
+
static netdev_tx_t macsec_start_xmit(struct sk_buff *skb,
struct net_device *dev)
{
@@ -3403,6 +3470,13 @@ static netdev_tx_t macsec_start_xmit(struct sk_buff *skb,
skb_dst_drop(skb);
dst_hold(&md_dst->dst);
skb_dst_set(skb, &md_dst->dst);
+
+ skb = insert_tx_tag(skb, dev);
+ if (IS_ERR(skb)) {
+ dev->stats.tx_dropped++;
+ return NETDEV_TX_OK;
+ }
+
skb->dev = macsec->real_dev;
return dev_queue_xmit(skb);
}
@@ -4137,6 +4211,11 @@ static int macsec_newlink(struct net *net, struct net_device *dev,
if (err)
goto del_dev;
}
+
+ dev->needed_headroom -= MACSEC_NEEDED_HEADROOM;
+ dev->needed_headroom += ops->needed_headroom;
+ dev->needed_tailroom -= MACSEC_NEEDED_TAILROOM;
+ dev->needed_tailroom += ops->needed_tailroom;
}
err = register_macsec_dev(real_dev, dev);
diff --git a/include/net/macsec.h b/include/net/macsec.h
index 33dc7f2aa42e..a988249d9608 100644
--- a/include/net/macsec.h
+++ b/include/net/macsec.h
@@ -272,6 +272,7 @@ struct macsec_context {
struct macsec_rx_sa_stats *rx_sa_stats;
struct macsec_dev_stats *dev_stats;
} stats;
+ struct sk_buff *skb;
};
/**
@@ -302,6 +303,10 @@ struct macsec_ops {
int (*mdo_get_tx_sa_stats)(struct macsec_context *ctx);
int (*mdo_get_rx_sc_stats)(struct macsec_context *ctx);
int (*mdo_get_rx_sa_stats)(struct macsec_context *ctx);
+ /* Offload tag */
+ int (*mdo_insert_tx_tag)(struct macsec_context *ctx);
+ int needed_headroom;
+ int needed_tailroom;
};
#if IS_ENABLED(CONFIG_MACSEC)
--
2.34.12023-08-11, 18:32:48 +0300, Radu Pirea (NXP OSS) wrote:
> Offloading MACsec in PHYs requires inserting the SecTAG and the ICV in
> the ethernet frame. This operation will increase the frame size with 32
> bytes.
"up to 32 bytes"?
The SecTAG and ICV can both be shorter, at least with the software
implementation.
[...]
> +static struct sk_buff *insert_tx_tag(struct sk_buff *skb,
> + struct net_device *dev)
> +{
[...]
> +
> + ctx.secy = &macsec->secy;
> + ctx.skb = skb;
I think it would be a bit more readable to just pass the skb to
->mdo_insert_tx_tag instead of adding it to the context.
> +
> + err = ops->mdo_insert_tx_tag(&ctx);
> + if (err)
> + goto cleanup;
[...]
> @@ -3403,6 +3470,13 @@ static netdev_tx_t macsec_start_xmit(struct sk_buff *skb,
> skb_dst_drop(skb);
> dst_hold(&md_dst->dst);
> skb_dst_set(skb, &md_dst->dst);
> +
> + skb = insert_tx_tag(skb, dev);
> + if (IS_ERR(skb)) {
> + dev->stats.tx_dropped++;
That should probably use DEV_STATS_INC (see commit
32d0a49d36a2 ("macsec: use DEV_STATS_INC()")).
> + return NETDEV_TX_OK;
> + }
> +
> skb->dev = macsec->real_dev;
> return dev_queue_xmit(skb);
> }
> @@ -4137,6 +4211,11 @@ static int macsec_newlink(struct net *net, struct net_device *dev,
> if (err)
> goto del_dev;
> }
> +
> + dev->needed_headroom -= MACSEC_NEEDED_HEADROOM;
> + dev->needed_headroom += ops->needed_headroom;
> + dev->needed_tailroom -= MACSEC_NEEDED_TAILROOM;
> + dev->needed_tailroom += ops->needed_tailroom;
If the driver doesn't set ops->needed_headroom, we'll subtract
MACSEC_NEEDED_HEADROOM and not add anything back. Is that correct for
all existing drivers? (and same for tailroom)
You set needed_tailroom to 0 in your driver, but the commit message
for this patch says that the HW needs space for the ICV. I'm a bit
puzzled by this, especially since MACSEC_NEEDED_TAILROOM already
reserves space for the ICV.
Also, since this is pattern repeated twice more (with a sign change)
in macsec_update_offload, we could probably stuff this into a helper
(either modifying dev->needed_headroom directly, or returning the
value to add/subtract).
> }
>
[...]
> @@ -302,6 +303,10 @@ struct macsec_ops {
> int (*mdo_get_tx_sa_stats)(struct macsec_context *ctx);
> int (*mdo_get_rx_sc_stats)(struct macsec_context *ctx);
> int (*mdo_get_rx_sa_stats)(struct macsec_context *ctx);
> + /* Offload tag */
> + int (*mdo_insert_tx_tag)(struct macsec_context *ctx);
> + int needed_headroom;
> + int needed_tailroom;
unsigned?
> };
--
Sabrina
On Fri, Aug 11, 2023 at 06:32:48PM +0300, Radu Pirea (NXP OSS) wrote:
> Offloading MACsec in PHYs requires inserting the SecTAG and the ICV in
> the ethernet frame. This operation will increase the frame size with 32
> bytes. If the frames are sent at line rate, the PHY will not have enough
> room to insert the SecTAG and the ICV.
>
> To mitigate this scenario, the PHY offer to use require a specific
> ethertype with some padding bytes present in the ethernet frame. This
> ethertype and its associated bytes will be replaced by the SecTAG and ICV.
>
> Signed-off-by: Radu Pirea (NXP OSS) <radu-nicolae.pirea@oss.nxp.com>
...
> diff --git a/drivers/net/macsec.c b/drivers/net/macsec.c
...
> @@ -4137,6 +4211,11 @@ static int macsec_newlink(struct net *net, struct net_device *dev,
> if (err)
> goto del_dev;
> }
> +
> + dev->needed_headroom -= MACSEC_NEEDED_HEADROOM;
> + dev->needed_headroom += ops->needed_headroom;
> + dev->needed_tailroom -= MACSEC_NEEDED_TAILROOM;
> + dev->needed_tailroom += ops->needed_tailroom;
Hi Radu,
Just above the beginning of this hunk it is assumed that ops may be NULL.
However, here it is dereferenced unconditionally. Is this safe?
Flagged by Smatch.
> }
>
> err = register_macsec_dev(real_dev, dev);
> diff --git a/include/net/macsec.h b/include/net/macsec.h
> index 33dc7f2aa42e..a988249d9608 100644
> --- a/include/net/macsec.h
> +++ b/include/net/macsec.h
> @@ -272,6 +272,7 @@ struct macsec_context {
> struct macsec_rx_sa_stats *rx_sa_stats;
> struct macsec_dev_stats *dev_stats;
> } stats;
> + struct sk_buff *skb;
Not strictly related to this patch,
but it would be nice to update the kernel doc for this
structure so that it's fields are documented.
> };
>
> /**
> @@ -302,6 +303,10 @@ struct macsec_ops {
> int (*mdo_get_tx_sa_stats)(struct macsec_context *ctx);
> int (*mdo_get_rx_sc_stats)(struct macsec_context *ctx);
> int (*mdo_get_rx_sa_stats)(struct macsec_context *ctx);
> + /* Offload tag */
> + int (*mdo_insert_tx_tag)(struct macsec_context *ctx);
> + int needed_headroom;
> + int needed_tailroom;
Ditto.
> };
>
> #if IS_ENABLED(CONFIG_MACSEC)
> --
> 2.34.1
>
>
On 12.08.2023 21:07, Simon Horman wrote:
> On Fri, Aug 11, 2023 at 06:32:48PM +0300, Radu Pirea (NXP OSS) wrote:
>> @@ -4137,6 +4211,11 @@ static int macsec_newlink(struct net *net, struct net_device *dev,
>> if (err)
>> goto del_dev;
>> }
>> +
>> + dev->needed_headroom -= MACSEC_NEEDED_HEADROOM;
>> + dev->needed_headroom += ops->needed_headroom;
>> + dev->needed_tailroom -= MACSEC_NEEDED_TAILROOM;
>> + dev->needed_tailroom += ops->needed_tailroom;
>
> Hi Radu,
>
> Just above the beginning of this hunk it is assumed that ops may be NULL.
> However, here it is dereferenced unconditionally. Is this safe?
Of course it's not safe.
Thank you.
> Flagged by Smatch.
>
>> }
>>
>> err = register_macsec_dev(real_dev, dev);
>> diff --git a/include/net/macsec.h b/include/net/macsec.h
>> index 33dc7f2aa42e..a988249d9608 100644
>> --- a/include/net/macsec.h
>> +++ b/include/net/macsec.h
>> @@ -272,6 +272,7 @@ struct macsec_context {
>> struct macsec_rx_sa_stats *rx_sa_stats;
>> struct macsec_dev_stats *dev_stats;
>> } stats;
>> + struct sk_buff *skb;
>
> Not strictly related to this patch,
> but it would be nice to update the kernel doc for this
> structure so that it's fields are documented.
Agreed. I will update the documentation.
--
Radu P.
On Fri, Aug 11, 2023 at 06:32:48PM +0300, Radu Pirea (NXP OSS) wrote:
> Offloading MACsec in PHYs requires inserting the SecTAG and the ICV in
> the ethernet frame. This operation will increase the frame size with 32
> bytes. If the frames are sent at line rate, the PHY will not have enough
> room to insert the SecTAG and the ICV.
>
> To mitigate this scenario, the PHY offer to use require a specific
> ethertype with some padding bytes present in the ethernet frame. This
> ethertype and its associated bytes will be replaced by the SecTAG and ICV.
I think this could be worded better, to take into account different
implementations. As far as i understand, some PHYs include a MAC,
which reassembles the frame, and then places the frame into a queue
for processing. After processing, a second MAC does the actual send on
the wire. The queue allows for some number of back to back frames
without having problems. The PHY then uses flow control pause to slow
down the SoC MAC when there is a long burst of line rate frames which
would otherwise overflow the queue.
So:
> If the frames are sent at line rate, the PHY will not have enough
> room to insert the SecTAG and the ICV.
This probably want to clarify that a PHY which does not buffer....
> To mitigate this scenario, the PHY offer to use require a specific
and here you want to say some PHYs offer, since not all PHYs will do
this.
> + if (macsec->offload == MACSEC_OFFLOAD_OFF) {
> + dev->needed_headroom -= ops->needed_headroom;
> + dev->needed_headroom += MACSEC_NEEDED_HEADROOM;
> + dev->needed_tailroom -= ops->needed_tailroom;
> + dev->needed_tailroom += MACSEC_NEEDED_TAILROOM;
> + } else {
> + dev->needed_headroom -= MACSEC_NEEDED_HEADROOM;
> + dev->needed_headroom += ops->needed_headroom;
> + dev->needed_tailroom -= MACSEC_NEEDED_TAILROOM;
> + dev->needed_tailroom += ops->needed_tailroom;
> + }
It is not obvious to me what this is doing. Should this actually be in
macsec_dev_init()? My main problem is why there is an else condition?
> +static struct sk_buff *insert_tx_tag(struct sk_buff *skb,
> + struct net_device *dev)
> +{
> + struct macsec_dev *macsec = macsec_priv(dev);
> + const struct macsec_ops *ops;
> + struct macsec_context ctx;
> + int err;
> +
> + if (!macsec_is_offloaded(macsec))
> + return ERR_PTR(-EINVAL);
Hasn't this already been checked in macsec_start_xmit()?
> +
> + ops = macsec_get_ops(macsec, &ctx);
> + if (!ops)
> + return ERR_PTR(-EINVAL);
> +
> + if (!ops->mdo_insert_tx_tag)
> + return skb;
You are in the hot path here. You don't expect this to change from
frame to frame. So could you evaluate this once and store it
somewhere? Maybe in macsec_dev ?
Andrew
On 11.08.2023 20:42, Andrew Lunn wrote:
> On Fri, Aug 11, 2023 at 06:32:48PM +0300, Radu Pirea (NXP OSS) wrote:
>
>> + if (macsec->offload == MACSEC_OFFLOAD_OFF) {
>> + dev->needed_headroom -= ops->needed_headroom;
>> + dev->needed_headroom += MACSEC_NEEDED_HEADROOM;
>> + dev->needed_tailroom -= ops->needed_tailroom;
>> + dev->needed_tailroom += MACSEC_NEEDED_TAILROOM;
>> + } else {
>> + dev->needed_headroom -= MACSEC_NEEDED_HEADROOM;
>> + dev->needed_headroom += ops->needed_headroom;
>> + dev->needed_tailroom -= MACSEC_NEEDED_TAILROOM;
>> + dev->needed_tailroom += ops->needed_tailroom;
>> + }
>
> It is not obvious to me what this is doing. Should this actually be in
> macsec_dev_init()? My main problem is why there is an else condition?
The user can enable/disable offloading after the interface is created,
that's why the else condition is needed.
>> +static struct sk_buff *insert_tx_tag(struct sk_buff *skb,
>> + struct net_device *dev)
>> +{
>> + struct macsec_dev *macsec = macsec_priv(dev);
>> + const struct macsec_ops *ops;
>> + struct macsec_context ctx;
>> + int err;
>> +
>> + if (!macsec_is_offloaded(macsec))
>> + return ERR_PTR(-EINVAL);
>
> Hasn't this already been checked in macsec_start_xmit()
Yes. This check is useless.
>
>> +
>> + ops = macsec_get_ops(macsec, &ctx);
>> + if (!ops)
>> + return ERR_PTR(-EINVAL);
>> +
>> + if (!ops->mdo_insert_tx_tag)
>> + return skb;
>
> You are in the hot path here. You don't expect this to change from
> frame to frame. So could you evaluate this once and store it
> somewhere? Maybe in macsec_dev ?
The macsec_dev struct seems to be the right place.
>
> Andrew
--
Radu P.
© 2016 - 2025 Red Hat, Inc.