[PATCH net v3 0/2] Fix link_mode derived params functionality

netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed

* [PATCH net v3 0/2] Fix link_mode derived params functionality
@ 2021-04-07 10:06 Danielle Ratson
  2021-04-07 10:06 ` [PATCH net v3 1/2] ethtool: Remove link_mode param and derive link params from driver Danielle Ratson
                   ` (2 more replies)
  0 siblings, 3 replies; 4+ messages in thread
From: Danielle Ratson @ 2021-04-07 10:06 UTC (permalink / raw)
  To: netdev
  Cc: davem, kuba, eric.dumazet, andrew, mkubecek, f.fainelli,
	acardace, irusskikh, gustavo, magnus.karlsson, ecree, idosch,
	jiri, mlxsw, Danielle Ratson

Currently, link_mode parameter derives 3 other link parameters, speed,
lanes and duplex, and the derived information is sent to user space.

Few bugs were found in that functionality.
First, some drivers clear the 'ethtool_link_ksettings' struct in their
get_link_ksettings() callback and cause receiving wrong link mode
information in user space. And also, some drivers can report random
values in the 'link_mode' field and cause general protection fault.

Second, the link parameters are only derived in netlink path so in ioctl
path, we don't any reasonable values.

Third, setting 'speed 10000 lanes 1' fails since the lanes parameter
wasn't set for ETHTOOL_LINK_MODE_10000baseR_FEC_BIT.

Patch #1 solves the first two problems by removing link_mode parameter
and deriving the link parameters in driver instead of ethtool.
Patch #2 solves the third one, by setting the lanes parameter for the
link_mode.

v3:
	* Remove the link_mode parameter in the first patch to solve
	  both two issues from patch#1 and patch#2.
	* Add the second patch to solve the third issue.

v2:
	* Add patch #2.
	* Introduce 'cap_link_mode_supported' instead of adding a
	  validity field to 'ethtool_link_ksettings' struct in patch #1.

Danielle Ratson (2):
  ethtool: Remove link_mode param and derive link params from driver
  ethtool: Add lanes parameter for ETHTOOL_LINK_MODE_10000baseR_FEC_BIT

 .../mellanox/mlxsw/spectrum_ethtool.c         | 19 ++++++++++++++-----
 include/linux/ethtool.h                       |  9 ++++++++-
 net/ethtool/common.c                          | 17 +++++++++++++++++
 net/ethtool/ioctl.c                           | 18 +-----------------
 4 files changed, 40 insertions(+), 23 deletions(-)

-- 
2.26.2

^ permalink raw reply	[flat|nested] 4+ messages in thread

* [PATCH net v3 1/2] ethtool: Remove link_mode param and derive link params from driver
  2021-04-07 10:06 [PATCH net v3 0/2] Fix link_mode derived params functionality Danielle Ratson
@ 2021-04-07 10:06 ` Danielle Ratson
  2021-04-07 10:06 ` [PATCH net v3 2/2] ethtool: Add lanes parameter for ETHTOOL_LINK_MODE_10000baseR_FEC_BIT Danielle Ratson
  2021-04-07 22:10 ` [PATCH net v3 0/2] Fix link_mode derived params functionality patchwork-bot+netdevbpf
  2 siblings, 0 replies; 4+ messages in thread
From: Danielle Ratson @ 2021-04-07 10:06 UTC (permalink / raw)
  To: netdev
  Cc: davem, kuba, eric.dumazet, andrew, mkubecek, f.fainelli,
	acardace, irusskikh, gustavo, magnus.karlsson, ecree, idosch,
	jiri, mlxsw, Danielle Ratson

Some drivers clear the 'ethtool_link_ksettings' struct in their
get_link_ksettings() callback, before populating it with actual values.
Such drivers will set the new 'link_mode' field to zero, resulting in
user space receiving wrong link mode information given that zero is a
valid value for the field.

Another problem is that some drivers (notably tun) can report random
values in the 'link_mode' field. This can result in a general protection
fault when the field is used as an index to the 'link_mode_params' array
[1].

This happens because such drivers implement their set_link_ksettings()
callback by simply overwriting their private copy of
'ethtool_link_ksettings' struct with the one they get from the stack,
which is not always properly initialized.

Fix these problems by removing 'link_mode' from 'ethtool_link_ksettings'
and instead have drivers call ethtool_params_from_link_mode() with the
current link mode. The function will derive the link parameters (e.g.,
speed) from the link mode and fill them in the 'ethtool_link_ksettings'
struct.

v3:
	* Remove link_mode parameter and derive the link parameters in
	  the driver instead of passing link_mode parameter to ethtool
	  and derive it there.

v2:
	* Introduce 'cap_link_mode_supported' instead of adding a
	  validity field to 'ethtool_link_ksettings' struct.

[1]
general protection fault, probably for non-canonical address 0xdffffc00f14cc32c: 0000 [#1] PREEMPT SMP KASAN
KASAN: probably user-memory-access in range [0x000000078a661960-0x000000078a661967]
CPU: 0 PID: 8452 Comm: syz-executor360 Not tainted 5.11.0-syzkaller #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
RIP: 0010:__ethtool_get_link_ksettings+0x1a3/0x3a0 net/ethtool/ioctl.c:446
Code: b7 3e fa 83 fd ff 0f 84 30 01 00 00 e8 16 b0 3e fa 48 8d 3c ed 60 d5 69 8a 48 b8 00 00 00 00 00 fc ff df 48 89 fa 48 c1 ea 03 <0f> b6 14 02 48 89 f8 83 e0 07 83 c0 03
+38 d0 7c 08 84 d2 0f 85 b9
RSP: 0018:ffffc900019df7a0 EFLAGS: 00010202
RAX: dffffc0000000000 RBX: ffff888026136008 RCX: 0000000000000000
RDX: 00000000f14cc32c RSI: ffffffff873439ca RDI: 000000078a661960
RBP: 00000000ffff8880 R08: 00000000ffffffff R09: ffff88802613606f
R10: ffffffff873439bc R11: 0000000000000000 R12: 0000000000000000
R13: ffff88802613606c R14: ffff888011d0c210 R15: ffff888011d0c210
FS:  0000000000749300(0000) GS:ffff8880b9c00000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00000000004b60f0 CR3: 00000000185c2000 CR4: 00000000001506f0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
Call Trace:
 linkinfo_prepare_data+0xfd/0x280 net/ethtool/linkinfo.c:37
 ethnl_default_notify+0x1dc/0x630 net/ethtool/netlink.c:586
 ethtool_notify+0xbd/0x1f0 net/ethtool/netlink.c:656
 ethtool_set_link_ksettings+0x277/0x330 net/ethtool/ioctl.c:620
 dev_ethtool+0x2b35/0x45d0 net/ethtool/ioctl.c:2842
 dev_ioctl+0x463/0xb70 net/core/dev_ioctl.c:440
 sock_do_ioctl+0x148/0x2d0 net/socket.c:1060
 sock_ioctl+0x477/0x6a0 net/socket.c:1177
 vfs_ioctl fs/ioctl.c:48 [inline]
 __do_sys_ioctl fs/ioctl.c:753 [inline]
 __se_sys_ioctl fs/ioctl.c:739 [inline]
 __x64_sys_ioctl+0x193/0x200 fs/ioctl.c:739
 do_syscall_64+0x2d/0x70 arch/x86/entry/common.c:46
 entry_SYSCALL_64_after_hwframe+0x44/0xa9

Fixes: c8907043c6ac9 ("ethtool: Get link mode in use instead of speed and duplex parameters")
Signed-off-by: Danielle Ratson <danieller@nvidia.com>
Reported-by: Eric Dumazet <eric.dumazet@gmail.com>
Reviewed-by: Ido Schimmel <idosch@nvidia.com>
---
 .../mellanox/mlxsw/spectrum_ethtool.c         | 19 ++++++++++++++-----
 include/linux/ethtool.h                       |  9 ++++++++-
 net/ethtool/common.c                          | 16 ++++++++++++++++
 net/ethtool/ioctl.c                           | 18 +-----------------
 4 files changed, 39 insertions(+), 23 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_ethtool.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_ethtool.c
index 0bd64169bf81..078601d31cde 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_ethtool.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_ethtool.c
@@ -1230,16 +1230,22 @@ mlxsw_sp1_from_ptys_link_mode(struct mlxsw_sp *mlxsw_sp, bool carrier_ok,
 			      u32 ptys_eth_proto,
 			      struct ethtool_link_ksettings *cmd)
 {
+	struct mlxsw_sp1_port_link_mode link;
 	int i;
 
-	cmd->link_mode = -1;
+	cmd->base.speed = SPEED_UNKNOWN;
+	cmd->base.duplex = DUPLEX_UNKNOWN;
+	cmd->lanes = 0;
 
 	if (!carrier_ok)
 		return;
 
 	for (i = 0; i < MLXSW_SP1_PORT_LINK_MODE_LEN; i++) {
-		if (ptys_eth_proto & mlxsw_sp1_port_link_mode[i].mask)
-			cmd->link_mode = mlxsw_sp1_port_link_mode[i].mask_ethtool;
+		if (ptys_eth_proto & mlxsw_sp1_port_link_mode[i].mask) {
+			link = mlxsw_sp1_port_link_mode[i];
+			ethtool_params_from_link_mode(cmd,
+						      link.mask_ethtool);
+		}
 	}
 }
 
@@ -1672,7 +1678,9 @@ mlxsw_sp2_from_ptys_link_mode(struct mlxsw_sp *mlxsw_sp, bool carrier_ok,
 	struct mlxsw_sp2_port_link_mode link;
 	int i;
 
-	cmd->link_mode = -1;
+	cmd->base.speed = SPEED_UNKNOWN;
+	cmd->base.duplex = DUPLEX_UNKNOWN;
+	cmd->lanes = 0;
 
 	if (!carrier_ok)
 		return;
@@ -1680,7 +1688,8 @@ mlxsw_sp2_from_ptys_link_mode(struct mlxsw_sp *mlxsw_sp, bool carrier_ok,
 	for (i = 0; i < MLXSW_SP2_PORT_LINK_MODE_LEN; i++) {
 		if (ptys_eth_proto & mlxsw_sp2_port_link_mode[i].mask) {
 			link = mlxsw_sp2_port_link_mode[i];
-			cmd->link_mode = link.mask_ethtool[1];
+			ethtool_params_from_link_mode(cmd,
+						      link.mask_ethtool[1]);
 		}
 	}
 }
diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index ec4cd3921c67..7a106a022506 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -129,7 +129,6 @@ struct ethtool_link_ksettings {
 		__ETHTOOL_DECLARE_LINK_MODE_MASK(lp_advertising);
 	} link_modes;
 	u32	lanes;
-	enum ethtool_link_mode_bit_indices link_mode;
 };
 
 /**
@@ -571,4 +570,12 @@ struct ethtool_phy_ops {
  */
 void ethtool_set_ethtool_phy_ops(const struct ethtool_phy_ops *ops);
 
+/*
+ * ethtool_params_from_link_mode - Derive link parameters from a given link mode
+ * @link_ksettings: Link parameters to be derived from the link mode
+ * @link_mode: Link mode
+ */
+void
+ethtool_params_from_link_mode(struct ethtool_link_ksettings *link_ksettings,
+			      enum ethtool_link_mode_bit_indices link_mode);
 #endif /* _LINUX_ETHTOOL_H */
diff --git a/net/ethtool/common.c b/net/ethtool/common.c
index c6a383dfd6c2..030aa7984a91 100644
--- a/net/ethtool/common.c
+++ b/net/ethtool/common.c
@@ -562,3 +562,19 @@ void ethtool_set_ethtool_phy_ops(const struct ethtool_phy_ops *ops)
 	rtnl_unlock();
 }
 EXPORT_SYMBOL_GPL(ethtool_set_ethtool_phy_ops);
+
+void
+ethtool_params_from_link_mode(struct ethtool_link_ksettings *link_ksettings,
+			      enum ethtool_link_mode_bit_indices link_mode)
+{
+	const struct link_mode_info *link_info;
+
+	if (WARN_ON_ONCE(link_mode >= __ETHTOOL_LINK_MODE_MASK_NBITS))
+		return;
+
+	link_info = &link_mode_params[link_mode];
+	link_ksettings->base.speed = link_info->speed;
+	link_ksettings->lanes = link_info->lanes;
+	link_ksettings->base.duplex = link_info->duplex;
+}
+EXPORT_SYMBOL_GPL(ethtool_params_from_link_mode);
diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c
index 24783b71c584..771688e1b0da 100644
--- a/net/ethtool/ioctl.c
+++ b/net/ethtool/ioctl.c
@@ -426,29 +426,13 @@ struct ethtool_link_usettings {
 int __ethtool_get_link_ksettings(struct net_device *dev,
 				 struct ethtool_link_ksettings *link_ksettings)
 {
-	const struct link_mode_info *link_info;
-	int err;
-
 	ASSERT_RTNL();
 
 	if (!dev->ethtool_ops->get_link_ksettings)
 		return -EOPNOTSUPP;
 
 	memset(link_ksettings, 0, sizeof(*link_ksettings));
-
-	link_ksettings->link_mode = -1;
-	err = dev->ethtool_ops->get_link_ksettings(dev, link_ksettings);
-	if (err)
-		return err;
-
-	if (link_ksettings->link_mode != -1) {
-		link_info = &link_mode_params[link_ksettings->link_mode];
-		link_ksettings->base.speed = link_info->speed;
-		link_ksettings->lanes = link_info->lanes;
-		link_ksettings->base.duplex = link_info->duplex;
-	}
-
-	return 0;
+	return dev->ethtool_ops->get_link_ksettings(dev, link_ksettings);
 }
 EXPORT_SYMBOL(__ethtool_get_link_ksettings);
 
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 4+ messages in thread

* [PATCH net v3 2/2] ethtool: Add lanes parameter for ETHTOOL_LINK_MODE_10000baseR_FEC_BIT
  2021-04-07 10:06 [PATCH net v3 0/2] Fix link_mode derived params functionality Danielle Ratson
  2021-04-07 10:06 ` [PATCH net v3 1/2] ethtool: Remove link_mode param and derive link params from driver Danielle Ratson
@ 2021-04-07 10:06 ` Danielle Ratson
  2021-04-07 22:10 ` [PATCH net v3 0/2] Fix link_mode derived params functionality patchwork-bot+netdevbpf
  2 siblings, 0 replies; 4+ messages in thread
From: Danielle Ratson @ 2021-04-07 10:06 UTC (permalink / raw)
  To: netdev
  Cc: davem, kuba, eric.dumazet, andrew, mkubecek, f.fainelli,
	acardace, irusskikh, gustavo, magnus.karlsson, ecree, idosch,
	jiri, mlxsw, Danielle Ratson

Lanes field is missing for ETHTOOL_LINK_MODE_10000baseR_FEC_BIT
link mode and it causes a failure when trying to set
'speed 10000 lanes 1' on Spectrum-2 machines when autoneg is set to on.

Add the lanes parameter for ETHTOOL_LINK_MODE_10000baseR_FEC_BIT
link mode.

Fixes: c8907043c6ac9 ("ethtool: Get link mode in use instead of speed and duplex parameters")
Signed-off-by: Danielle Ratson <danieller@nvidia.com>
Reviewed-by: Ido Schimmel <idosch@nvidia.com>
---
 net/ethtool/common.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/net/ethtool/common.c b/net/ethtool/common.c
index 030aa7984a91..f9dcbad84788 100644
--- a/net/ethtool/common.c
+++ b/net/ethtool/common.c
@@ -273,6 +273,7 @@ const struct link_mode_info link_mode_params[] = {
 	__DEFINE_LINK_MODE_PARAMS(10000, KR, Full),
 	[ETHTOOL_LINK_MODE_10000baseR_FEC_BIT] = {
 		.speed	= SPEED_10000,
+		.lanes	= 1,
 		.duplex = DUPLEX_FULL,
 	},
 	__DEFINE_LINK_MODE_PARAMS(20000, MLD2, Full),
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [PATCH net v3 0/2] Fix link_mode derived params functionality
  2021-04-07 10:06 [PATCH net v3 0/2] Fix link_mode derived params functionality Danielle Ratson
  2021-04-07 10:06 ` [PATCH net v3 1/2] ethtool: Remove link_mode param and derive link params from driver Danielle Ratson
  2021-04-07 10:06 ` [PATCH net v3 2/2] ethtool: Add lanes parameter for ETHTOOL_LINK_MODE_10000baseR_FEC_BIT Danielle Ratson
@ 2021-04-07 22:10 ` patchwork-bot+netdevbpf
  2 siblings, 0 replies; 4+ messages in thread
From: patchwork-bot+netdevbpf @ 2021-04-07 22:10 UTC (permalink / raw)
  To: Danielle Ratson
  Cc: netdev, davem, kuba, eric.dumazet, andrew, mkubecek, f.fainelli,
	acardace, irusskikh, gustavo, magnus.karlsson, ecree, idosch,
	jiri, mlxsw

Hello:

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

On Wed, 7 Apr 2021 13:06:50 +0300 you wrote:
> Currently, link_mode parameter derives 3 other link parameters, speed,
> lanes and duplex, and the derived information is sent to user space.
> 
> Few bugs were found in that functionality.
> First, some drivers clear the 'ethtool_link_ksettings' struct in their
> get_link_ksettings() callback and cause receiving wrong link mode
> information in user space. And also, some drivers can report random
> values in the 'link_mode' field and cause general protection fault.
> 
> [...]

Here is the summary with links:
  - [net,v3,1/2] ethtool: Remove link_mode param and derive link params from driver
    https://git.kernel.org/netdev/net/c/a975d7d8a356
  - [net,v3,2/2] ethtool: Add lanes parameter for ETHTOOL_LINK_MODE_10000baseR_FEC_BIT
    https://git.kernel.org/netdev/net/c/fde32dbe712b

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



^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2021-04-07 22:10 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-07 10:06 [PATCH net v3 0/2] Fix link_mode derived params functionality Danielle Ratson
2021-04-07 10:06 ` [PATCH net v3 1/2] ethtool: Remove link_mode param and derive link params from driver Danielle Ratson
2021-04-07 10:06 ` [PATCH net v3 2/2] ethtool: Add lanes parameter for ETHTOOL_LINK_MODE_10000baseR_FEC_BIT Danielle Ratson
2021-04-07 22:10 ` [PATCH net v3 0/2] Fix link_mode derived params functionality patchwork-bot+netdevbpf

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).