[PATCH ethtool] netlink: settings: fix netlink support when PLCA is not present

[PATCH ethtool] netlink: settings: fix netlink support when PLCA is not present

[Jakub Kicinski]

PLCA support threw the PLCA commands as required into the initial
support check at the start of nl_gset(). That's not correct.
The initial check (AFAIU) queries for the base support in the kernel
i.e. support for the commands which correspond to ioctls.
If those are not available (presumably very old kernel or kernel
without ethtool-netlink) we're better off using the ioctl.

For new functionality, however, falling back to ioctl
is counterproductive. New functionality (like PLCA) isn't
supported via the ioctl, anyway, and we're losing all the other
netlink-only functionality (I noticed that the link down statistics
are gone).

After much deliberation I decided to add a second check for
command support in gset_request(). Seems cleanest and if any
of the non-required commands narrows the capabilities (e.g.
does not support dump) we should just skip it too. Falling
back to ioctl would again be a regression.

Fixes: cf02fc1b1095 ("add support for IEEE 802.3cg-2019 Clause 148")
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 netlink/settings.c | 31 ++++++++++++++++---------------
 1 file changed, 16 insertions(+), 15 deletions(-)

diff --git a/netlink/settings.c b/netlink/settings.c
index 168b182530a2..4fd75d2e0a5a 100644
--- a/netlink/settings.c
+++ b/netlink/settings.c
@@ -963,13 +963,17 @@ int plca_status_reply_cb(const struct nlmsghdr *nlhdr, void *data)
 	return MNL_CB_OK;
 }
 
-static int gset_request(struct nl_context *nlctx, uint8_t msg_type,
+static int gset_request(struct cmd_context *ctx, uint8_t msg_type,
 			uint16_t hdr_attr, mnl_cb_t cb)
 {
+	struct nl_context *nlctx = ctx->nlctx;
 	struct nl_socket *nlsk = nlctx->ethnl_socket;
 	u32 flags;
 	int ret;
 
+	if (netlink_cmd_check(ctx, msg_type, true))
+		return 0;
+
 	flags = get_stats_flag(nlctx, msg_type, hdr_attr);
 
 	ret = nlsock_prep_get_request(nlsk, msg_type, hdr_attr, flags);
@@ -980,61 +984,58 @@ static int gset_request(struct nl_context *nlctx, uint8_t msg_type,
 
 int nl_gset(struct cmd_context *ctx)
 {
-	struct nl_context *nlctx = ctx->nlctx;
 	int ret;
 
+	/* Check for the base set of commands */
 	if (netlink_cmd_check(ctx, ETHTOOL_MSG_LINKMODES_GET, true) ||
 	    netlink_cmd_check(ctx, ETHTOOL_MSG_LINKINFO_GET, true) ||
 	    netlink_cmd_check(ctx, ETHTOOL_MSG_WOL_GET, true) ||
 	    netlink_cmd_check(ctx, ETHTOOL_MSG_DEBUG_GET, true) ||
-	    netlink_cmd_check(ctx, ETHTOOL_MSG_LINKSTATE_GET, true) ||
-	    netlink_cmd_check(ctx, ETHTOOL_MSG_PLCA_GET_CFG, true) ||
-	    netlink_cmd_check(ctx, ETHTOOL_MSG_PLCA_GET_STATUS, true))
+	    netlink_cmd_check(ctx, ETHTOOL_MSG_LINKSTATE_GET, true))
 		return -EOPNOTSUPP;
 
-	nlctx->suppress_nlerr = 1;
+	ctx->nlctx->suppress_nlerr = 1;
 
-	ret = gset_request(nlctx, ETHTOOL_MSG_LINKMODES_GET,
+	ret = gset_request(ctx, ETHTOOL_MSG_LINKMODES_GET,
 			   ETHTOOL_A_LINKMODES_HEADER, linkmodes_reply_cb);
 	if (ret == -ENODEV)
 		return ret;
 
-	ret = gset_request(nlctx, ETHTOOL_MSG_LINKINFO_GET,
+	ret = gset_request(ctx, ETHTOOL_MSG_LINKINFO_GET,
 			   ETHTOOL_A_LINKINFO_HEADER, linkinfo_reply_cb);
 	if (ret == -ENODEV)
 		return ret;
 
-	ret = gset_request(nlctx, ETHTOOL_MSG_WOL_GET, ETHTOOL_A_WOL_HEADER,
+	ret = gset_request(ctx, ETHTOOL_MSG_WOL_GET, ETHTOOL_A_WOL_HEADER,
 			   wol_reply_cb);
 	if (ret == -ENODEV)
 		return ret;
 
-	ret = gset_request(nlctx, ETHTOOL_MSG_PLCA_GET_CFG,
+	ret = gset_request(ctx, ETHTOOL_MSG_PLCA_GET_CFG,
 			   ETHTOOL_A_PLCA_HEADER, plca_cfg_reply_cb);
 	if (ret == -ENODEV)
 		return ret;
 
-	ret = gset_request(nlctx, ETHTOOL_MSG_DEBUG_GET, ETHTOOL_A_DEBUG_HEADER,
+	ret = gset_request(ctx, ETHTOOL_MSG_DEBUG_GET, ETHTOOL_A_DEBUG_HEADER,
 			   debug_reply_cb);
 	if (ret == -ENODEV)
 		return ret;
 
-	ret = gset_request(nlctx, ETHTOOL_MSG_LINKSTATE_GET,
+	ret = gset_request(ctx, ETHTOOL_MSG_LINKSTATE_GET,
 			   ETHTOOL_A_LINKSTATE_HEADER, linkstate_reply_cb);
 	if (ret == -ENODEV)
 		return ret;
 
-	ret = gset_request(nlctx, ETHTOOL_MSG_PLCA_GET_STATUS,
+	ret = gset_request(ctx, ETHTOOL_MSG_PLCA_GET_STATUS,
 			   ETHTOOL_A_PLCA_HEADER, plca_status_reply_cb);
 	if (ret == -ENODEV)
 		return ret;
 
-	if (!nlctx->no_banner) {
+	if (!ctx->nlctx->no_banner) {
 		printf("No data available\n");
 		return 75;
 	}
 
-
 	return 0;
 }
 
-- 
2.40.0

Re: [PATCH ethtool] netlink: settings: fix netlink support when PLCA is not present

[Piergiorgio Beruto]

On Mon, Apr 24, 2023 at 05:07:42PM -0700, Jakub Kicinski wrote:
> PLCA support threw the PLCA commands as required into the initial
> support check at the start of nl_gset(). That's not correct.
> The initial check (AFAIU) queries for the base support in the kernel
> i.e. support for the commands which correspond to ioctls.
> If those are not available (presumably very old kernel or kernel
> without ethtool-netlink) we're better off using the ioctl.
> 
> For new functionality, however, falling back to ioctl
> is counterproductive. New functionality (like PLCA) isn't
> supported via the ioctl, anyway, and we're losing all the other
> netlink-only functionality (I noticed that the link down statistics
> are gone).
> 
> After much deliberation I decided to add a second check for
> command support in gset_request(). Seems cleanest and if any
> of the non-required commands narrows the capabilities (e.g.
> does not support dump) we should just skip it too. Falling
> back to ioctl would again be a regression.
Thanks Jackub, that makes sense to me (FWIW).
I'm trying this patch on my system and I can see it does not create an
issue on systems where PLCA is -not- supported.

However, as soon as I try this on a system where PLCA is enabled, I get
a segmentation fault of ethtool. I'm currently investigating the reason.

Thanks,
Piergiorgio

Re: [PATCH ethtool] netlink: settings: fix netlink support when PLCA is not present

[Piergiorgio Beruto]

On Mon, Apr 24, 2023 at 05:07:42PM -0700, Jakub Kicinski wrote:
> PLCA support threw the PLCA commands as required into the initial
> support check at the start of nl_gset(). That's not correct.
> The initial check (AFAIU) queries for the base support in the kernel
> i.e. support for the commands which correspond to ioctls.
> If those are not available (presumably very old kernel or kernel
> without ethtool-netlink) we're better off using the ioctl.
> 
> For new functionality, however, falling back to ioctl
> is counterproductive. New functionality (like PLCA) isn't
> supported via the ioctl, anyway, and we're losing all the other
> netlink-only functionality (I noticed that the link down statistics
> are gone).
> 
> After much deliberation I decided to add a second check for
> command support in gset_request(). Seems cleanest and if any
> of the non-required commands narrows the capabilities (e.g.
> does not support dump) we should just skip it too. Falling
> back to ioctl would again be a regression.
Hi Jackub,
please ignore my previous reply, the segmentation fault I saw was
actually triggered by a different problem I had on my reference
platform.

I've successfully tested this patch with and without netlink.
Please, add me as reviewer and tester.

Kind Regards,
Piergiorgio

Re: [PATCH ethtool] netlink: settings: fix netlink support when PLCA is not present

[Jakub Kicinski]

On Tue, 25 Apr 2023 16:40:57 +0200 Piergiorgio Beruto wrote:
> please ignore my previous reply, the segmentation fault I saw was
> actually triggered by a different problem I had on my reference
> platform.
> 
> I've successfully tested this patch with and without netlink.
> Please, add me as reviewer and tester.

Good to hear :)  Thank you!

Re: [PATCH ethtool] netlink: settings: fix netlink support when PLCA is not present

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to ethtool/ethtool.git (master)
by Michal Kubecek <mkubecek@suse.cz>:

On Mon, 24 Apr 2023 17:07:42 -0700 you wrote:
> PLCA support threw the PLCA commands as required into the initial
> support check at the start of nl_gset(). That's not correct.
> The initial check (AFAIU) queries for the base support in the kernel
> i.e. support for the commands which correspond to ioctls.
> If those are not available (presumably very old kernel or kernel
> without ethtool-netlink) we're better off using the ioctl.
> 
> [...]

Here is the summary with links:
  - [ethtool] netlink: settings: fix netlink support when PLCA is not present
    https://git.kernel.org/pub/scm/network/ethtool/ethtool.git/commit/?id=3d1f1c1e5070

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