From 5505c528b985be7aea656082c1cc8455c31fd5bf Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Tue, 2 Sep 2025 10:21:51 +0900 Subject: [PATCH 1/4] ethtool-util: drop use of deprecated ETHTOOL_GSET and ETHTOOL_SSET The methods are deprecated since kernel v4.20, https://github.com/torvalds/linux/commit/9b3004953503462a4fab31b85e44ae446d48f0bd and they are trivial wrapper of ETHTOOL_GLINKSETTINGS and ETHTOOL_SLINKSETTINGS, respectively. Hence, the fallback logic is nowadays completely meaningless. Let's drop them. --- src/shared/ethtool-util.c | 82 ++------------------------------------- 1 file changed, 3 insertions(+), 79 deletions(-) diff --git a/src/shared/ethtool-util.c b/src/shared/ethtool-util.c index d961a81eca..d4b4566b62 100644 --- a/src/shared/ethtool-util.c +++ b/src/shared/ethtool-util.c @@ -704,45 +704,6 @@ static int get_glinksettings(int fd, struct ifreq *ifr, union ethtool_link_usett return 0; } -static int get_gset(int fd, struct ifreq *ifr, union ethtool_link_usettings **ret) { - struct ethtool_cmd ecmd = { - .cmd = ETHTOOL_GSET, - }; - - assert(fd >= 0); - assert(ifr); - assert(ret); - - ifr->ifr_data = (void *) &ecmd; - - if (ioctl(fd, SIOCETHTOOL, ifr) < 0) - return -errno; - - union ethtool_link_usettings *u = new(union ethtool_link_usettings, 1); - if (!u) - return -ENOMEM; - - *u = (union ethtool_link_usettings) { - .base.cmd = ETHTOOL_GSET, - .base.link_mode_masks_nwords = 1, - .base.speed = ethtool_cmd_speed(&ecmd), - .base.duplex = ecmd.duplex, - .base.port = ecmd.port, - .base.phy_address = ecmd.phy_address, - .base.autoneg = ecmd.autoneg, - .base.mdio_support = ecmd.mdio_support, - .base.eth_tp_mdix = ecmd.eth_tp_mdix, - .base.eth_tp_mdix_ctrl = ecmd.eth_tp_mdix_ctrl, - }; - - u->link_modes.supported[0] = ecmd.supported; - u->link_modes.advertising[0] = ecmd.advertising; - u->link_modes.lp_advertising[0] = ecmd.lp_advertising; - - *ret = u; - return 0; -} - static int set_slinksettings(int fd, struct ifreq *ifr, const union ethtool_link_usettings *u) { assert(fd >= 0); assert(ifr); @@ -764,37 +725,6 @@ static int set_slinksettings(int fd, struct ifreq *ifr, const union ethtool_link return RET_NERRNO(ioctl(fd, SIOCETHTOOL, ifr)); } -static int set_sset(int fd, struct ifreq *ifr, const union ethtool_link_usettings *u) { - struct ethtool_cmd ecmd = { - .cmd = ETHTOOL_SSET, - }; - - assert(fd >= 0); - assert(ifr); - assert(u); - - if (u->base.cmd != ETHTOOL_GSET || u->base.link_mode_masks_nwords <= 0) - return -EINVAL; - - ecmd.supported = u->link_modes.supported[0]; - ecmd.advertising = u->link_modes.advertising[0]; - ecmd.lp_advertising = u->link_modes.lp_advertising[0]; - - ethtool_cmd_speed_set(&ecmd, u->base.speed); - - ecmd.duplex = u->base.duplex; - ecmd.port = u->base.port; - ecmd.phy_address = u->base.phy_address; - ecmd.autoneg = u->base.autoneg; - ecmd.mdio_support = u->base.mdio_support; - ecmd.eth_tp_mdix = u->base.eth_tp_mdix; - ecmd.eth_tp_mdix_ctrl = u->base.eth_tp_mdix_ctrl; - - ifr->ifr_data = (void *) &ecmd; - - return RET_NERRNO(ioctl(fd, SIOCETHTOOL, ifr)); -} - int ethtool_set_glinksettings( int *fd, const char *ifname, @@ -843,11 +773,8 @@ int ethtool_set_glinksettings( strscpy(ifr.ifr_name, sizeof(ifr.ifr_name), ifname); r = get_glinksettings(*fd, &ifr, &u); - if (r < 0) { - r = get_gset(*fd, &ifr, &u); - if (r < 0) - return log_debug_errno(r, "ethtool: Cannot get device settings for %s: %m", ifname); - } + if (r < 0) + return log_debug_errno(r, "ethtool: Cannot get device settings for %s: %m", ifname); if (speed > 0) UPDATE(u->base.speed, DIV_ROUND_UP(speed, 1000000), changed); @@ -883,10 +810,7 @@ int ethtool_set_glinksettings( if (!changed) return 0; - if (u->base.cmd == ETHTOOL_GLINKSETTINGS) - r = set_slinksettings(*fd, &ifr, u); - else - r = set_sset(*fd, &ifr, u); + r = set_slinksettings(*fd, &ifr, u); if (r < 0) return log_debug_errno(r, "ethtool: Cannot set device settings for %s: %m", ifname); From 8a4929c4e4048734472f13527c54a7a0ce8e4b17 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Tue, 2 Sep 2025 10:28:11 +0900 Subject: [PATCH 2/4] ethtool-util: rename functions and update log messages --- src/shared/ethtool-util.c | 14 +++++++------- src/shared/ethtool-util.h | 2 +- src/udev/net/link-config.c | 2 +- 3 files changed, 9 insertions(+), 9 deletions(-) diff --git a/src/shared/ethtool-util.c b/src/shared/ethtool-util.c index d4b4566b62..8b437c5a83 100644 --- a/src/shared/ethtool-util.c +++ b/src/shared/ethtool-util.c @@ -652,7 +652,7 @@ int ethtool_set_features(int *ethtool_fd, const char *ifname, const int features return 0; } -static int get_glinksettings(int fd, struct ifreq *ifr, union ethtool_link_usettings **ret) { +static int get_link_settings(int fd, struct ifreq *ifr, union ethtool_link_usettings **ret) { union ethtool_link_usettings ecmd = { .base.cmd = ETHTOOL_GLINKSETTINGS, }; @@ -704,7 +704,7 @@ static int get_glinksettings(int fd, struct ifreq *ifr, union ethtool_link_usett return 0; } -static int set_slinksettings(int fd, struct ifreq *ifr, const union ethtool_link_usettings *u) { +static int set_link_settings(int fd, struct ifreq *ifr, const union ethtool_link_usettings *u) { assert(fd >= 0); assert(ifr); assert(u); @@ -725,7 +725,7 @@ static int set_slinksettings(int fd, struct ifreq *ifr, const union ethtool_link return RET_NERRNO(ioctl(fd, SIOCETHTOOL, ifr)); } -int ethtool_set_glinksettings( +int ethtool_set_link_settings( int *fd, const char *ifname, int autonegotiation, @@ -772,9 +772,9 @@ int ethtool_set_glinksettings( strscpy(ifr.ifr_name, sizeof(ifr.ifr_name), ifname); - r = get_glinksettings(*fd, &ifr, &u); + r = get_link_settings(*fd, &ifr, &u); if (r < 0) - return log_debug_errno(r, "ethtool: Cannot get device settings for %s: %m", ifname); + return log_debug_errno(r, "ethtool: Cannot get link settings for %s: %m", ifname); if (speed > 0) UPDATE(u->base.speed, DIV_ROUND_UP(speed, 1000000), changed); @@ -810,9 +810,9 @@ int ethtool_set_glinksettings( if (!changed) return 0; - r = set_slinksettings(*fd, &ifr, u); + r = set_link_settings(*fd, &ifr, u); if (r < 0) - return log_debug_errno(r, "ethtool: Cannot set device settings for %s: %m", ifname); + return log_debug_errno(r, "ethtool: Cannot set link settings for %s: %m", ifname); return r; } diff --git a/src/shared/ethtool-util.h b/src/shared/ethtool-util.h index eea27fb381..a17c33fe47 100644 --- a/src/shared/ethtool-util.h +++ b/src/shared/ethtool-util.h @@ -167,7 +167,7 @@ int ethtool_get_permanent_hw_addr(int *ethtool_fd, const char *ifname, struct hw int ethtool_set_wol(int *ethtool_fd, const char *ifname, uint32_t wolopts, const uint8_t password[SOPASS_MAX]); int ethtool_set_nic_buffer_size(int *ethtool_fd, const char *ifname, const netdev_ring_param *ring); int ethtool_set_features(int *ethtool_fd, const char *ifname, const int features[static _NET_DEV_FEAT_MAX]); -int ethtool_set_glinksettings( +int ethtool_set_link_settings( int *fd, const char *ifname, int autonegotiation, diff --git a/src/udev/net/link-config.c b/src/udev/net/link-config.c index 280403a8c5..d1404fed8b 100644 --- a/src/udev/net/link-config.c +++ b/src/udev/net/link-config.c @@ -491,7 +491,7 @@ static int link_apply_ethtool_settings(Link *link, int *ethtool_fd) { return 0; } - r = ethtool_set_glinksettings(ethtool_fd, name, + r = ethtool_set_link_settings(ethtool_fd, name, config->autonegotiation, config->advertise, config->speed, config->duplex, config->port, config->mdi); if (r < 0) { From f8606bc54e61672f8f6da4a5141544e999ca2ca3 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Tue, 2 Sep 2025 10:39:45 +0900 Subject: [PATCH 3/4] ethtool-util: fix comment The very initial implementation sets cmd with zero when the bitmap length does not match. But, it is fixed by https://github.com/torvalds/linux/commit/793cf87de9d1a62dc9079c3ec5fcc01cfc62fafb Fortunately, our code does not follow the outdated comment, but checks cmd field correctly. --- src/shared/ethtool-util.c | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/src/shared/ethtool-util.c b/src/shared/ethtool-util.c index 8b437c5a83..a443686d87 100644 --- a/src/shared/ethtool-util.c +++ b/src/shared/ethtool-util.c @@ -661,13 +661,12 @@ static int get_link_settings(int fd, struct ifreq *ifr, union ethtool_link_usett assert(ifr); assert(ret); - /* The interaction user/kernel via the new API requires a small ETHTOOL_GLINKSETTINGS - handshake first to agree on the length of the link mode bitmaps. If kernel doesn't - agree with user, it returns the bitmap length it is expecting from user as a negative - length (and cmd field is 0). When kernel and user agree, kernel returns valid info in - all fields (ie. link mode length > 0 and cmd is ETHTOOL_GLINKSETTINGS). Based on - https://github.com/torvalds/linux/commit/3f1ac7a700d039c61d8d8b99f28d605d489a60cf - */ + /* The interaction user/kernel via the new API requires a small ETHTOOL_GLINKSETTINGS handshake first + * to agree on the length of the link mode bitmaps. If kernel doesn't agree with user, it returns the + * bitmap length it is expecting from user as a negative length. When kernel and user agree, kernel + * returns valid info in all fields (with bitmap length > 0). + * https://github.com/torvalds/linux/commit/3f1ac7a700d039c61d8d8b99f28d605d489a60cf + * https://github.com/torvalds/linux/commit/793cf87de9d1a62dc9079c3ec5fcc01cfc62fafb */ ifr->ifr_data = (void*) &ecmd; From f6e9e1e304d6aaa3bddaeea56e89948b61bc77c6 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Tue, 2 Sep 2025 12:13:03 +0900 Subject: [PATCH 4/4] ethtool-util: drop use of union ethtool_link_usettings Previously, we shift arrays on read and then shift back on write. It is inefficient and not necessary. Let's directly use the buffer that kernel provides as is. --- src/shared/ethtool-util.c | 103 +++++++++++++++----------------------- src/shared/ethtool-util.h | 15 ------ 2 files changed, 39 insertions(+), 79 deletions(-) diff --git a/src/shared/ethtool-util.c b/src/shared/ethtool-util.c index a443686d87..74a979c184 100644 --- a/src/shared/ethtool-util.c +++ b/src/shared/ethtool-util.c @@ -652,9 +652,9 @@ int ethtool_set_features(int *ethtool_fd, const char *ifname, const int features return 0; } -static int get_link_settings(int fd, struct ifreq *ifr, union ethtool_link_usettings **ret) { - union ethtool_link_usettings ecmd = { - .base.cmd = ETHTOOL_GLINKSETTINGS, +static int get_link_settings(int fd, struct ifreq *ifr, struct ethtool_link_settings **ret) { + struct ethtool_link_settings ecmd = { + .cmd = ETHTOOL_GLINKSETTINGS, }; assert(fd >= 0); @@ -668,62 +668,34 @@ static int get_link_settings(int fd, struct ifreq *ifr, union ethtool_link_usett * https://github.com/torvalds/linux/commit/3f1ac7a700d039c61d8d8b99f28d605d489a60cf * https://github.com/torvalds/linux/commit/793cf87de9d1a62dc9079c3ec5fcc01cfc62fafb */ - ifr->ifr_data = (void*) &ecmd; + ifr->ifr_data = &ecmd; if (ioctl(fd, SIOCETHTOOL, ifr) < 0) return -errno; - if (ecmd.base.link_mode_masks_nwords >= 0 || ecmd.base.cmd != ETHTOOL_GLINKSETTINGS) + if (ecmd.link_mode_masks_nwords >= 0 || ecmd.cmd != ETHTOOL_GLINKSETTINGS) return -EOPNOTSUPP; - ecmd.base.link_mode_masks_nwords = -ecmd.base.link_mode_masks_nwords; - - ifr->ifr_data = (void *) &ecmd; - - if (ioctl(fd, SIOCETHTOOL, ifr) < 0) - return -errno; - - if (ecmd.base.link_mode_masks_nwords <= 0 || ecmd.base.cmd != ETHTOOL_GLINKSETTINGS) - return -EOPNOTSUPP; - - union ethtool_link_usettings *u = new0(union ethtool_link_usettings, 1); - if (!u) + int8_t n = -ecmd.link_mode_masks_nwords; + size_t sz = offsetof(struct ethtool_link_settings, link_mode_masks) + sizeof(uint32_t) * n * 3; + _cleanup_free_ struct ethtool_link_settings *settings = calloc(sz, 1); + if (!settings) return -ENOMEM; - u->base = ecmd.base; + settings->cmd = ETHTOOL_GLINKSETTINGS; + settings->link_mode_masks_nwords = n; - uint32_t *p = ecmd.base.link_mode_masks; - memcpy(u->link_modes.supported, p, sizeof(uint32_t) * ecmd.base.link_mode_masks_nwords); - p += ecmd.base.link_mode_masks_nwords; - memcpy(u->link_modes.advertising, p, sizeof(uint32_t) * ecmd.base.link_mode_masks_nwords); - p += ecmd.base.link_mode_masks_nwords; - memcpy(u->link_modes.lp_advertising, p, sizeof(uint32_t) * ecmd.base.link_mode_masks_nwords); + ifr->ifr_data = settings; + if (ioctl(fd, SIOCETHTOOL, ifr) < 0) + return -errno; - *ret = u; + if (settings->link_mode_masks_nwords != n || settings->cmd != ETHTOOL_GLINKSETTINGS) + return -EOPNOTSUPP; + + *ret = TAKE_PTR(settings); return 0; } -static int set_link_settings(int fd, struct ifreq *ifr, const union ethtool_link_usettings *u) { - assert(fd >= 0); - assert(ifr); - assert(u); - - if (u->base.cmd != ETHTOOL_GLINKSETTINGS || u->base.link_mode_masks_nwords <= 0) - return -EINVAL; - - union ethtool_link_usettings ecmd = { .base = u->base }; - ecmd.base.cmd = ETHTOOL_SLINKSETTINGS; - - uint32_t *p = ecmd.base.link_mode_masks; - p = mempcpy(p, u->link_modes.supported, sizeof(uint32_t) * ecmd.base.link_mode_masks_nwords); - p = mempcpy(p, u->link_modes.advertising, sizeof(uint32_t) * ecmd.base.link_mode_masks_nwords); - memcpy(p, u->link_modes.lp_advertising, sizeof(uint32_t) * ecmd.base.link_mode_masks_nwords); - - ifr->ifr_data = (void *) &ecmd; - - return RET_NERRNO(ioctl(fd, SIOCETHTOOL, ifr)); -} - int ethtool_set_link_settings( int *fd, const char *ifname, @@ -734,7 +706,7 @@ int ethtool_set_link_settings( NetDevPort port, uint8_t mdi) { - _cleanup_free_ union ethtool_link_usettings *u = NULL; + _cleanup_free_ struct ethtool_link_settings *settings = NULL; struct ifreq ifr = {}; bool changed = false; int r; @@ -771,47 +743,50 @@ int ethtool_set_link_settings( strscpy(ifr.ifr_name, sizeof(ifr.ifr_name), ifname); - r = get_link_settings(*fd, &ifr, &u); + r = get_link_settings(*fd, &ifr, &settings); if (r < 0) return log_debug_errno(r, "ethtool: Cannot get link settings for %s: %m", ifname); if (speed > 0) - UPDATE(u->base.speed, DIV_ROUND_UP(speed, 1000000), changed); + UPDATE(settings->speed, DIV_ROUND_UP(speed, 1000000), changed); if (duplex >= 0) - UPDATE(u->base.duplex, duplex, changed); + UPDATE(settings->duplex, duplex, changed); if (port >= 0) - UPDATE(u->base.port, port, changed); + UPDATE(settings->port, port, changed); if (autonegotiation >= 0) - UPDATE(u->base.autoneg, autonegotiation, changed); + UPDATE(settings->autoneg, autonegotiation, changed); if (!memeqzero(advertise, sizeof(uint32_t) * N_ADVERTISE)) { - UPDATE(u->base.autoneg, AUTONEG_ENABLE, changed); + UPDATE(settings->autoneg, AUTONEG_ENABLE, changed); + + uint32_t *a = settings->link_mode_masks + settings->link_mode_masks_nwords; + size_t n = MIN(settings->link_mode_masks_nwords, N_ADVERTISE); changed = changed || - memcmp(&u->link_modes.advertising, advertise, sizeof(uint32_t) * N_ADVERTISE) != 0 || - !memeqzero((uint8_t*) &u->link_modes.advertising + sizeof(uint32_t) * N_ADVERTISE, - ETHTOOL_LINK_MODE_MASK_MAX_KERNEL_NBYTES - sizeof(uint32_t) * N_ADVERTISE); - memcpy(&u->link_modes.advertising, advertise, sizeof(uint32_t) * N_ADVERTISE); - memzero((uint8_t*) &u->link_modes.advertising + sizeof(uint32_t) * N_ADVERTISE, - ETHTOOL_LINK_MODE_MASK_MAX_KERNEL_NBYTES - sizeof(uint32_t) * N_ADVERTISE); + memcmp(a, advertise, sizeof(uint32_t) * n) != 0 || + !memeqzero(a + n, sizeof(uint32_t) * (settings->link_mode_masks_nwords - n)); + + memcpy(a, advertise, sizeof(uint32_t) * n); + memzero(a + n, sizeof(uint32_t) * (settings->link_mode_masks_nwords - n)); } if (mdi != ETH_TP_MDI_INVALID) { - if (u->base.eth_tp_mdix_ctrl == ETH_TP_MDI_INVALID) + if (settings->eth_tp_mdix_ctrl == ETH_TP_MDI_INVALID) log_debug("ethtool: setting MDI not supported for %s, ignoring.", ifname); else - UPDATE(u->base.eth_tp_mdix_ctrl, mdi, changed); + UPDATE(settings->eth_tp_mdix_ctrl, mdi, changed); } if (!changed) return 0; - r = set_link_settings(*fd, &ifr, u); - if (r < 0) - return log_debug_errno(r, "ethtool: Cannot set link settings for %s: %m", ifname); + settings->cmd = ETHTOOL_SLINKSETTINGS; + ifr.ifr_data = settings; + if (ioctl(*fd, SIOCETHTOOL, &ifr) < 0) + return log_debug_errno(errno, "ethtool: Cannot set link settings for %s: %m", ifname); return r; } diff --git a/src/shared/ethtool-util.h b/src/shared/ethtool-util.h index a17c33fe47..d7bb2d0a2d 100644 --- a/src/shared/ethtool-util.h +++ b/src/shared/ethtool-util.h @@ -100,21 +100,6 @@ typedef enum NetDevPort { _NET_DEV_PORT_INVALID = -EINVAL, } NetDevPort; -#define ETHTOOL_LINK_MODE_MASK_MAX_KERNEL_NU32 (SCHAR_MAX) -#define ETHTOOL_LINK_MODE_MASK_MAX_KERNEL_NBYTES (4 * ETHTOOL_LINK_MODE_MASK_MAX_KERNEL_NU32) - -/* layout of the struct passed from/to userland */ -union ethtool_link_usettings { - struct ethtool_link_settings base; - - struct { - uint8_t header[offsetof(struct ethtool_link_settings, link_mode_masks)]; - uint32_t supported[ETHTOOL_LINK_MODE_MASK_MAX_KERNEL_NU32]; - uint32_t advertising[ETHTOOL_LINK_MODE_MASK_MAX_KERNEL_NU32]; - uint32_t lp_advertising[ETHTOOL_LINK_MODE_MASK_MAX_KERNEL_NU32]; - } link_modes; -}; - typedef struct u32_opt { uint32_t value; /* a value of 0 indicates the hardware advertised maximum should be used. */ bool set;