drivers/net/macsec.c | 28 ++++++++++++++++++++++------ 1 file changed, 22 insertions(+), 6 deletions(-)
From: Carlos Fernandez <carlos.fernandez@technica-engineering.de>
According to 802.1AE standard, when ES and SC flags in TCI are zero, used
SCI should be the current active SC_RX. Current kernel does not implement
it and uses the header MAC address.
Without this patch, when ES = 0 (using a bridge or switch), header MAC
will not fit the SCI and MACSec frames will be discarted.
Signed-off-by: Carlos Fernandez <carlos.fernandez@technica-engineering.de>
---
drivers/net/macsec.c | 28 ++++++++++++++++++++++------
1 file changed, 22 insertions(+), 6 deletions(-)
diff --git a/drivers/net/macsec.c b/drivers/net/macsec.c
index 3427993f94f7..ea9b15d555f4 100644
--- a/drivers/net/macsec.c
+++ b/drivers/net/macsec.c
@@ -256,16 +256,32 @@ 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_frame_sci(struct macsec_eth_header *hdr, bool sci_present,
+ struct macsec_rxh_data *rxd)
{
+ struct macsec_dev *macsec_device;
sci_t sci;
-
+ /* SC = 1*/
if (sci_present)
memcpy(&sci, hdr->secure_channel_id,
- sizeof(hdr->secure_channel_id));
- else
+ sizeof(hdr->secure_channel_id));
+ /* SC = 0; ES = 0*/
+ else if (0 == (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;
+
+ for_each_rxsc(secy, rx_sc) {
+ rx_sc = rx_sc ? macsec_rxsc_get(rx_sc) : NULL;
+ if (rx_sc && rx_sc->active) {
+ sci = rx_sc->sci;
+ return sci;
+ }
+ }
+ }
+ } else {
sci = make_sci(hdr->eth.h_source, MACSEC_PORT_ES);
-
+ }
return sci;
}
@@ -1150,11 +1166,11 @@ 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);
--
2.34.1Hi,
kernel test robot noticed the following build warnings:
[auto build test WARNING on net-next/main]
[also build test WARNING on net/main linus/master horms-ipvs/master v6.4-rc6]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/carlos-fernandez-technica-engineering-de/net-macsec-SCI-assignment-for-ES-0/20230615-192230
base: net-next/main
patch link: https://lore.kernel.org/r/20230615111315.6072-1-carlos.fernandez%40technica-engineering.de
patch subject: [PATCH] net: macsec SCI assignment for ES = 0
config: s390-randconfig-r026-20230615 (https://download.01.org/0day-ci/archive/20230616/202306160102.4iSEfuNY-lkp@intel.com/config)
compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project.git 4a5ac14ee968ff0ad5d2cc1ffa0299048db4c88a)
reproduce (this is a W=1 build):
mkdir -p ~/bin
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install s390 cross compiling tool for clang build
# apt-get install binutils-s390x-linux-gnu
git remote add net-next https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git
git fetch net-next main
git checkout net-next/main
b4 shazam https://lore.kernel.org/r/20230615111315.6072-1-carlos.fernandez@technica-engineering.de
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang ~/bin/make.cross W=1 O=build_dir ARCH=s390 olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang ~/bin/make.cross W=1 O=build_dir ARCH=s390 SHELL=/bin/bash
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202306160102.4iSEfuNY-lkp@intel.com/
All warnings (new ones prefixed by >>):
In file included from drivers/net/macsec.c:9:
In file included from include/linux/skbuff.h:28:
In file included from include/linux/dma-mapping.h:10:
In file included from include/linux/scatterlist.h:9:
In file included from arch/s390/include/asm/io.h:75:
include/asm-generic/io.h:547:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
547 | val = __raw_readb(PCI_IOBASE + addr);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:560:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
560 | val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr));
| ~~~~~~~~~~ ^
include/uapi/linux/byteorder/big_endian.h:37:59: note: expanded from macro '__le16_to_cpu'
37 | #define __le16_to_cpu(x) __swab16((__force __u16)(__le16)(x))
| ^
include/uapi/linux/swab.h:102:54: note: expanded from macro '__swab16'
102 | #define __swab16(x) (__u16)__builtin_bswap16((__u16)(x))
| ^
In file included from drivers/net/macsec.c:9:
In file included from include/linux/skbuff.h:28:
In file included from include/linux/dma-mapping.h:10:
In file included from include/linux/scatterlist.h:9:
In file included from arch/s390/include/asm/io.h:75:
include/asm-generic/io.h:573:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
573 | val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
| ~~~~~~~~~~ ^
include/uapi/linux/byteorder/big_endian.h:35:59: note: expanded from macro '__le32_to_cpu'
35 | #define __le32_to_cpu(x) __swab32((__force __u32)(__le32)(x))
| ^
include/uapi/linux/swab.h:115:54: note: expanded from macro '__swab32'
115 | #define __swab32(x) (__u32)__builtin_bswap32((__u32)(x))
| ^
In file included from drivers/net/macsec.c:9:
In file included from include/linux/skbuff.h:28:
In file included from include/linux/dma-mapping.h:10:
In file included from include/linux/scatterlist.h:9:
In file included from arch/s390/include/asm/io.h:75:
include/asm-generic/io.h:584:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
584 | __raw_writeb(value, PCI_IOBASE + addr);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:594:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
594 | __raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:604:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
604 | __raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:692:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
692 | readsb(PCI_IOBASE + addr, buffer, count);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:700:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
700 | readsw(PCI_IOBASE + addr, buffer, count);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:708:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
708 | readsl(PCI_IOBASE + addr, buffer, count);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:717:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
717 | writesb(PCI_IOBASE + addr, buffer, count);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:726:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
726 | writesw(PCI_IOBASE + addr, buffer, count);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:735:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
735 | writesl(PCI_IOBASE + addr, buffer, count);
| ~~~~~~~~~~ ^
>> drivers/net/macsec.c:270:3: warning: variable 'sci' is used uninitialized whenever 'for' loop exits because its condition is false [-Wsometimes-uninitialized]
270 | list_for_each_entry_rcu(macsec_device, &rxd->secys, secys) {
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/rculist.h:392:3: note: expanded from macro 'list_for_each_entry_rcu'
392 | &pos->member != (head); \
| ^~~~~~~~~~~~~~~~~~~~~~
drivers/net/macsec.c:285:9: note: uninitialized use occurs here
285 | return sci;
| ^~~
drivers/net/macsec.c:270:3: note: remove the condition if it is always true
270 | list_for_each_entry_rcu(macsec_device, &rxd->secys, secys) {
| ^
include/linux/rculist.h:392:3: note: expanded from macro 'list_for_each_entry_rcu'
392 | &pos->member != (head); \
| ^
drivers/net/macsec.c:263:11: note: initialize the variable 'sci' to silence this warning
263 | sci_t sci;
| ^
| = 0
13 warnings generated.
vim +270 drivers/net/macsec.c
258
259 static sci_t macsec_frame_sci(struct macsec_eth_header *hdr, bool sci_present,
260 struct macsec_rxh_data *rxd)
261 {
262 struct macsec_dev *macsec_device;
263 sci_t sci;
264 /* SC = 1*/
265 if (sci_present)
266 memcpy(&sci, hdr->secure_channel_id,
267 sizeof(hdr->secure_channel_id));
268 /* SC = 0; ES = 0*/
269 else if (0 == (hdr->tci_an & (MACSEC_TCI_ES | MACSEC_TCI_SC))) {
> 270 list_for_each_entry_rcu(macsec_device, &rxd->secys, secys) {
271 struct macsec_rx_sc *rx_sc;
272 struct macsec_secy *secy = &macsec_device->secy;
273
274 for_each_rxsc(secy, rx_sc) {
275 rx_sc = rx_sc ? macsec_rxsc_get(rx_sc) : NULL;
276 if (rx_sc && rx_sc->active) {
277 sci = rx_sc->sci;
278 return sci;
279 }
280 }
281 }
282 } else {
283 sci = make_sci(hdr->eth.h_source, MACSEC_PORT_ES);
284 }
285 return sci;
286 }
287
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
On Thu, Jun 15, 2023 at 01:13:15PM +0200, carlos.fernandez@technica-engineering.de wrote:
> From: Carlos Fernandez <carlos.fernandez@technica-engineering.de>
>
> According to 802.1AE standard, when ES and SC flags in TCI are zero, used
> SCI should be the current active SC_RX. Current kernel does not implement
> it and uses the header MAC address.
>
> Without this patch, when ES = 0 (using a bridge or switch), header MAC
> will not fit the SCI and MACSec frames will be discarted.
>
> Signed-off-by: Carlos Fernandez <carlos.fernandez@technica-engineering.de>
Hi Carlos,
some feedback from my side.
> ---
> drivers/net/macsec.c | 28 ++++++++++++++++++++++------
> 1 file changed, 22 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/net/macsec.c b/drivers/net/macsec.c
> index 3427993f94f7..ea9b15d555f4 100644
> --- a/drivers/net/macsec.c
> +++ b/drivers/net/macsec.c
> @@ -256,16 +256,32 @@ 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_frame_sci(struct macsec_eth_header *hdr, bool sci_present,
> + struct macsec_rxh_data *rxd)
The indentation of the line above should be such that it aligns with the
inside of the parentheses on the previous line.
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;
> -
> + /* SC = 1*/
> if (sci_present)
> memcpy(&sci, hdr->secure_channel_id,
> - sizeof(hdr->secure_channel_id));
> - else
> + sizeof(hdr->secure_channel_id));
> + /* SC = 0; ES = 0*/
> + else if (0 == (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;
For new networking code, please use reverse xmas tree order - longest line
to shortest - for local variable declarations.
struct macsec_secy *secy = &macsec_device->secy;
struct macsec_rx_sc *rx_sc;
> +
> + for_each_rxsc(secy, rx_sc) {
> + rx_sc = rx_sc ? macsec_rxsc_get(rx_sc) : NULL;
> + if (rx_sc && rx_sc->active) {
> + sci = rx_sc->sci;
> + return sci;
> + }
I wonder if this can be more succinctly written as:
if (rx_sc && rx_sc->active)
return rx_sc->sci;
> + }
> + }
> + } else {
> sci = make_sci(hdr->eth.h_source, MACSEC_PORT_ES);
> -
> + }
> return sci;
> }
clang-16 complains, as I understand things, that if
the else if condition is met above, but sci is not returned from
within it, then sci is uninitialised here.
Perhaps that cannot happen.
But perhaps it would be better guard against it somehow?
drivers/net/macsec.c:270:3: warning: variable 'sci' is used uninitialized whenever 'for' loop exits because its condition is false [-Wsometimes-uninitialized]
list_for_each_entry_rcu(macsec_device, &rxd->secys, secys) {
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
./include/linux/rculist.h:392:3: note: expanded from macro 'list_for_each_entry_rcu'
&pos->member != (head); \
^~~~~~~~~~~~~~~~~~~~~~
drivers/net/macsec.c:285:9: note: uninitialized use occurs here
return sci;
^~~
drivers/net/macsec.c:270:3: note: remove the condition if it is always true
list_for_each_entry_rcu(macsec_device, &rxd->secys, secys) {
^
./include/linux/rculist.h:392:3: note: expanded from macro 'list_for_each_entry_rcu'
&pos->member != (head); \
^
drivers/net/macsec.c:263:11: note: initialize the variable 'sci' to silence this warning
sci_t sci;
^
= 0
...
Hi Simon,
Sure, I'll send the patch again as version 2. Thanks.
________________________________________
From: Simon Horman <simon.horman@corigine.com>
Sent: Thursday, June 15, 2023 4:23 PM
To: Carlos Fernandez
Cc: davem@davemloft.net; edumazet@google.com; kuba@kernel.org; pabeni@redhat.com; netdev@vger.kernel.org; linux-kernel@vger.kernel.org
Subject: Re: [PATCH] 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.
On Thu, Jun 15, 2023 at 01:13:15PM +0200, carlos.fernandez@technica-engineering.de wrote:
> From: Carlos Fernandez <carlos.fernandez@technica-engineering.de>
>
> According to 802.1AE standard, when ES and SC flags in TCI are zero, used
> SCI should be the current active SC_RX. Current kernel does not implement
> it and uses the header MAC address.
>
> Without this patch, when ES = 0 (using a bridge or switch), header MAC
> will not fit the SCI and MACSec frames will be discarted.
>
> Signed-off-by: Carlos Fernandez <carlos.fernandez@technica-engineering.de>
Hi Carlos,
some feedback from my side.
> ---
> drivers/net/macsec.c | 28 ++++++++++++++++++++++------
> 1 file changed, 22 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/net/macsec.c b/drivers/net/macsec.c
> index 3427993f94f7..ea9b15d555f4 100644
> --- a/drivers/net/macsec.c
> +++ b/drivers/net/macsec.c
> @@ -256,16 +256,32 @@ 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_frame_sci(struct macsec_eth_header *hdr, bool sci_present,
> + struct macsec_rxh_data *rxd)
The indentation of the line above should be such that it aligns with the
inside of the parentheses on the previous line.
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;
> -
> + /* SC = 1*/
> if (sci_present)
> memcpy(&sci, hdr->secure_channel_id,
> - sizeof(hdr->secure_channel_id));
> - else
> + sizeof(hdr->secure_channel_id));
> + /* SC = 0; ES = 0*/
> + else if (0 == (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;
For new networking code, please use reverse xmas tree order - longest line
to shortest - for local variable declarations.
struct macsec_secy *secy = &macsec_device->secy;
struct macsec_rx_sc *rx_sc;
> +
> + for_each_rxsc(secy, rx_sc) {
> + rx_sc = rx_sc ? macsec_rxsc_get(rx_sc) : NULL;
> + if (rx_sc && rx_sc->active) {
> + sci = rx_sc->sci;
> + return sci;
> + }
I wonder if this can be more succinctly written as:
if (rx_sc && rx_sc->active)
return rx_sc->sci;
> + }
> + }
> + } else {
> sci = make_sci(hdr->eth.h_source, MACSEC_PORT_ES);
> -
> + }
> return sci;
> }
clang-16 complains, as I understand things, that if
the else if condition is met above, but sci is not returned from
within it, then sci is uninitialised here.
Perhaps that cannot happen.
But perhaps it would be better guard against it somehow?
drivers/net/macsec.c:270:3: warning: variable 'sci' is used uninitialized whenever 'for' loop exits because its condition is false [-Wsometimes-uninitialized]
list_for_each_entry_rcu(macsec_device, &rxd->secys, secys) {
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
./include/linux/rculist.h:392:3: note: expanded from macro 'list_for_each_entry_rcu'
&pos->member != (head); \
^~~~~~~~~~~~~~~~~~~~~~
drivers/net/macsec.c:285:9: note: uninitialized use occurs here
return sci;
^~~
drivers/net/macsec.c:270:3: note: remove the condition if it is always true
list_for_each_entry_rcu(macsec_device, &rxd->secys, secys) {
^
./include/linux/rculist.h:392:3: note: expanded from macro 'list_for_each_entry_rcu'
&pos->member != (head); \
^
drivers/net/macsec.c:263:11: note: initialize the variable 'sci' to silence this warning
sci_t sci;
^
= 0
...
© 2016 - 2026 Red Hat, Inc.