A few nit picks and questions, when you repost please make sure to CC
Sabrina Dubroca <[email protected]>
On Tue, 20 Jun 2023 11:13:01 +0200
[email protected] 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
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 <[email protected]>
Sent: Thursday, June 22, 2023 2:34 AM
To: Carlos Fernandez
Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; 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 <[email protected]>
On Tue, 20 Jun 2023 11:13:01 +0200
[email protected] 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
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 <[email protected]>
Sent: Thursday, June 22, 2023 10:00 AM
To: Jakub Kicinski
Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; 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 <[email protected]>
Sent: Thursday, June 22, 2023 2:34 AM
To: Carlos Fernandez
Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; 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 <[email protected]>
On Tue, 20 Jun 2023 11:13:01 +0200
[email protected] 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
Hi Carlos,
2023-06-22, 11:49:44 +0000, Carlos Fernandez wrote:
> 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.
Why is it a 1 item list? Even if that was guaranteed to be true in
normal conditions, we could have a situation where lots of MACsec
SecYs and RXSCs are set up, and packets start hitting this loop.
And could you quote the correct section of 802.1AE? I can't find the
reference for the behavior you're implementing in this patch. All I
can find is (from section 9.5):
The ES bit is clear if the Source Address is not used to determine the SCI.
The SC bit shall be clear if an SCI is not present in the SecTAG.
which doesn't say anything about how to interpret both bits being
clear.
(and please don't top-post)
Thanks,
--
Sabrina