[PATCH] net: macsec: use TX SCI as MAC address

[PATCH] net: macsec: use TX SCI as MAC address

[Radu Pirea (NXP OSS)]

According to IEEE 802.1AE the SCI comprises the MAC address and the port
identifier.
If a new MACsec interface is created with a specific TX SCI, use that
SCI to set the MAC address of the new interface.

Signed-off-by: Radu Pirea (NXP OSS) <radu-nicolae.pirea@oss.nxp.com>
---
 drivers/net/macsec.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/net/macsec.c b/drivers/net/macsec.c
index 984dfa5d6c11..6db69daf880d 100644
--- a/drivers/net/macsec.c
+++ b/drivers/net/macsec.c
@@ -4103,12 +4103,14 @@ static int macsec_newlink(struct net *net, struct net_device *dev,
 	/* need to be already registered so that ->init has run and
 	 * the MAC addr is set
 	 */
-	if (data && data[IFLA_MACSEC_SCI])
+	if (data && data[IFLA_MACSEC_SCI]) {
 		sci = nla_get_sci(data[IFLA_MACSEC_SCI]);
-	else if (data && data[IFLA_MACSEC_PORT])
+		eth_hw_addr_set(dev, (u8 *)&sci);
+	} else if (data && data[IFLA_MACSEC_PORT]) {
 		sci = dev_to_sci(dev, nla_get_be16(data[IFLA_MACSEC_PORT]));
-	else
+	} else {
 		sci = dev_to_sci(dev, MACSEC_PORT_ES);
+	}
 
 	if (rx_handler && sci_exists(real_dev, sci)) {
 		err = -EBUSY;
-- 
2.34.1

Re: [PATCH] net: macsec: use TX SCI as MAC address

[Sabrina Dubroca]

2023-08-08, 17:14:29 +0300, Radu Pirea (NXP OSS) wrote:
> According to IEEE 802.1AE the SCI comprises the MAC address and the port
> identifier.

I don't think the SCI needs to be composed of the actual device's MAC
address. 8.2.1 says that the MAC address *can* be used to compose the
SCI, but doesn't mandate it.

If you want the SCI to match the device's MAC address, why not use
IFLA_MACSEC_PORT instead?

> If a new MACsec interface is created with a specific TX SCI, use that
> SCI to set the MAC address of the new interface.
> 
> Signed-off-by: Radu Pirea (NXP OSS) <radu-nicolae.pirea@oss.nxp.com>
> ---
>  drivers/net/macsec.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/macsec.c b/drivers/net/macsec.c
> index 984dfa5d6c11..6db69daf880d 100644
> --- a/drivers/net/macsec.c
> +++ b/drivers/net/macsec.c
> @@ -4103,12 +4103,14 @@ static int macsec_newlink(struct net *net, struct net_device *dev,
>  	/* need to be already registered so that ->init has run and
>  	 * the MAC addr is set
>  	 */
> -	if (data && data[IFLA_MACSEC_SCI])
> +	if (data && data[IFLA_MACSEC_SCI]) {
>  		sci = nla_get_sci(data[IFLA_MACSEC_SCI]);
> -	else if (data && data[IFLA_MACSEC_PORT])
> +		eth_hw_addr_set(dev, (u8 *)&sci);
> +	} else if (data && data[IFLA_MACSEC_PORT]) {
>  		sci = dev_to_sci(dev, nla_get_be16(data[IFLA_MACSEC_PORT]));
> -	else
> +	} else {
>  		sci = dev_to_sci(dev, MACSEC_PORT_ES);
> +	}
>  
>  	if (rx_handler && sci_exists(real_dev, sci)) {
>  		err = -EBUSY;
> -- 
> 2.34.1
> 

-- 
Sabrina

Re: [PATCH] net: macsec: use TX SCI as MAC address

[Radu Pirea (OSS)]

On 08.08.2023 18:22, Sabrina Dubroca wrote:
> 2023-08-08, 17:14:29 +0300, Radu Pirea (NXP OSS) wrote:
>> According to IEEE 802.1AE the SCI comprises the MAC address and the port
>> identifier.
> 
> I don't think the SCI needs to be composed of the actual device's MAC
> address. 8.2.1 says that the MAC address *can* be used to compose the
> SCI, but doesn't mandate it.
I used IEEE 802.1AE-2018 as documentation and the text is slightly 
different. However, the purpose of this patch is not to force this match 
between the MAC address and the SCI, is just to have different MAC 
addresses when the interfaces are created with an specific SCI.

For example, the following command will not set 00:01:be:be:ef:17 as MAC 
address for the new interface. Would you expect that?
ip link add link enet_p2 macsec0 type macsec address 00:01:be:be:ef:17 
port 1 encrypt on

> 
> If you want the SCI to match the device's MAC address, why not use
> IFLA_MACSEC_PORT instead?

In this case, if no MAC address is specified, it makes sense to inherit 
the MAC address from the real netdev.

> 
>> If a new MACsec interface is created with a specific TX SCI, use that
>> SCI to set the MAC address of the new interface.
>>
>> Signed-off-by: Radu Pirea (NXP OSS) <radu-nicolae.pirea@oss.nxp.com>
>> ---
>>   drivers/net/macsec.c | 8 +++++---
>>   1 file changed, 5 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/net/macsec.c b/drivers/net/macsec.c
>> index 984dfa5d6c11..6db69daf880d 100644
>> --- a/drivers/net/macsec.c
>> +++ b/drivers/net/macsec.c
>> @@ -4103,12 +4103,14 @@ static int macsec_newlink(struct net *net, struct net_device *dev,
>>   	/* need to be already registered so that ->init has run and
>>   	 * the MAC addr is set
>>   	 */
>> -	if (data && data[IFLA_MACSEC_SCI])
>> +	if (data && data[IFLA_MACSEC_SCI]) {
>>   		sci = nla_get_sci(data[IFLA_MACSEC_SCI]);
>> -	else if (data && data[IFLA_MACSEC_PORT])
>> +		eth_hw_addr_set(dev, (u8 *)&sci);
>> +	} else if (data && data[IFLA_MACSEC_PORT]) {
>>   		sci = dev_to_sci(dev, nla_get_be16(data[IFLA_MACSEC_PORT]));
>> -	else
>> +	} else {
>>   		sci = dev_to_sci(dev, MACSEC_PORT_ES);
>> +	}
>>   
>>   	if (rx_handler && sci_exists(real_dev, sci)) {
>>   		err = -EBUSY;
>> -- 
>> 2.34.1
>>
> 

-- 
Radu P.

Re: [PATCH] net: macsec: use TX SCI as MAC address

[Sabrina Dubroca]

2023-08-09, 09:37:40 +0300, Radu Pirea (OSS) wrote:
> 
> 
> On 08.08.2023 18:22, Sabrina Dubroca wrote:
> > 2023-08-08, 17:14:29 +0300, Radu Pirea (NXP OSS) wrote:
> > > According to IEEE 802.1AE the SCI comprises the MAC address and the port
> > > identifier.
> > 
> > I don't think the SCI needs to be composed of the actual device's MAC
> > address. 8.2.1 says that the MAC address *can* be used to compose the
> > SCI, but doesn't mandate it.
> I used IEEE 802.1AE-2018 as documentation and the text is slightly
> different. However, the purpose of this patch is not to force this match
> between the MAC address and the SCI, is just to have different MAC addresses
> when the interfaces are created with an specific SCI.
> 
> For example, the following command will not set 00:01:be:be:ef:17 as MAC
> address for the new interface. Would you expect that?
> ip link add link enet_p2 macsec0 type macsec address 00:01:be:be:ef:17 port
> 1 encrypt on

Yes, because "address XXX" comes after "type macsec", so it's an
argument of "type macsec", not of "ip link". IMO the manpage is pretty
clear about this.

The command you want is:

ip link add link enet_p2 macsec0 addr 00:01:be:be:ef:17 type macsec port 1 encrypt on

And with this, I don't think your patch is needed at all. It would
even introduce an undesireable behavior, in case an explicit address
is provided (as in my command example) alongside a full SCI (instead
of just the port).

-- 
Sabrina

Re: [PATCH] net: macsec: use TX SCI as MAC address

[Radu Pirea (OSS)]

On 09.08.2023 15:10, Sabrina Dubroca wrote:
> 2023-08-09, 09:37:40 +0300, Radu Pirea (OSS) wrote:
>>
>>
>> On 08.08.2023 18:22, Sabrina Dubroca wrote:
>>> 2023-08-08, 17:14:29 +0300, Radu Pirea (NXP OSS) wrote:
>>>> According to IEEE 802.1AE the SCI comprises the MAC address and the port
>>>> identifier.
>>>
>>> I don't think the SCI needs to be composed of the actual device's MAC
>>> address. 8.2.1 says that the MAC address *can* be used to compose the
>>> SCI, but doesn't mandate it.
>> I used IEEE 802.1AE-2018 as documentation and the text is slightly
>> different. However, the purpose of this patch is not to force this match
>> between the MAC address and the SCI, is just to have different MAC addresses
>> when the interfaces are created with an specific SCI.
>>
>> For example, the following command will not set 00:01:be:be:ef:17 as MAC
>> address for the new interface. Would you expect that?
>> ip link add link enet_p2 macsec0 type macsec address 00:01:be:be:ef:17 port
>> 1 encrypt on
> 
> Yes, because "address XXX" comes after "type macsec", so it's an
> argument of "type macsec", not of "ip link". IMO the manpage is pretty
> clear about this.
> 
> The command you want is:
> 
> ip link add link enet_p2 macsec0 addr 00:01:be:be:ef:17 type macsec port 1 encrypt on
Now I see...

> 
> And with this, I don't think your patch is needed at all. It would
> even introduce an undesireable behavior, in case an explicit address
> is provided (as in my command example) alongside a full SCI (instead
> of just the port).
> I agree. Thank you.

-- 
Radu P.