[PATCH ethtool 0/5] ethtool: Module EEPROM fixes

[PATCH ethtool 0/5] ethtool: Module EEPROM fixes

[Ido Schimmel]

From: Ido Schimmel <idosch@nvidia.com>

This patchset contains fixes for various issues I noticed while working
on the module EEPROM parsing code.

In addition to these fixes, I have the following submissions ready for
the next branch [1]:

* Trivial coding style / documentation changes

* Reworking the EEPROM parsing code to use a memory map with pointers to
  individual pages as the current state is sub-optimal. In the IOCTL path,
  one large buffer is passed to parsers (e.g., SFF-8636, CMIS). In the
  netlink path, individual pages are passed, some of which might not be
  valid. With the proposed changes, the IOCTL and netlink paths only
  differ in the way they initialize the memory map. The parsing code is
  completely shared

* Extending the CMIS parser to request and parse diagnostic information
  (already present in SFF-8636) from optional / banked pages

[1] https://github.com/idosch/ethtool/commits/submit/cmis_dump_v1

Ido Schimmel (5):
  sff-8636: Fix parsing of Page 03h in IOCTL path
  cmis: Fix invalid memory access in IOCTL path
  netlink: eeprom: Fallback to IOCTL when a complete hex/raw dump is
    requested
  ethtool: Fix compilation warning when pretty dump is disabled
  netlink: eeprom: Fix compilation when pretty dump is disabled

 cmis.c                  |  2 +-
 ethtool.c               | 13 ++++++++-----
 netlink/module-eeprom.c | 14 ++++++++++++++
 qsfp.c                  |  2 +-
 4 files changed, 24 insertions(+), 7 deletions(-)

-- 
2.31.1

[PATCH ethtool 1/5] sff-8636: Fix parsing of Page 03h in IOCTL path

[Ido Schimmel]

From: Ido Schimmel <idosch@nvidia.com>

The offset of Page 03h compared to the base address of the Lower Memory
is 512 bytes. However, all the offsets to the page start at address 128,
which is the address that separates Lower and Upper memory (see Figure
6-1 in SFF-8636). Therefore, reading these offsets compared to the start
of Page 03h results in incorrect memory accesses as can be seen in the
output below.

Instead, pass Page 03h with the correct offset.

This is a temporary solution until SFF-8636 is refactored to use a
memory map for parsing.

Before patch:

 # ethtool -m swp13
 ...
 Laser bias current high alarm threshold   : 16.448 mA
 Laser bias current low alarm threshold    : 16.500 mA
 Laser bias current high warning threshold : 16.480 mA
 Laser bias current low warning threshold  : 61.538 mA
 Laser output power high alarm threshold   : 1.2576 mW / 1.00 dBm
 Laser output power low alarm threshold    : 1.0321 mW / 0.14 dBm
 Laser output power high warning threshold : 2.1318 mW / 3.29 dBm
 Laser output power low warning threshold  : 2.0530 mW / 3.12 dBm
 Module temperature high alarm threshold   : 0.00 degrees C / 32.00 degrees F
 Module temperature low alarm threshold    : 0.00 degrees C / 32.00 degrees F
 Module temperature high warning threshold : 0.00 degrees C / 32.00 degrees F
 Module temperature low warning threshold  : 0.00 degrees C / 32.00 degrees F
 Module voltage high alarm threshold       : 0.2377 V
 Module voltage low alarm threshold        : 2.5701 V
 Module voltage high warning threshold     : 2.8276 V
 Module voltage low warning threshold      : 2.6982 V
 Laser rx power high alarm threshold       : 0.8224 mW / -0.85 dBm
 Laser rx power low alarm threshold        : 0.8224 mW / -0.85 dBm
 Laser rx power high warning threshold     : 0.8224 mW / -0.85 dBm
 Laser rx power low warning threshold      : 0.8224 mW / -0.85 dBm

After patch:

 # ethtool -m swp13
 ...
 Laser bias current high alarm threshold   : 8.500 mA
 Laser bias current low alarm threshold    : 5.492 mA
 Laser bias current high warning threshold : 8.000 mA
 Laser bias current low warning threshold  : 6.000 mA
 Laser output power high alarm threshold   : 3.4673 mW / 5.40 dBm
 Laser output power low alarm threshold    : 0.0724 mW / -11.40 dBm
 Laser output power high warning threshold : 1.7378 mW / 2.40 dBm
 Laser output power low warning threshold  : 0.1445 mW / -8.40 dBm
 Module temperature high alarm threshold   : 80.00 degrees C / 176.00 degrees F
 Module temperature low alarm threshold    : -10.00 degrees C / 14.00 degrees F
 Module temperature high warning threshold : 70.00 degrees C / 158.00 degrees F
 Module temperature low warning threshold  : 0.00 degrees C / 32.00 degrees F
 Module voltage high alarm threshold       : 3.5000 V
 Module voltage low alarm threshold        : 3.1000 V
 Module voltage high warning threshold     : 3.4650 V
 Module voltage low warning threshold      : 3.1350 V
 Laser rx power high alarm threshold       : 3.4673 mW / 5.40 dBm
 Laser rx power low alarm threshold        : 0.0467 mW / -13.31 dBm
 Laser rx power high warning threshold     : 1.7378 mW / 2.40 dBm
 Laser rx power low warning threshold      : 0.0933 mW / -10.30 dBm

The following AddressSanitizer report is fixed:

==44670==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x617000000320 at pc 0x00000047ad93 bp 0x7ffcb4dc0070 sp 0x7ffcb4dc0068
READ of size 1 at 0x617000000320 thread T0
    #0 0x47ad92 in sff8636_dom_parse qsfp.c:683
    #1 0x47c5d6 in sff8636_show_dom qsfp.c:771
    #2 0x47d21f in sff8636_show_all qsfp.c:870
    #3 0x42130b in do_getmodule ethtool.c:4908
    #4 0x42a38a in main ethtool.c:6383
    #5 0x7f500bf421e1 in __libc_start_main (/lib64/libc.so.6+0x281e1)
    #6 0x40258d in _start (ethtool+0x40258d)

0x617000000320 is located 16 bytes to the right of 656-byte region [0x617000000080,0x617000000310)
allocated by thread T0 here:
    #0 0x7f500c2d6527 in __interceptor_calloc (/lib64/libasan.so.6+0xab527)
    #1 0x420d8c in do_getmodule ethtool.c:4859
    #2 0x42a38a in main ethtool.c:6383
    #3 0x7f500bf421e1 in __libc_start_main (/lib64/libc.so.6+0x281e1)

SUMMARY: AddressSanitizer: heap-buffer-overflow qsfp.c:683 in sff8636_dom_parse

Fixes: fc47fdb7c364 ("ethtool: Refactor human-readable module EEPROM output for new API")
Signed-off-by: Ido Schimmel <idosch@nvidia.com>
---
 qsfp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/qsfp.c b/qsfp.c
index 644fe148a5aa..e84226bc1554 100644
--- a/qsfp.c
+++ b/qsfp.c
@@ -867,7 +867,7 @@ void sff8636_show_all(const __u8 *id, __u32 eeprom_len)
 		(id[SFF8636_ID_OFFSET] == SFF8024_ID_QSFP_PLUS) ||
 		(id[SFF8636_ID_OFFSET] == SFF8024_ID_QSFP28)) {
 		sff6836_show_page_zero(id);
-		sff8636_show_dom(id, id + SFF8636_PAGE03H_OFFSET, eeprom_len);
+		sff8636_show_dom(id, id + 3 * 0x80, eeprom_len);
 	}
 }
 
-- 
2.31.1

[PATCH ethtool 2/5] cmis: Fix invalid memory access in IOCTL path

[Ido Schimmel]

From: Ido Schimmel <idosch@nvidia.com>

Page 01h is an optional page that is not available for flat memory
modules. Trying to blindly access it results in the following report
from AddressSanitizer [1].

Instead, pass the base address of the Lower Memory. This results in
wrong information being parsed, but this never worked correctly since
CMIS support first appeared in cited commit.

The information will be parsed correctly in a follow-up submission that
reworks the EEPROM parsing code to use a memory map with pointers to
individual pages instead of passing one large buffer.

[1]
==968785==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x6120000001d4 at pc 0x0000004806ee bp 0x7ffefbc977a0 sp 0x7ffefbc97798
READ of size 1 at 0x6120000001d4 thread T0
    #0 0x4806ed in cmis_print_smf_cbl_len cmis.c:127
    #1 0x48113e in cmis_show_link_len_from_page cmis.c:279
    #2 0x4811e3 in cmis_show_link_len cmis.c:300
    #3 0x481358 in qsfp_dd_show_all cmis.c:336
    #4 0x47d190 in sff8636_show_all qsfp.c:861
    #5 0x42130b in do_getmodule ethtool.c:4908
    #6 0x42a38a in main ethtool.c:6383
    #7 0x7f11db6c51e1 in __libc_start_main (/lib64/libc.so.6+0x281e1)
    #8 0x40258d in _start (ethtool+0x40258d)

Address 0x6120000001d4 is a wild pointer.
SUMMARY: AddressSanitizer: heap-buffer-overflow cmis.c:127 in cmis_print_smf_cbl_len

Fixes: 88ca347ef35a ("Add QSFP-DD support").
Signed-off-by: Ido Schimmel <idosch@nvidia.com>
---
 cmis.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/cmis.c b/cmis.c
index 361b721f332f..1a91e798e4b8 100644
--- a/cmis.c
+++ b/cmis.c
@@ -297,7 +297,7 @@ static void cmis_show_link_len_from_page(const __u8 *page_one_data)
  */
 static void cmis_show_link_len(const __u8 *id)
 {
-	cmis_show_link_len_from_page(id + PAG01H_UPPER_OFFSET);
+	cmis_show_link_len_from_page(id);
 }
 
 /**
-- 
2.31.1

[PATCH ethtool 3/5] netlink: eeprom: Fallback to IOCTL when a complete hex/raw dump is requested

[Ido Schimmel]

From: Ido Schimmel <idosch@nvidia.com>

The IOCTL backend provides a complete hex/raw dump of the module EEPROM
contents:

 # ethtool -m swp11 hex on | wc -l
 34

 # ethtool -m swp11 raw on | wc -c
 512

With the netlink backend, only the first 128 bytes from I2C address 0x50
are dumped:

 # ethtool -m swp11 hex on | wc -l
 10

 # ethtool -m swp11 raw on | wc -c
 128

The presence of optional / banked pages is unknown without parsing the
EEPROM contents which is unavailable when pretty printing is disabled
(i.e., configure --disable-pretty-dump). With the IOCTL backend, this
parsing happens inside the kernel.

Therefore, when a complete hex/raw dump is requested, fallback to the
IOCTL backend.

After the patch:

 # ethtool -m swp11 hex on | wc -l
 34

 # ethtool -m swp11 raw on | wc -c
 512

This avoids breaking users that are relying on current behavior.

If users want a hex/raw dump of optional/banked pages that are not
returned with the IOCTL backend, they will be required to request these
explicitly via the netlink backend. For example:

 # ethtool -m swp11 hex on page 0x2

This is desirable as that way there is no ambiguity regarding the
location of optional/banked pages in the dump.

Another way to implement the above would be to use the 'nlchk' callback
added in commit 67a9ef551661 ("ethtool: add nlchk for redirecting to
netlink"). However, it is called before the netlink instance is
initialized and before the command line parameters are parsed via
nl_parser().

Fixes: 25b64c66f58d ("ethtool: Add netlink handler for getmodule (-m)")
Signed-off-by: Ido Schimmel <idosch@nvidia.com>
---
 netlink/module-eeprom.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/netlink/module-eeprom.c b/netlink/module-eeprom.c
index 38e7d2cd6cf3..e9a122df3259 100644
--- a/netlink/module-eeprom.c
+++ b/netlink/module-eeprom.c
@@ -365,6 +365,16 @@ int nl_getmodule(struct cmd_context *ctx)
 		return -EINVAL;
 	}
 
+	/* When complete hex/raw dump of the EEPROM is requested, fallback to
+	 * ioctl. Netlink can only request specific pages.
+	 */
+	if ((getmodule_cmd_params.dump_hex || getmodule_cmd_params.dump_raw) &&
+	    !getmodule_cmd_params.page && !getmodule_cmd_params.bank &&
+	    !getmodule_cmd_params.i2c_address) {
+		nlctx->ioctl_fallback = true;
+		return -EOPNOTSUPP;
+	}
+
 	request.i2c_address = ETH_I2C_ADDRESS_LOW;
 	request.length = 128;
 	ret = page_fetch(nlctx, &request);
-- 
2.31.1

[PATCH ethtool 4/5] ethtool: Fix compilation warning when pretty dump is disabled

[Ido Schimmel]

From: Ido Schimmel <idosch@nvidia.com>

When pretty dump is disabled (i.e., configure --disable-pretty-dump),
gcc 11.2.1 emits the following warning:

ethtool.c: In function ‘dump_regs’:
ethtool.c:1160:31: warning: comparison is always false due to limited range of data type [-Wtype-limits]
 1160 |                 for (i = 0; i < ARRAY_SIZE(driver_list); i++)
      |                               ^

Fix it by avoiding iterating over 'driver_list' when pretty dump is
disabled.

Signed-off-by: Ido Schimmel <idosch@nvidia.com>
---
 ethtool.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/ethtool.c b/ethtool.c
index a6826e9f9e3f..46887c7263e1 100644
--- a/ethtool.c
+++ b/ethtool.c
@@ -1089,12 +1089,12 @@ static int parse_hkey(char **rss_hkey, u32 key_size,
 	return 0;
 }
 
+#ifdef ETHTOOL_ENABLE_PRETTY_DUMP
 static const struct {
 	const char *name;
 	int (*func)(struct ethtool_drvinfo *info, struct ethtool_regs *regs);
 
 } driver_list[] = {
-#ifdef ETHTOOL_ENABLE_PRETTY_DUMP
 	{ "8139cp", realtek_dump_regs },
 	{ "8139too", realtek_dump_regs },
 	{ "r8169", realtek_dump_regs },
@@ -1129,8 +1129,8 @@ static const struct {
 	{ "fec", fec_dump_regs },
 	{ "igc", igc_dump_regs },
 	{ "bnxt_en", bnxt_dump_regs },
-#endif
 };
+#endif
 
 void dump_hex(FILE *file, const u8 *data, int len, int offset)
 {
@@ -1149,14 +1149,15 @@ void dump_hex(FILE *file, const u8 *data, int len, int offset)
 static int dump_regs(int gregs_dump_raw, int gregs_dump_hex,
 		     struct ethtool_drvinfo *info, struct ethtool_regs *regs)
 {
-	unsigned int i;
-
 	if (gregs_dump_raw) {
 		fwrite(regs->data, regs->len, 1, stdout);
 		goto nested;
 	}
 
-	if (!gregs_dump_hex)
+#ifdef ETHTOOL_ENABLE_PRETTY_DUMP
+	if (!gregs_dump_hex) {
+		unsigned int i;
+
 		for (i = 0; i < ARRAY_SIZE(driver_list); i++)
 			if (!strncmp(driver_list[i].name, info->driver,
 				     ETHTOOL_BUSINFO_LEN)) {
@@ -1168,6 +1169,8 @@ static int dump_regs(int gregs_dump_raw, int gregs_dump_hex,
 				 */
 				break;
 			}
+	}
+#endif
 
 	dump_hex(stdout, regs->data, regs->len, 0);
 
-- 
2.31.1

[PATCH ethtool 5/5] netlink: eeprom: Fix compilation when pretty dump is disabled

[Ido Schimmel]

From: Ido Schimmel <idosch@nvidia.com>

When pretty dump is disabled (i.e., configure --disable-pretty-dump),
the following errors are emitted:

/usr/bin/ld: netlink/module-eeprom.o: in function `decoder_print':
netlink/module-eeprom.c:330: undefined reference to `sff8636_show_all_paged'
netlink/module-eeprom.c:334: undefined reference to `cmis_show_all'
netlink/module-eeprom.c:325: undefined reference to `sff8079_show_all'

The else clause is unreachable when pretty dump is disabled, so wrap it
with ifdef directive.

This will be re-worked in future patches where the netlink code only
queries the SFF-8024 Identifier Value and defers page requests to
individual parsers.

Signed-off-by: Ido Schimmel <idosch@nvidia.com>
---
 netlink/module-eeprom.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/netlink/module-eeprom.c b/netlink/module-eeprom.c
index e9a122df3259..48cd2cc55bee 100644
--- a/netlink/module-eeprom.c
+++ b/netlink/module-eeprom.c
@@ -275,6 +275,7 @@ static int page_fetch(struct nl_context *nlctx, const struct ethtool_module_eepr
 	return nlsock_process_reply(nlsock, nomsg_reply_cb, NULL);
 }
 
+#ifdef ETHTOOL_ENABLE_PRETTY_DUMP
 static int decoder_prefetch(struct nl_context *nlctx)
 {
 	struct ethtool_module_eeprom *page_zero_lower = cache_get(0, 0, ETH_I2C_ADDRESS_LOW);
@@ -338,6 +339,7 @@ static void decoder_print(void)
 		break;
 	}
 }
+#endif
 
 int nl_getmodule(struct cmd_context *ctx)
 {
@@ -414,10 +416,12 @@ int nl_getmodule(struct cmd_context *ctx)
 		else
 			dump_hex(stdout, eeprom_data, dump_length, request.offset);
 	} else {
+#ifdef ETHTOOL_ENABLE_PRETTY_DUMP
 		ret = decoder_prefetch(nlctx);
 		if (ret)
 			goto cleanup;
 		decoder_print();
+#endif
 	}
 
 cleanup:
-- 
2.31.1

Re: [PATCH ethtool 0/5] ethtool: Module EEPROM fixes

[patchwork-bot+netdevbpf]

Hello:

This series was applied to ethtool/ethtool.git (refs/heads/master):

On Tue, 14 Sep 2021 14:27:33 +0300 you wrote:
> From: Ido Schimmel <idosch@nvidia.com>
> 
> This patchset contains fixes for various issues I noticed while working
> on the module EEPROM parsing code.
> 
> In addition to these fixes, I have the following submissions ready for
> the next branch [1]:
> 
> [...]

Here is the summary with links:
  - [ethtool,1/5] sff-8636: Fix parsing of Page 03h in IOCTL path
    https://git.kernel.org/pub/scm/network/ethtool/ethtool.git/commit/?id=769a50e66b11
  - [ethtool,2/5] cmis: Fix invalid memory access in IOCTL path
    https://git.kernel.org/pub/scm/network/ethtool/ethtool.git/commit/?id=7e153a7da4f1
  - [ethtool,3/5] netlink: eeprom: Fallback to IOCTL when a complete hex/raw dump is requested
    https://git.kernel.org/pub/scm/network/ethtool/ethtool.git/commit/?id=2ddb1a1f06e2
  - [ethtool,4/5] ethtool: Fix compilation warning when pretty dump is disabled
    https://git.kernel.org/pub/scm/network/ethtool/ethtool.git/commit/?id=d02409cc1788
  - [ethtool,5/5] netlink: eeprom: Fix compilation when pretty dump is disabled
    https://git.kernel.org/pub/scm/network/ethtool/ethtool.git/commit/?id=a7431bc5ab09

You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html