Re: [PATCH v3] net: macsec SCI assignment for ES = 0

[Carlos Fernandez]

Hi Jakub, 

Sure, I'll add Sabrina Dubroca and add your suggestions in the next patch version.

The return of the rx_sci is the goal of this patch. When ES = 0 and SC = 0, we should return it instead of the hdr MAC addr. If not, MACSec communication will not work when using a switch between the transmitter and receiver, frames will be dropped.

Thanks, 

________________________________________
From: Jakub Kicinski <kuba@xxxxxxxxxx>
Sent: Thursday, June 22, 2023 2:34 AM
To: Carlos Fernandez
Cc: davem@xxxxxxxxxxxxx; edumazet@xxxxxxxxxx; pabeni@xxxxxxxxxx; netdev@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Sabrina Dubroca
Subject: Re: [PATCH v3] net: macsec SCI assignment for ES = 0

CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe.

A few nit picks and questions, when you repost please make sure to CC
Sabrina Dubroca <sd@xxxxxxxxxxxxxxx>

On Tue, 20 Jun 2023 11:13:01 +0200
carlos.fernandez@xxxxxxxxxxxxxxxxxxxxxxx wrote:
> -static sci_t macsec_frame_sci(struct macsec_eth_header *hdr, bool sci_present)
> +static sci_t macsec_frame_sci(struct macsec_eth_header *hdr, bool sci_present,
> +                           struct macsec_rxh_data *rxd)
>  {
> +     struct macsec_dev *macsec_device;
>       sci_t sci;
>
> -     if (sci_present)
> +     if (sci_present) {
>               memcpy(&sci, hdr->secure_channel_id,
> -                    sizeof(hdr->secure_channel_id));
> -     else
> +                     sizeof(hdr->secure_channel_id));

the alignment of sizeof() was correct, don't change it

> +     } else if (0 == (hdr->tci_an & (MACSEC_TCI_ES | MACSEC_TCI_SC))) {

Just
        } else if (hdr->tci_an & (MACSEC_TCI_ES | MACSEC_TCI_SC)) {

> +             list_for_each_entry_rcu(macsec_device, &rxd->secys, secys) {
> +                     struct macsec_rx_sc *rx_sc;
> +                     struct macsec_secy *secy = &macsec_device->secy;

You should reorder these two declaration, networking likes local
variable declaration lines longest to shortest.

> +                     for_each_rxsc(secy, rx_sc) {
> +                             rx_sc = rx_sc ? macsec_rxsc_get(rx_sc) : NULL;
> +                             if (rx_sc && rx_sc->active)
> +                                     return rx_sc->sci;
> +                     }

I haven't looked in detail but are you possibly returning rx_sc->sci
here just to ...

> +             }
> +             /* If not found, use MAC in hdr as default*/
>               sci = make_sci(hdr->eth.h_source, MACSEC_PORT_ES);
> -
> +     } else {
> +             sci = make_sci(hdr->eth.h_source, MACSEC_PORT_ES);
> +     }
>       return sci;
>  }
>
> @@ -1150,11 +1165,12 @@ static rx_handler_result_t macsec_handle_frame(struct sk_buff **pskb)
>
>       macsec_skb_cb(skb)->has_sci = !!(hdr->tci_an & MACSEC_TCI_SC);
>       macsec_skb_cb(skb)->assoc_num = hdr->tci_an & MACSEC_AN_MASK;
> -     sci = macsec_frame_sci(hdr, macsec_skb_cb(skb)->has_sci);
>
>       rcu_read_lock();
>       rxd = macsec_data_rcu(skb->dev);
>
> +     sci = macsec_frame_sci(hdr, macsec_skb_cb(skb)->has_sci, rxd);
> +
>       list_for_each_entry_rcu(macsec, &rxd->secys, secys) {
>               struct macsec_rx_sc *sc = find_rx_sc(&macsec->secy, sci);

... look up the rx_sc based on the sci?
--
pw-bot: cr