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

[Carlos Fernandez]

Hi Jakub,

Also, about the double look up:
I know it's there, but I tried to only change the function that returns the correct SCI in every case. Also, it should be a 1 and only item list. I do not think this should be dangerous or slow.

Thanks,

________________________________________
From: Carlos Fernandez <carlos.fernandez@xxxxxxxxxxxxxxxxxxxxxxx>
Sent: Thursday, June 22, 2023 10:00 AM
To: Jakub Kicinski
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.

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