| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <ZOduuY6cfXbgv_L3@hog>
Date: Thu, 24 Aug 2023 16:54:22 +0200
From: Sabrina Dubroca <sd@...asysnail.net>
To: "Radu Pirea (NXP OSS)" <radu-nicolae.pirea@....nxp.com>
Cc: andrew@...n.ch, hkallweit1@...il.com, linux@...linux.org.uk,
davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
pabeni@...hat.com, richardcochran@...il.com,
sebastian.tobuschat@....com, netdev@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [RFC net-next v2 2/5] net: macsec: introduce mdo_insert_tx_tag
2023-08-24, 12:16:12 +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 up
> to 32 bytes. If the frames are sent at line rate, the PHY will not have
> enough room to insert the SecTAG and the ICV.
>
> Some PHYs use a hardware buffer to store a number of ethernet frames and,
> if it fills up, a pause frame is sent to the MAC to control the flow.
> This HW implementation does not need any modification in the stack.
>
> Other PHYs might offer to use 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.
>
> mdo_insert_tx_tag allows the PHY drivers to add any specific tag in the
> skb.
Please add a per-patch changelog between versions. For example:
v2:
- add doc for the new fields in macsec_ops
- add insert_tx_tag to macsec_dev
- use unsigned int for macsec_ops.needed_{head,tail}room
[etc]
> Signed-off-by: Radu Pirea (NXP OSS) <radu-nicolae.pirea@....nxp.com>
> ---
> drivers/net/macsec.c | 96 +++++++++++++++++++++++++++++++++++++++++++-
> include/net/macsec.h | 10 +++++
> 2 files changed, 105 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/macsec.c b/drivers/net/macsec.c
> index ae60817ec5c2..5541aaced61f 100644
> --- a/drivers/net/macsec.c
> +++ b/drivers/net/macsec.c
> @@ -93,6 +93,7 @@ struct pcpu_secy_stats {
> * @secys: linked list of SecY's on the underlying device
> * @gro_cells: pointer to the Generic Receive Offload cell
> * @offload: status of offloading on the MACsec device
> + * @insert_tx_tag: insert tx tag if true
(probably a bit nitpicky)
Maybe briefly mention something about offloading and why devices might
needed that tag? Otherwise this doc feels a bit like it's there just
to make a checker happy, it doesn't say anything that "bool
insert_tx_tag" doesn't already tell us.
Maybe something like:
"when offloading, device requires to insert an additional tag"
> */
> struct macsec_dev {
> struct macsec_secy secy;
> @@ -102,6 +103,7 @@ struct macsec_dev {
> struct list_head secys;
> struct gro_cells gro_cells;
> enum macsec_offload offload;
> + bool insert_tx_tag;
> };
>
> /**
> @@ -2582,6 +2584,33 @@ static bool macsec_is_configured(struct macsec_dev *macsec)
> return false;
> }
>
> +static bool macsec_can_insert_tx_tag(struct macsec_dev *macsec,
It feels more like a "needs" than a "can" situation to me. The device
needs this tag inserted in order to fully work.
> + const struct macsec_ops *ops)
> +{
> + return macsec->offload == MACSEC_OFFLOAD_PHY &&
> + ops->mdo_insert_tx_tag;
> +}
> +
> +static void macsec_adjust_room(struct net_device *dev,
> + const struct macsec_ops *ops)
> +{
> + struct macsec_dev *macsec = macsec = macsec_priv(dev);
duplicate "macsec = macsec = ..."
> +
> + if (macsec_is_offloaded(macsec)) {
Shouldn't that whole adjustment (in both directions) depend on
->insert_tx_tag?
> + dev->needed_headroom -= MACSEC_NEEDED_HEADROOM;
> + dev->needed_headroom += ops->needed_headroom;
I would compute "diff = ops->needed_headroom - MACSEC_NEEDED_HEADROOM"
at the start and then we can simply do "+= diff" or "-= diff" (and
same for tailroom).
> + dev->needed_tailroom -= MACSEC_NEEDED_TAILROOM;
> + dev->needed_tailroom += ops->needed_tailroom;
> +
> + return;
> + }
nit: else instead of the early return would make things more
symmetrical.
> +
> + dev->needed_headroom -= ops->needed_headroom;
> + dev->needed_headroom += MACSEC_NEEDED_HEADROOM;
> + dev->needed_tailroom -= ops->needed_tailroom;
> + dev->needed_tailroom += MACSEC_NEEDED_TAILROOM;
> +}
> +
> static int macsec_update_offload(struct net_device *dev, enum macsec_offload offload)
> {
> enum macsec_offload prev_offload;
> @@ -2619,9 +2648,15 @@ static int macsec_update_offload(struct net_device *dev, enum macsec_offload off
> 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)
> + if (ret) {
> macsec->offload = prev_offload;
> + goto out;
I would prefer a direct return right here instead of this goto.
> + }
> +
> + macsec_adjust_room(dev, ops);
> + macsec->insert_tx_tag = macsec_can_insert_tx_tag(macsec, ops);
>
> +out:
> return ret;
> }
>
> @@ -3378,6 +3413,55 @@ 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 phy_device *phydev;
> + struct macsec_context ctx;
> + int err;
> +
> + if (!macsec->insert_tx_tag)
> + return skb;
I think it would look a bit nicer if this test was moved out, before
calling insert_tx_tag(). Then if we call insert_tx_tag(), we know we
have to insert it.
> + ops = macsec_get_ops(macsec, &ctx);
> + phydev = macsec->real_dev->phydev;
> +
[...]
> @@ -4125,6 +4216,9 @@ static int macsec_newlink(struct net *net, struct net_device *dev,
> err = macsec_offload(ops->mdo_add_secy, &ctx);
> if (err)
> goto del_dev;
> +
> + macsec_adjust_room(dev, ops);
> + macsec->insert_tx_tag = macsec_can_insert_tx_tag(macsec, ops);
> }
> }
>
> diff --git a/include/net/macsec.h b/include/net/macsec.h
> index 76f024727bb4..9577921897f9 100644
> --- a/include/net/macsec.h
> +++ b/include/net/macsec.h
> @@ -312,6 +312,11 @@ struct macsec_context {
> * @mdo_get_tx_sa_stats: called when TX SA stats are read
> * @mdo_get_rx_sc_stats: called when RX SC stats are read
> * @mdo_get_rx_sa_stats: called when RX SA stats are read
> + * @mdo_insert_tx_tag: called to insert the TX offload tag
> + * @needed_headroom: number of bytes reserved at the beginning of the sk_buff
> + * for the TX Tag
> + * @needed_tailroom: number of bytes reserved at the end of the sk_buff for the
> + * TX Tag
It would be nice to use a consistent name (either "TX offload tag" or
"TX tag") and case in those 3 descriptions (slight preference for
"tag" over "Tag" on my side).
I'd also add ", to be filled by mdo_insert_tx_tag" (not sure whether
that needs to be @mdo_insert_tx_tag or just mdo_insert_tx_tag) to the
needed_headroom/needed_tailroom descriptions, just to be really clear.
--
Sabrina
Powered by blists - more mailing lists