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

[Subbaraya Sundeep]

On 2025-06-09 at 06:47:02, Carlos Fernandez (carlos.fernandez@xxxxxxxxxxxxxxxxxxxxxxx) wrote:
> According to 802.1AE standard, when ES and SC flags in TCI are zero,
> used SCI should be the current active SC_RX. Current code uses the
> header MAC address. Without this patch, when ES flag is 0 (using a
> bridge or switch), header MAC will not fit the SCI and MACSec frames
> will be discarted.
> 
> In order to test this issue, MACsec link should be stablished between
> two interfaces, setting SC and ES flags to zero and a port identifier
> different than one. For example, using ip macsec tools:
> 
> ip link add link $ETH0 macsec0 type macsec port 11 send_sci off I
Looks like 'I' above is typo.
> end_station off
> ip macsec add macsec0 tx sa 0 pn 2 on key 01 $ETH1_KEY
> ip macsec add macsec0 rx port 11 address $ETH1_MAC
> ip macsec add macsec0 rx port 11 address $ETH1_MAC sa 0 pn 2 on key 02
> ip link set dev macsec0 up
> 
> ip link add link $ETH1 macsec1 type macsec port 11 send_sci off I
Ditto. Please fix these and resubmit.
With that you can add Reviewed-by: Subbaraya Sundeep <sbhatta@xxxxxxxxxxx>

Thanks,
Sundeep
> end_station off
> ip macsec add macsec1 tx sa 0 pn 2 on key 01 $ETH0_KEY
> ip macsec add macsec1 rx port 11 address $ETH0_MAC
> ip macsec add macsec1 rx port 11 address $ETH0_MAC sa 0 pn 2 on key 02
> ip link set dev macsec1 up
> 
> 
> Fixes: c09440f7dcb3 ("macsec: introduce IEEE 802.1AE driver")
> Co-developed-by: Andreu Montiel <Andreu.Montiel@xxxxxxxxxxxxxxxxxxxxxxx>
> Signed-off-by: Andreu Montiel <Andreu.Montiel@xxxxxxxxxxxxxxxxxxxxxxx>
> Signed-off-by: Carlos Fernandez <carlos.fernandez@xxxxxxxxxxxxxxxxxxxxxxx>
> ---
> v4: 
> * Added testing info in commit as suggested. 
> 
> v3: https://patchwork.kernel.org/project/netdevbpf/patch/20250604123407.2795263-1-carlos.fernandez@xxxxxxxxxxxxxxxxxxxxxxx/
> * Wrong drop frame afer macsec_frame_sci
> * Wrong Fixes tag in message 
> 
> v2: https://patchwork.kernel.org/project/netdevbpf/patch/20250604113213.2595524-1-carlos.fernandez@xxxxxxxxxxxxxxxxxxxxxxx/
> * Active sci lookup logic in a separate helper.
> * Unnecessary loops avoided. 
> * Check RXSC is exactly one for lower device.
> * Drops frame in case of error.
> 
> 
> v1: https://patchwork.kernel.org/project/netdevbpf/patch/20250529124455.2761783-1-carlos.fernandez@xxxxxxxxxxxxxxxxxxxxxxx/
> 
> 
>  drivers/net/macsec.c | 40 ++++++++++++++++++++++++++++++++++------
>  1 file changed, 34 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/macsec.c b/drivers/net/macsec.c
> index 3d315e30ee47..7edbe76b5455 100644
> --- a/drivers/net/macsec.c
> +++ b/drivers/net/macsec.c
> @@ -247,15 +247,39 @@ static sci_t make_sci(const u8 *addr, __be16 port)
>  	return sci;
>  }
>  
> -static sci_t macsec_frame_sci(struct macsec_eth_header *hdr, bool sci_present)
> +static sci_t macsec_active_sci(struct macsec_secy *secy)
>  {
> -	sci_t sci;
> +	struct macsec_rx_sc *rx_sc = rcu_dereference_bh(secy->rx_sc);
> +
> +	/* Case single RX SC */
> +	if (rx_sc && !rcu_dereference_bh(rx_sc->next))
> +		return (rx_sc->active) ? rx_sc->sci : 0;
> +	/* Case no RX SC or multiple */
> +	else
> +		return 0;
> +}
> +
> +static sci_t macsec_frame_sci(struct macsec_eth_header *hdr, bool sci_present,
> +			      struct macsec_rxh_data *rxd)
> +{
> +	struct macsec_dev *macsec;
> +	sci_t sci = 0;
>  
> -	if (sci_present)
> +	/* SC = 1 */
> +	if (sci_present) {
>  		memcpy(&sci, hdr->secure_channel_id,
>  		       sizeof(hdr->secure_channel_id));
> -	else
> +	/* SC = 0; ES = 0 */
> +	} else if ((!(hdr->tci_an & (MACSEC_TCI_ES | MACSEC_TCI_SC))) &&
> +		   (list_is_singular(&rxd->secys))) {
> +		/* Only one SECY should exist on this scenario */
> +		macsec = list_first_or_null_rcu(&rxd->secys, struct macsec_dev,
> +						secys);
> +		if (macsec)
> +			return macsec_active_sci(&macsec->secy);
> +	} else {
>  		sci = make_sci(hdr->eth.h_source, MACSEC_PORT_ES);
> +	}
>  
>  	return sci;
>  }
> @@ -1109,7 +1133,7 @@ static rx_handler_result_t macsec_handle_frame(struct sk_buff **pskb)
>  	struct macsec_rxh_data *rxd;
>  	struct macsec_dev *macsec;
>  	unsigned int len;
> -	sci_t sci;
> +	sci_t sci = 0;
>  	u32 hdr_pn;
>  	bool cbit;
>  	struct pcpu_rx_sc_stats *rxsc_stats;
> @@ -1156,11 +1180,14 @@ 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);
> +	if (!sci)
> +		goto drop_nosc;
> +
>  	list_for_each_entry_rcu(macsec, &rxd->secys, secys) {
>  		struct macsec_rx_sc *sc = find_rx_sc(&macsec->secy, sci);
>  
> @@ -1283,6 +1310,7 @@ static rx_handler_result_t macsec_handle_frame(struct sk_buff **pskb)
>  	macsec_rxsa_put(rx_sa);
>  drop_nosa:
>  	macsec_rxsc_put(rx_sc);
> +drop_nosc:
>  	rcu_read_unlock();
>  drop_direct:
>  	kfree_skb(skb);
> -- 
> 2.43.0
>