[PATCH ethtool 0/2] netlink: improve compatibility with ioctl interface

[PATCH ethtool 0/2] netlink: improve compatibility with ioctl interface

[Michal Kubecek]

Restore special behavior of "ethtool -s <dev> autoneg on" if no advertised
modes, speed and duplex are requested: ioctl code enables all link modes
supported by the device. This is most important for network devices which
report no advertised modes when autonegotiation is disabled.

First patch cleans up the parser interface; it allows nl_sset() to inspect
the composed message and append an attribute to it if needed.

Ido Schimmel (1):
  ethtool: Improve compatibility between netlink and ioctl interfaces

Michal Kubecek (1):
  netlink: do not send messages and process replies in nl_parser()

 netlink/cable_test.c |   2 +-
 netlink/channels.c   |   2 +-
 netlink/coalesce.c   |   2 +-
 netlink/eee.c        |   2 +-
 netlink/parser.c     |  43 ++++++++++-----
 netlink/parser.h     |   3 +-
 netlink/pause.c      |   2 +-
 netlink/rings.c      |   2 +-
 netlink/settings.c   | 127 +++++++++++++++++++++++++++++++++++++++++--
 9 files changed, 158 insertions(+), 27 deletions(-)

-- 
2.29.2

[PATCH ethtool 1/2] netlink: do not send messages and process replies in nl_parser()

[Michal Kubecek]

When called with group_style = PARSER_GROUP_MSG, nl_parser() not only
parses the command line and composes the messages but also sends them to
kernel and processes the replies. This is inconsistent with other modes and
also impractical as it takes the control over the process from caller where
it belongs.

Modify nl_parser() to pass composed messages back to caller (which is only
nl_sset() at the moment) and let it send requests and process replies. This
will be needed for an upcoming backward compatibility patch which will need
to inspect and possibly modify one of the composed messages.

Signed-off-by: Michal Kubecek <mkubecek@suse.cz>
---
 netlink/cable_test.c |  2 +-
 netlink/channels.c   |  2 +-
 netlink/coalesce.c   |  2 +-
 netlink/eee.c        |  2 +-
 netlink/parser.c     | 43 ++++++++++++++++++++++++++++---------------
 netlink/parser.h     |  3 ++-
 netlink/pause.c      |  2 +-
 netlink/rings.c      |  2 +-
 netlink/settings.c   | 35 ++++++++++++++++++++++++++++++-----
 9 files changed, 66 insertions(+), 27 deletions(-)

diff --git a/netlink/cable_test.c b/netlink/cable_test.c
index 8a7145324610..17139f7d297d 100644
--- a/netlink/cable_test.c
+++ b/netlink/cable_test.c
@@ -574,7 +574,7 @@ int nl_cable_test_tdr(struct cmd_context *ctx)
 			       ctx->devname, 0))
 		return -EMSGSIZE;
 
-	ret = nl_parser(nlctx, tdr_params, NULL, PARSER_GROUP_NEST);
+	ret = nl_parser(nlctx, tdr_params, NULL, PARSER_GROUP_NEST, NULL);
 	if (ret < 0)
 		return ret;
 
diff --git a/netlink/channels.c b/netlink/channels.c
index c6002ceeb121..894c74bcc11a 100644
--- a/netlink/channels.c
+++ b/netlink/channels.c
@@ -126,7 +126,7 @@ int nl_schannels(struct cmd_context *ctx)
 			       ctx->devname, 0))
 		return -EMSGSIZE;
 
-	ret = nl_parser(nlctx, schannels_params, NULL, PARSER_GROUP_NONE);
+	ret = nl_parser(nlctx, schannels_params, NULL, PARSER_GROUP_NONE, NULL);
 	if (ret < 0)
 		return 1;
 
diff --git a/netlink/coalesce.c b/netlink/coalesce.c
index 07a92d04b7a1..75922a91c2e7 100644
--- a/netlink/coalesce.c
+++ b/netlink/coalesce.c
@@ -254,7 +254,7 @@ int nl_scoalesce(struct cmd_context *ctx)
 			       ctx->devname, 0))
 		return -EMSGSIZE;
 
-	ret = nl_parser(nlctx, scoalesce_params, NULL, PARSER_GROUP_NONE);
+	ret = nl_parser(nlctx, scoalesce_params, NULL, PARSER_GROUP_NONE, NULL);
 	if (ret < 0)
 		return 1;
 
diff --git a/netlink/eee.c b/netlink/eee.c
index d3135b2094a4..04d8f0bbe3fc 100644
--- a/netlink/eee.c
+++ b/netlink/eee.c
@@ -174,7 +174,7 @@ int nl_seee(struct cmd_context *ctx)
 			       ctx->devname, 0))
 		return -EMSGSIZE;
 
-	ret = nl_parser(nlctx, seee_params, NULL, PARSER_GROUP_NONE);
+	ret = nl_parser(nlctx, seee_params, NULL, PARSER_GROUP_NONE, NULL);
 	if (ret < 0)
 		return 1;
 
diff --git a/netlink/parser.c b/netlink/parser.c
index 3b25f5d5a88e..c2eae93efb69 100644
--- a/netlink/parser.c
+++ b/netlink/parser.c
@@ -920,7 +920,7 @@ static void __parser_set(uint64_t *map, unsigned int idx)
 }
 
 struct tmp_buff {
-	struct nl_msg_buff	msgbuff;
+	struct nl_msg_buff	*msgbuff;
 	unsigned int		id;
 	unsigned int		orig_len;
 	struct tmp_buff		*next;
@@ -951,7 +951,12 @@ static struct tmp_buff *tmp_buff_find_or_create(struct tmp_buff **phead,
 	if (!new_buff)
 		return NULL;
 	new_buff->id = id;
-	msgbuff_init(&new_buff->msgbuff);
+	new_buff->msgbuff = malloc(sizeof(*new_buff->msgbuff));
+	if (!new_buff->msgbuff) {
+		free(new_buff);
+		return NULL;
+	}
+	msgbuff_init(new_buff->msgbuff);
 	new_buff->next = NULL;
 	*pbuff = new_buff;
 
@@ -965,7 +970,10 @@ static void tmp_buff_destroy(struct tmp_buff *head)
 
 	while (buff) {
 		next = buff->next;
-		msgbuff_done(&buff->msgbuff);
+		if (buff->msgbuff) {
+			msgbuff_done(buff->msgbuff);
+			free(buff->msgbuff);
+		}
 		free(buff);
 		buff = next;
 	}
@@ -980,13 +988,22 @@ static void tmp_buff_destroy(struct tmp_buff *head)
  *               param_parser::offset)
  * @group_style: defines if identifiers in .group represent separate messages,
  *               nested attributes or are not allowed
+ * @msgbuffs:    (only used for @group_style = PARSER_GROUP_MSG) array to store
+ *               pointers to composed messages; caller must make sure this
+ *               array is sufficient, i.e. that it has at least as many entries
+ *               as the number of different .group values in params array;
+ *               entries are filled from the start, remaining entries are not
+ *               modified; caller should zero initialize the array before
+ *               calling nl_parser()
  */
 int nl_parser(struct nl_context *nlctx, const struct param_parser *params,
-	      void *dest, enum parser_group_style group_style)
+	      void *dest, enum parser_group_style group_style,
+	      struct nl_msg_buff **msgbuffs)
 {
 	struct nl_socket *nlsk = nlctx->ethnl_socket;
 	const struct param_parser *parser;
 	struct tmp_buff *buffs = NULL;
+	unsigned int n_msgbuffs = 0;
 	struct tmp_buff *buff;
 	unsigned int n_params;
 	uint64_t *params_seen;
@@ -1004,7 +1021,7 @@ int nl_parser(struct nl_context *nlctx, const struct param_parser *params,
 		buff = tmp_buff_find_or_create(&buffs, parser->group);
 		if (!buff)
 			goto out_free_buffs;
-		msgbuff = &buff->msgbuff;
+		msgbuff = buff->msgbuff;
 		ret = msg_init(nlctx, msgbuff, parser->group,
 			       NLM_F_REQUEST | NLM_F_ACK);
 		if (ret < 0)
@@ -1013,7 +1030,7 @@ int nl_parser(struct nl_context *nlctx, const struct param_parser *params,
 		switch (group_style) {
 		case PARSER_GROUP_NEST:
 			ret = -EMSGSIZE;
-			nest = ethnla_nest_start(&buff->msgbuff, parser->group);
+			nest = ethnla_nest_start(buff->msgbuff, parser->group);
 			if (!nest)
 				goto out_free_buffs;
 			break;
@@ -1062,7 +1079,7 @@ int nl_parser(struct nl_context *nlctx, const struct param_parser *params,
 		buff = NULL;
 		if (parser->group)
 			buff = tmp_buff_find(buffs, parser->group);
-		msgbuff = buff ? &buff->msgbuff : &nlsk->msgbuff;
+		msgbuff = buff ? buff->msgbuff : &nlsk->msgbuff;
 
 		param_dest = dest ? ((char *)dest + parser->dest_offset) : NULL;
 		ret = parser->handler(nlctx, parser->type, parser->handler_data,
@@ -1074,12 +1091,12 @@ int nl_parser(struct nl_context *nlctx, const struct param_parser *params,
 	if (group_style == PARSER_GROUP_MSG) {
 		ret = -EOPNOTSUPP;
 		for (buff = buffs; buff; buff = buff->next)
-			if (msgbuff_len(&buff->msgbuff) > buff->orig_len &&
+			if (msgbuff_len(buff->msgbuff) > buff->orig_len &&
 			    netlink_cmd_check(nlctx->ctx, buff->id, false))
 				goto out_free;
 	}
 	for (buff = buffs; buff; buff = buff->next) {
-		struct nl_msg_buff *msgbuff = &buff->msgbuff;
+		struct nl_msg_buff *msgbuff = buff->msgbuff;
 
 		if (group_style == PARSER_GROUP_NONE ||
 		    msgbuff_len(msgbuff) == buff->orig_len)
@@ -1092,12 +1109,8 @@ int nl_parser(struct nl_context *nlctx, const struct param_parser *params,
 				goto out_free;
 			break;
 		case PARSER_GROUP_MSG:
-			ret = nlsock_sendmsg(nlsk, msgbuff);
-			if (ret < 0)
-				goto out_free;
-			ret = nlsock_process_reply(nlsk, nomsg_reply_cb, NULL);
-			if (ret < 0)
-				goto out_free;
+			msgbuffs[n_msgbuffs++] = msgbuff;
+			buff->msgbuff = NULL;
 			break;
 		default:
 			break;
diff --git a/netlink/parser.h b/netlink/parser.h
index fd55bc768d42..28f26ccc2a1c 100644
--- a/netlink/parser.h
+++ b/netlink/parser.h
@@ -143,6 +143,7 @@ int nl_parse_char_bitset(struct nl_context *nlctx, uint16_t type,
 
 /* main entry point called to parse the command line */
 int nl_parser(struct nl_context *nlctx, const struct param_parser *params,
-	      void *dest, enum parser_group_style group_style);
+	      void *dest, enum parser_group_style group_style,
+	      struct nl_msg_buff **msgbuffs);
 
 #endif /* ETHTOOL_NETLINK_PARSER_H__ */
diff --git a/netlink/pause.c b/netlink/pause.c
index 9bc9a301821f..867d0da71f72 100644
--- a/netlink/pause.c
+++ b/netlink/pause.c
@@ -293,7 +293,7 @@ int nl_spause(struct cmd_context *ctx)
 			       ctx->devname, 0))
 		return -EMSGSIZE;
 
-	ret = nl_parser(nlctx, spause_params, NULL, PARSER_GROUP_NONE);
+	ret = nl_parser(nlctx, spause_params, NULL, PARSER_GROUP_NONE, NULL);
 	if (ret < 0)
 		return 1;
 
diff --git a/netlink/rings.c b/netlink/rings.c
index 4061520212d5..b8c458fce25f 100644
--- a/netlink/rings.c
+++ b/netlink/rings.c
@@ -126,7 +126,7 @@ int nl_sring(struct cmd_context *ctx)
 			       ctx->devname, 0))
 		return -EMSGSIZE;
 
-	ret = nl_parser(nlctx, sring_params, NULL, PARSER_GROUP_NONE);
+	ret = nl_parser(nlctx, sring_params, NULL, PARSER_GROUP_NONE, NULL);
 	if (ret < 0)
 		return 1;
 
diff --git a/netlink/settings.c b/netlink/settings.c
index 41a2e5af1945..dc9280c114b5 100644
--- a/netlink/settings.c
+++ b/netlink/settings.c
@@ -1110,9 +1110,16 @@ static const struct param_parser sset_params[] = {
 	{}
 };
 
+/* Maximum number of request messages sent to kernel; must be equal to the
+ * number of different .group values in sset_params[] array.
+ */
+#define SSET_MAX_MSGS 4
+
 int nl_sset(struct cmd_context *ctx)
 {
+	struct nl_msg_buff *msgbuffs[SSET_MAX_MSGS] = {};
 	struct nl_context *nlctx = ctx->nlctx;
+	unsigned int i;
 	int ret;
 
 	nlctx->cmd = "-s";
@@ -1120,11 +1127,29 @@ int nl_sset(struct cmd_context *ctx)
 	nlctx->argc = ctx->argc;
 	nlctx->devname = ctx->devname;
 
-	ret = nl_parser(nlctx, sset_params, NULL, PARSER_GROUP_MSG);
-	if (ret < 0)
-		return 1;
+	ret = nl_parser(nlctx, sset_params, NULL, PARSER_GROUP_MSG, msgbuffs);
+	if (ret < 0) {
+		ret = 1;
+		goto out_free;
+	}
+
+	for (i = 0; i < SSET_MAX_MSGS && msgbuffs[i]; i++) {
+		struct nl_socket *nlsk = nlctx->ethnl_socket;
 
-	if (ret == 0)
-		return 0;
+		ret = nlsock_sendmsg(nlsk, msgbuffs[i]);
+		if (ret < 0)
+			goto out_free;
+		ret = nlsock_process_reply(nlsk, nomsg_reply_cb, NULL);
+		if (ret < 0)
+			goto out_free;
+	}
+
+out_free:
+	for (i = 0; i < SSET_MAX_MSGS && msgbuffs[i]; i++) {
+		msgbuff_done(msgbuffs[i]);
+		free(msgbuffs[i]);
+	}
+	if (ret >= 0)
+		return ret;
 	return nlctx->exit_code ?: 75;
 }
-- 
2.29.2

[PATCH ethtool 2/2] ethtool: Improve compatibility between netlink and ioctl interfaces

[Michal Kubecek]

From: Ido Schimmel <idosch@nvidia.com>

With the ioctl interface, when autoneg is enabled, but without
specifying speed, duplex or link modes, the advertised link modes are
set to the supported link modes by the ethtool user space utility.

This does not happen when using the netlink interface. Fix this
incompatibility problem by having ethtool query the supported link modes
from the kernel and advertise all the "real" ones when only "autoneg on"
is specified.

Before:

Settings for eth0:
	Supported ports: [ TP ]
	Supported link modes:   10baseT/Half 10baseT/Full
	                        100baseT/Half 100baseT/Full
	                        1000baseT/Full
	Supported pause frame use: No
	Supports auto-negotiation: Yes
	Supported FEC modes: Not reported
	Advertised link modes:  100baseT/Half 100baseT/Full
	Advertised pause frame use: No
	Advertised auto-negotiation: Yes
	Advertised FEC modes: Not reported
	Speed: 1000Mb/s
	Duplex: Full
	Auto-negotiation: on
	Port: Twisted Pair
	PHYAD: 0
	Transceiver: internal
	MDI-X: off (auto)
	Supports Wake-on: umbg
	Wake-on: d
        Current message level: 0x00000007 (7)
                               drv probe link
	Link detected: yes

After:

Settings for eth0:
	Supported ports: [ TP ]
	Supported link modes:   10baseT/Half 10baseT/Full
	                        100baseT/Half 100baseT/Full
	                        1000baseT/Full
	Supported pause frame use: No
	Supports auto-negotiation: Yes
	Supported FEC modes: Not reported
	Advertised link modes:  10baseT/Half 10baseT/Full
	                        100baseT/Half 100baseT/Full
	                        1000baseT/Full
	Advertised pause frame use: No
	Advertised auto-negotiation: Yes
	Advertised FEC modes: Not reported
	Speed: 1000Mb/s
	Duplex: Full
	Auto-negotiation: on
	Port: Twisted Pair
	PHYAD: 0
	Transceiver: internal
	MDI-X: on (auto)
	Supports Wake-on: umbg
	Wake-on: d
        Current message level: 0x00000007 (7)
                               drv probe link
	Link detected: yes

Signed-off-by: Ido Schimmel <idosch@nvidia.com>
Signed-off-by: Michal Kubecek <mkubecek@suse.cz>
---
 netlink/settings.c | 92 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 92 insertions(+)

diff --git a/netlink/settings.c b/netlink/settings.c
index dc9280c114b5..90c28b1bc424 100644
--- a/netlink/settings.c
+++ b/netlink/settings.c
@@ -1115,6 +1115,93 @@ static const struct param_parser sset_params[] = {
  */
 #define SSET_MAX_MSGS 4
 
+static int linkmodes_reply_advert_all_cb(const struct nlmsghdr *nlhdr,
+					 void *data)
+{
+	const struct nlattr *tb[ETHTOOL_A_LINKMODES_MAX + 1] = {};
+	DECLARE_ATTR_TB_INFO(tb);
+	struct nl_msg_buff *req_msgbuff = data;
+	const struct nlattr *ours_attr;
+	struct nlattr *req_bitset;
+	uint32_t *supported_modes;
+	unsigned int modes_count;
+	unsigned int i;
+	int ret;
+
+	ret = mnl_attr_parse(nlhdr, GENL_HDRLEN, attr_cb, &tb_info);
+	if (ret < 0)
+		return MNL_CB_ERROR;
+	ours_attr = tb[ETHTOOL_A_LINKMODES_OURS];
+	if (!ours_attr)
+		return MNL_CB_ERROR;
+	modes_count = bitset_get_count(tb[ETHTOOL_A_LINKMODES_OURS], &ret);
+	if (ret < 0)
+		return MNL_CB_ERROR;
+	supported_modes = get_compact_bitset_mask(tb[ETHTOOL_A_LINKMODES_OURS]);
+	if (!supported_modes)
+		return MNL_CB_ERROR;
+
+	/* keep only "real" link modes */
+	for (i = 0; i < modes_count; i++)
+		if (!lm_class_match(i, LM_CLASS_REAL))
+			supported_modes[i / 32] &= ~((uint32_t)1 << (i % 32));
+
+	req_bitset = ethnla_nest_start(req_msgbuff, ETHTOOL_A_LINKMODES_OURS);
+	if (!req_bitset)
+		return MNL_CB_ERROR;
+
+	if (ethnla_put_u32(req_msgbuff, ETHTOOL_A_BITSET_SIZE, modes_count) ||
+	    ethnla_put(req_msgbuff, ETHTOOL_A_BITSET_VALUE,
+		       DIV_ROUND_UP(modes_count, 32) * sizeof(uint32_t),
+		       supported_modes) ||
+	    ethnla_put(req_msgbuff, ETHTOOL_A_BITSET_MASK,
+		       DIV_ROUND_UP(modes_count, 32) * sizeof(uint32_t),
+		       supported_modes)) {
+		ethnla_nest_cancel(req_msgbuff, req_bitset);
+		return MNL_CB_ERROR;
+	}
+
+	ethnla_nest_end(req_msgbuff, req_bitset);
+	return MNL_CB_OK;
+}
+
+/* For compatibility reasons with ioctl-based ethtool, when "autoneg on" is
+ * specified without "advertise", "speed" and "duplex", we need to query the
+ * supported link modes from the kernel and advertise all the "real" ones.
+ */
+static int nl_sset_compat_linkmodes(struct nl_context *nlctx,
+				    struct nl_msg_buff *msgbuff)
+{
+	const struct nlattr *tb[ETHTOOL_A_LINKMODES_MAX + 1] = {};
+	DECLARE_ATTR_TB_INFO(tb);
+	struct nl_socket *nlsk = nlctx->ethnl_socket;
+	int ret;
+
+	ret = mnl_attr_parse(msgbuff->nlhdr, GENL_HDRLEN, attr_cb, &tb_info);
+	if (ret < 0)
+		return ret;
+	if (!tb[ETHTOOL_A_LINKMODES_AUTONEG] || tb[ETHTOOL_A_LINKMODES_OURS] ||
+	    tb[ETHTOOL_A_LINKMODES_SPEED] || tb[ETHTOOL_A_LINKMODES_DUPLEX])
+		return 0;
+	if (!mnl_attr_get_u8(tb[ETHTOOL_A_LINKMODES_AUTONEG]))
+		return 0;
+
+	/* all conditions satisfied, create ETHTOOL_A_LINKMODES_OURS */
+	if (netlink_cmd_check(nlctx->ctx, ETHTOOL_MSG_LINKMODES_GET, false) ||
+	    netlink_cmd_check(nlctx->ctx, ETHTOOL_MSG_LINKMODES_SET, false))
+		return -EOPNOTSUPP;
+	ret = nlsock_prep_get_request(nlsk, ETHTOOL_MSG_LINKMODES_GET,
+				      ETHTOOL_A_LINKMODES_HEADER,
+				      ETHTOOL_FLAG_COMPACT_BITSETS);
+	if (ret < 0)
+		return ret;
+	ret = nlsock_sendmsg(nlsk, NULL);
+	if (ret < 0)
+		return ret;
+	return nlsock_process_reply(nlsk, linkmodes_reply_advert_all_cb,
+				    msgbuff);
+}
+
 int nl_sset(struct cmd_context *ctx)
 {
 	struct nl_msg_buff *msgbuffs[SSET_MAX_MSGS] = {};
@@ -1136,6 +1223,11 @@ int nl_sset(struct cmd_context *ctx)
 	for (i = 0; i < SSET_MAX_MSGS && msgbuffs[i]; i++) {
 		struct nl_socket *nlsk = nlctx->ethnl_socket;
 
+		if (msgbuffs[i]->genlhdr->cmd == ETHTOOL_MSG_LINKMODES_SET) {
+			ret = nl_sset_compat_linkmodes(nlctx, msgbuffs[i]);
+			if (ret < 0)
+				goto out_free;
+		}
 		ret = nlsock_sendmsg(nlsk, msgbuffs[i]);
 		if (ret < 0)
 			goto out_free;
-- 
2.29.2