Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
Content-Transfer-Encoding: quoted-printable
In-Reply-To: <20220505123803.17553-1-carlos.fernandez@technica-engineering.de>
References: <[PATCH] net/macsec copy salt to MACSec ctx for XPN> <20220505123803.17553-1-carlos.fernandez@technica-engineering.de>
Subject: Re: [PATCH] net: macsec: XPN Salt copied before passing offload context
To:     Carlos Fernandez <carlos.escuin@gmail.com>,
        carlos.fernandez@technica-enineering.de, davem@davemloft.net,
        edumazet@google.com, kuba@kernel.org, linux-kernel@vger.kernel.org,
        netdev@vger.kernel.org, pabeni@redhat.com
Cc:     Carlos Fernandez <carlos.fernandez@technica-engineering.de>
From:   Antoine Tenart <atenart@kernel.org>
Message-ID: <165175728541.217313.17093503108252590709@kwain>
Date:   Thu, 05 May 2022 15:28:05 +0200
Precedence: bulk

Hello,

(Note: please use "[PATCH net]" for fixes and "[PATCH net-next]" for
improvements in the subject when submitting patches to the networking
subsystem).

Quoting Carlos Fernandez (2022-05-05 14:38:03)
> When macsec offloading is used with XPN, before mdo_add_rxsa
> and mdo_add_txsa functions are called, the key salt is not
> copied to the macsec context struct.
>=20
> Fix by copying salt to context struct before calling the
> offloading functions.

The commit message and title are referring to the XPN salt only, but
there is another XPN specific entry being moved by this commit. I would
suggest to update the commit title to:
"net: macsec: retrieve the XPN attributes before offloading"

> Fixes: 48ef50fa866a ("macsec: Netlink support of XPN cipher suites")
> Signed-off-by: Carlos Fernandez <carlos.fernandez@technica-engineering.de>
> ---
>  drivers/net/macsec.c | 30 ++++++++++++++++--------------
>  1 file changed, 16 insertions(+), 14 deletions(-)
>=20
> diff --git a/drivers/net/macsec.c b/drivers/net/macsec.c
> index 832f09ac075e..4f2bd3d722c3 100644
> --- a/drivers/net/macsec.c
> +++ b/drivers/net/macsec.c
> @@ -1804,6 +1804,14 @@ static int macsec_add_rxsa(struct sk_buff *skb, st=
ruct genl_info *info)
> =20
>         rx_sa->sc =3D rx_sc;
> =20
> +       if (secy->xpn) {
> +               rx_sa->ssci =3D nla_get_ssci(tb_sa[MACSEC_SA_ATTR_SSCI]);
> +               nla_memcpy(rx_sa->key.salt.bytes, tb_sa[MACSEC_SA_ATTR_SA=
LT],
> +                          MACSEC_SALT_LEN);
> +       }
> +
> +       nla_memcpy(rx_sa->key.id, tb_sa[MACSEC_SA_ATTR_KEYID], MACSEC_KEY=
ID_LEN);

Is the key id part related to the XPN offloading not working?

Otherwise, it makes sense to copy all attributes before offloading the
operation but this should probably be in its own patch targeted at
net-next. (Same for the txsa part).

>         /* If h/w offloading is available, propagate to the device */
>         if (macsec_is_offloaded(netdev_priv(dev))) {
>                 const struct macsec_ops *ops;
> @@ -1826,13 +1834,6 @@ static int macsec_add_rxsa(struct sk_buff *skb, st=
ruct genl_info *info)
>                         goto cleanup;
>         }
> =20
> -       if (secy->xpn) {
> -               rx_sa->ssci =3D nla_get_ssci(tb_sa[MACSEC_SA_ATTR_SSCI]);
> -               nla_memcpy(rx_sa->key.salt.bytes, tb_sa[MACSEC_SA_ATTR_SA=
LT],
> -                          MACSEC_SALT_LEN);
> -       }
> -
> -       nla_memcpy(rx_sa->key.id, tb_sa[MACSEC_SA_ATTR_KEYID], MACSEC_KEY=
ID_LEN);
>         rcu_assign_pointer(rx_sc->sa[assoc_num], rx_sa);
> =20
>         rtnl_unlock();

Thanks!
Antoine