* Re: [PATCH 0/7] net: Convert dev_set_mac_address() to struct sockaddr_storage
From: Kuniyuki Iwashima @ 2025-05-21 0:19 UTC (permalink / raw)
To: kees
Cc: ahmed.zaki, aleksander.lobakin, alex.aring, andrew+netdev, ardb,
christophe.leroy, cratiu, d.bogdanov, davem, decui, dianders,
ebiggers, edumazet, fercerpav, gmazyland, grundler, haiyangz,
hayeswang, hch, horms, idosch, jiri, jv, kch, kuba, kuniyu, kys,
leiyang, linux-hardening, linux-hyperv, linux-kernel, linux-nvme,
linux-scsi, linux-usb, linux-wpan, linux, martin.petersen,
mgurtovoy, michael.christie, mingzhe.zou, miquel.raynal, mlombard,
netdev, pabeni, phahn-oss, sagi, sam, sdf, shaw.leon, stefan,
target-devel, viro, wei.liu
In-Reply-To: <20250520222452.work.063-kees@kernel.org>
From: Kees Cook <kees@kernel.org>
Date: Tue, 20 May 2025 15:30:59 -0700
> Hi,
>
> As part of the effort to allow the compiler to reason about object sizes,
> we need to deal with the problematic variably sized struct sockaddr,
> which has no internal runtime size tracking. In much of the network
> stack the use of struct sockaddr_storage has been adopted. Continue the
> transition toward this for more of the internal APIs. Specifically:
>
> - inet_addr_is_any()
> - netif_set_mac_address()
> - dev_set_mac_address()
>
> Only 3 callers of dev_set_mac_address() needed adjustment; all others
> were already using struct sockaddr_storage internally.
I guess dev_set_mac_address_user() was missed on the way ?
For example, tap_ioctl() still uses sockaddr and calls
dev_set_mac_address_user(), which cast it to _storage.
^ permalink raw reply
* Re: [PATCH 1/7] net: core: Convert inet_addr_is_any() to sockaddr_storage
From: Kuniyuki Iwashima @ 2025-05-21 0:04 UTC (permalink / raw)
To: kees
Cc: ahmed.zaki, aleksander.lobakin, alex.aring, andrew+netdev, ardb,
christophe.leroy, cratiu, d.bogdanov, davem, decui, dianders,
ebiggers, edumazet, fercerpav, gmazyland, grundler, gustavoars,
haiyangz, hayeswang, hch, horms, idosch, jiri, jv, kch, kuba,
kuniyu, kys, leiyang, linux-hardening, linux-hyperv, linux-kernel,
linux-nvme, linux-scsi, linux-usb, linux-wpan, linux,
martin.petersen, mgurtovoy, michael.christie, mingzhe.zou,
miquel.raynal, mlombard, netdev, pabeni, phahn-oss, sagi, sam,
sdf, shaw.leon, stefan, target-devel, viro, wei.liu
In-Reply-To: <20250520223108.2672023-1-kees@kernel.org>
From: Kees Cook <kees@kernel.org>
Date: Tue, 20 May 2025 15:31:00 -0700
> All the callers of inet_addr_is_any() have a sockaddr_storage-backed
> sockaddr. Avoid casts and switch prototype to the actual object being
> used.
>
> Acked-by: Gustavo A. R. Silva <gustavoars@kernel.org>
> Signed-off-by: Kees Cook <kees@kernel.org>
Reviewed-by: Kuniyuki Iwashima <kuniyu@amazon.com>
^ permalink raw reply
* Re: [PATCH 7/7] rtnetlink: do_setlink: Use struct sockaddr_storage
From: Gustavo A. R. Silva @ 2025-05-20 22:50 UTC (permalink / raw)
To: Kees Cook, Kuniyuki Iwashima
Cc: Eric Dumazet, Jakub Kicinski, David S. Miller, Paolo Abeni,
Simon Horman, Ido Schimmel, netdev, Christoph Hellwig,
Sagi Grimberg, Chaitanya Kulkarni, Martin K. Petersen,
Mike Christie, Max Gurtovoy, Maurizio Lombardi, Dmitry Bogdanov,
Mingzhe Zou, Christophe Leroy, Dr. David Alan Gilbert,
Andrew Lunn, Stanislav Fomichev, Cosmin Ratiu, Lei Yang,
Samuel Mendoza-Jonas, Paul Fertser, Alexander Aring,
Stefan Schmidt, Miquel Raynal, Hayes Wang, Douglas Anderson,
Grant Grundler, Jay Vosburgh, K. Y. Srinivasan, Haiyang Zhang,
Wei Liu, Dexuan Cui, Jiri Pirko, Eric Biggers, Milan Broz,
Philipp Hahn, Ard Biesheuvel, Al Viro, Ahmed Zaki,
Alexander Lobakin, Xiao Liang, linux-kernel, linux-nvme,
linux-scsi, target-devel, linux-wpan, linux-usb, linux-hyperv,
linux-hardening
In-Reply-To: <20250520223108.2672023-7-kees@kernel.org>
On 20/05/25 16:31, Kees Cook wrote:
> Instead of a heap allocating a variably sized struct sockaddr and lying
> about the type in the call to netif_set_mac_address(), use a stack
> allocated struct sockaddr_storage. This lets us drop the cast and avoid
> the allocation.
>
> Putting "ss" on the stack means it will get a reused stack slot since
> it is the same size (128B) as other existing single-scope stack variables,
> like the vfinfo array (128B), so no additional stack space is used by
> this function.
>
> Signed-off-by: Kees Cook <kees@kernel.org>
Acked-by: Gustavo A. R. Silva <gustavoars@kernel.org>
Thanks!
-Gustavo
> ---
> Cc: Kuniyuki Iwashima <kuniyu@amazon.com>
> Cc: Eric Dumazet <edumazet@google.com>
> Cc: Jakub Kicinski <kuba@kernel.org>
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: Paolo Abeni <pabeni@redhat.com>
> Cc: Simon Horman <horms@kernel.org>
> Cc: Ido Schimmel <idosch@nvidia.com>
> Cc: <netdev@vger.kernel.org>
> ---
> net/core/rtnetlink.c | 19 ++++---------------
> 1 file changed, 4 insertions(+), 15 deletions(-)
>
> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
> index 9743f1c2ae3c..f9a35bdc58ad 100644
> --- a/net/core/rtnetlink.c
> +++ b/net/core/rtnetlink.c
> @@ -3080,17 +3080,7 @@ static int do_setlink(const struct sk_buff *skb, struct net_device *dev,
> }
>
> if (tb[IFLA_ADDRESS]) {
> - struct sockaddr *sa;
> - int len;
> -
> - len = sizeof(sa_family_t) + max_t(size_t, dev->addr_len,
> - sizeof(*sa));
> - sa = kmalloc(len, GFP_KERNEL);
> - if (!sa) {
> - err = -ENOMEM;
> - goto errout;
> - }
> - sa->sa_family = dev->type;
> + struct sockaddr_storage ss = { };
>
> netdev_unlock_ops(dev);
>
> @@ -3098,10 +3088,9 @@ static int do_setlink(const struct sk_buff *skb, struct net_device *dev,
> down_write(&dev_addr_sem);
> netdev_lock_ops(dev);
>
> - memcpy(sa->sa_data, nla_data(tb[IFLA_ADDRESS]),
> - dev->addr_len);
> - err = netif_set_mac_address(dev, (struct sockaddr_storage *)sa, extack);
> - kfree(sa);
> + ss.ss_family = dev->type;
> + memcpy(ss.__data, nla_data(tb[IFLA_ADDRESS]), dev->addr_len);
> + err = netif_set_mac_address(dev, &ss, extack);
> if (err) {
> up_write(&dev_addr_sem);
> goto errout;
^ permalink raw reply
* Re: [PATCH 5/7] net: usb: r8152: Convert to use struct sockaddr_storage internally
From: Gustavo A. R. Silva @ 2025-05-20 22:49 UTC (permalink / raw)
To: Kees Cook, Kuniyuki Iwashima
Cc: Andrew Lunn, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Hayes Wang, Douglas Anderson, Grant Grundler,
linux-usb, netdev, Christoph Hellwig, Sagi Grimberg,
Chaitanya Kulkarni, Martin K. Petersen, Mike Christie,
Max Gurtovoy, Maurizio Lombardi, Dmitry Bogdanov, Mingzhe Zou,
Christophe Leroy, Simon Horman, Dr. David Alan Gilbert,
Stanislav Fomichev, Cosmin Ratiu, Lei Yang, Ido Schimmel,
Samuel Mendoza-Jonas, Paul Fertser, Alexander Aring,
Stefan Schmidt, Miquel Raynal, Jay Vosburgh, K. Y. Srinivasan,
Haiyang Zhang, Wei Liu, Dexuan Cui, Jiri Pirko, Eric Biggers,
Milan Broz, Philipp Hahn, Ard Biesheuvel, Al Viro, Ahmed Zaki,
Alexander Lobakin, Xiao Liang, linux-kernel, linux-nvme,
linux-scsi, target-devel, linux-wpan, linux-hyperv,
linux-hardening
In-Reply-To: <20250520223108.2672023-5-kees@kernel.org>
On 20/05/25 16:31, Kees Cook wrote:
> To support coming API type changes, switch to sockaddr_storage usage
> internally.
>
> Signed-off-by: Kees Cook <kees@kernel.org>
Acked-by: Gustavo A. R. Silva <gustavoars@kernel.org>
Thanks!
-Gustavo
> ---
> Cc: Andrew Lunn <andrew+netdev@lunn.ch>
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: Eric Dumazet <edumazet@google.com>
> Cc: Jakub Kicinski <kuba@kernel.org>
> Cc: Paolo Abeni <pabeni@redhat.com>
> Cc: Hayes Wang <hayeswang@realtek.com>
> Cc: Douglas Anderson <dianders@chromium.org>
> Cc: Grant Grundler <grundler@chromium.org>
> Cc: <linux-usb@vger.kernel.org>
> Cc: <netdev@vger.kernel.org>
> ---
> drivers/net/usb/r8152.c | 52 +++++++++++++++++++++--------------------
> 1 file changed, 27 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
> index 67f5d30ffcba..b18dee1b1bb3 100644
> --- a/drivers/net/usb/r8152.c
> +++ b/drivers/net/usb/r8152.c
> @@ -1665,14 +1665,14 @@ static int
> rtl8152_set_speed(struct r8152 *tp, u8 autoneg, u32 speed, u8 duplex,
> u32 advertising);
>
> -static int __rtl8152_set_mac_address(struct net_device *netdev, void *p,
> +static int __rtl8152_set_mac_address(struct net_device *netdev,
> + struct sockaddr_storage *addr,
> bool in_resume)
> {
> struct r8152 *tp = netdev_priv(netdev);
> - struct sockaddr *addr = p;
> int ret = -EADDRNOTAVAIL;
>
> - if (!is_valid_ether_addr(addr->sa_data))
> + if (!is_valid_ether_addr(addr->__data))
> goto out1;
>
> if (!in_resume) {
> @@ -1683,10 +1683,10 @@ static int __rtl8152_set_mac_address(struct net_device *netdev, void *p,
>
> mutex_lock(&tp->control);
>
> - eth_hw_addr_set(netdev, addr->sa_data);
> + eth_hw_addr_set(netdev, addr->__data);
>
> ocp_write_byte(tp, MCU_TYPE_PLA, PLA_CRWECR, CRWECR_CONFIG);
> - pla_ocp_write(tp, PLA_IDR, BYTE_EN_SIX_BYTES, 8, addr->sa_data);
> + pla_ocp_write(tp, PLA_IDR, BYTE_EN_SIX_BYTES, 8, addr->__data);
> ocp_write_byte(tp, MCU_TYPE_PLA, PLA_CRWECR, CRWECR_NORAML);
>
> mutex_unlock(&tp->control);
> @@ -1706,7 +1706,8 @@ static int rtl8152_set_mac_address(struct net_device *netdev, void *p)
> * host system provided MAC address.
> * Examples of this are Dell TB15 and Dell WD15 docks
> */
> -static int vendor_mac_passthru_addr_read(struct r8152 *tp, struct sockaddr *sa)
> +static int vendor_mac_passthru_addr_read(struct r8152 *tp,
> + struct sockaddr_storage *ss)
> {
> acpi_status status;
> struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
> @@ -1774,47 +1775,48 @@ static int vendor_mac_passthru_addr_read(struct r8152 *tp, struct sockaddr *sa)
> ret = -EINVAL;
> goto amacout;
> }
> - memcpy(sa->sa_data, buf, 6);
> + memcpy(ss->__data, buf, 6);
> tp->netdev->addr_assign_type = NET_ADDR_STOLEN;
> netif_info(tp, probe, tp->netdev,
> - "Using pass-thru MAC addr %pM\n", sa->sa_data);
> + "Using pass-thru MAC addr %pM\n", ss->__data);
>
> amacout:
> kfree(obj);
> return ret;
> }
>
> -static int determine_ethernet_addr(struct r8152 *tp, struct sockaddr *sa)
> +static int determine_ethernet_addr(struct r8152 *tp,
> + struct sockaddr_storage *ss)
> {
> struct net_device *dev = tp->netdev;
> int ret;
>
> - sa->sa_family = dev->type;
> + ss->ss_family = dev->type;
>
> - ret = eth_platform_get_mac_address(&tp->udev->dev, sa->sa_data);
> + ret = eth_platform_get_mac_address(&tp->udev->dev, ss->__data);
> if (ret < 0) {
> if (tp->version == RTL_VER_01) {
> - ret = pla_ocp_read(tp, PLA_IDR, 8, sa->sa_data);
> + ret = pla_ocp_read(tp, PLA_IDR, 8, ss->__data);
> } else {
> /* if device doesn't support MAC pass through this will
> * be expected to be non-zero
> */
> - ret = vendor_mac_passthru_addr_read(tp, sa);
> + ret = vendor_mac_passthru_addr_read(tp, ss);
> if (ret < 0)
> ret = pla_ocp_read(tp, PLA_BACKUP, 8,
> - sa->sa_data);
> + ss->__data);
> }
> }
>
> if (ret < 0) {
> netif_err(tp, probe, dev, "Get ether addr fail\n");
> - } else if (!is_valid_ether_addr(sa->sa_data)) {
> + } else if (!is_valid_ether_addr(ss->__data)) {
> netif_err(tp, probe, dev, "Invalid ether addr %pM\n",
> - sa->sa_data);
> + ss->__data);
> eth_hw_addr_random(dev);
> - ether_addr_copy(sa->sa_data, dev->dev_addr);
> + ether_addr_copy(ss->__data, dev->dev_addr);
> netif_info(tp, probe, dev, "Random ether addr %pM\n",
> - sa->sa_data);
> + ss->__data);
> return 0;
> }
>
> @@ -1824,17 +1826,17 @@ static int determine_ethernet_addr(struct r8152 *tp, struct sockaddr *sa)
> static int set_ethernet_addr(struct r8152 *tp, bool in_resume)
> {
> struct net_device *dev = tp->netdev;
> - struct sockaddr sa;
> + struct sockaddr_storage ss;
> int ret;
>
> - ret = determine_ethernet_addr(tp, &sa);
> + ret = determine_ethernet_addr(tp, &ss);
> if (ret < 0)
> return ret;
>
> if (tp->version == RTL_VER_01)
> - eth_hw_addr_set(dev, sa.sa_data);
> + eth_hw_addr_set(dev, ss.__data);
> else
> - ret = __rtl8152_set_mac_address(dev, &sa, in_resume);
> + ret = __rtl8152_set_mac_address(dev, &ss, in_resume);
>
> return ret;
> }
> @@ -8421,7 +8423,7 @@ static int rtl8152_post_reset(struct usb_interface *intf)
> {
> struct r8152 *tp = usb_get_intfdata(intf);
> struct net_device *netdev;
> - struct sockaddr sa;
> + struct sockaddr_storage ss;
>
> if (!tp || !test_bit(PROBED_WITH_NO_ERRORS, &tp->flags))
> goto exit;
> @@ -8429,8 +8431,8 @@ static int rtl8152_post_reset(struct usb_interface *intf)
> rtl_set_accessible(tp);
>
> /* reset the MAC address in case of policy change */
> - if (determine_ethernet_addr(tp, &sa) >= 0)
> - dev_set_mac_address (tp->netdev, &sa, NULL);
> + if (determine_ethernet_addr(tp, &ss) >= 0)
> + dev_set_mac_address(tp->netdev, (struct sockaddr *)&ss, NULL);
>
> netdev = tp->netdev;
> if (!netif_running(netdev))
^ permalink raw reply
* Re: [PATCH 2/7] net: core: Switch netif_set_mac_address() to struct sockaddr_storage
From: Gustavo A. R. Silva @ 2025-05-20 22:47 UTC (permalink / raw)
To: Kees Cook, Kuniyuki Iwashima
Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Simon Horman, Andrew Lunn, Stanislav Fomichev, Cosmin Ratiu,
Lei Yang, Ido Schimmel, netdev, Christoph Hellwig, Sagi Grimberg,
Chaitanya Kulkarni, Martin K. Petersen, Mike Christie,
Max Gurtovoy, Maurizio Lombardi, Dmitry Bogdanov, Mingzhe Zou,
Christophe Leroy, Dr. David Alan Gilbert, Samuel Mendoza-Jonas,
Paul Fertser, Alexander Aring, Stefan Schmidt, Miquel Raynal,
Hayes Wang, Douglas Anderson, Grant Grundler, Jay Vosburgh,
K. Y. Srinivasan, Haiyang Zhang, Wei Liu, Dexuan Cui, Jiri Pirko,
Eric Biggers, Milan Broz, Philipp Hahn, Ard Biesheuvel, Al Viro,
Ahmed Zaki, Alexander Lobakin, Xiao Liang, linux-kernel,
linux-nvme, linux-scsi, target-devel, linux-wpan, linux-usb,
linux-hyperv, linux-hardening
In-Reply-To: <20250520223108.2672023-2-kees@kernel.org>
On 20/05/25 16:31, Kees Cook wrote:
> In order to avoid passing around struct sockaddr that has a size the
> compiler cannot reason about (nor track at runtime), convert
> netif_set_mac_address() to take struct sockaddr_storage. This is just a
> cast conversion, so there is are no binary changes. Following patches
> will make actual allocation changes.
>
> Signed-off-by: Kees Cook <kees@kernel.org>
Acked-by: Gustavo A. R. Silva <gustavoars@kernel.org>
Thanks!
-Gustavo
> ---
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: Eric Dumazet <edumazet@google.com>
> Cc: Jakub Kicinski <kuba@kernel.org>
> Cc: Paolo Abeni <pabeni@redhat.com>
> Cc: Simon Horman <horms@kernel.org>
> Cc: Andrew Lunn <andrew+netdev@lunn.ch>
> Cc: Stanislav Fomichev <sdf@fomichev.me>
> Cc: Cosmin Ratiu <cratiu@nvidia.com>
> Cc: Lei Yang <leiyang@redhat.com>
> Cc: Kuniyuki Iwashima <kuniyu@amazon.com>
> Cc: Ido Schimmel <idosch@nvidia.com>
> Cc: <netdev@vger.kernel.org>
> ---
> include/linux/netdevice.h | 2 +-
> net/core/dev.c | 10 +++++-----
> net/core/dev_api.c | 4 ++--
> net/core/rtnetlink.c | 2 +-
> 4 files changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index ea9d335de130..47200a394a02 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -4212,7 +4212,7 @@ int netif_set_mtu(struct net_device *dev, int new_mtu);
> int dev_set_mtu(struct net_device *, int);
> int dev_pre_changeaddr_notify(struct net_device *dev, const char *addr,
> struct netlink_ext_ack *extack);
> -int netif_set_mac_address(struct net_device *dev, struct sockaddr *sa,
> +int netif_set_mac_address(struct net_device *dev, struct sockaddr_storage *ss,
> struct netlink_ext_ack *extack);
> int dev_set_mac_address(struct net_device *dev, struct sockaddr *sa,
> struct netlink_ext_ack *extack);
> diff --git a/net/core/dev.c b/net/core/dev.c
> index fccf2167b235..f8c8aad7df2e 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -9655,7 +9655,7 @@ int dev_pre_changeaddr_notify(struct net_device *dev, const char *addr,
> }
> EXPORT_SYMBOL(dev_pre_changeaddr_notify);
>
> -int netif_set_mac_address(struct net_device *dev, struct sockaddr *sa,
> +int netif_set_mac_address(struct net_device *dev, struct sockaddr_storage *ss,
> struct netlink_ext_ack *extack)
> {
> const struct net_device_ops *ops = dev->netdev_ops;
> @@ -9663,15 +9663,15 @@ int netif_set_mac_address(struct net_device *dev, struct sockaddr *sa,
>
> if (!ops->ndo_set_mac_address)
> return -EOPNOTSUPP;
> - if (sa->sa_family != dev->type)
> + if (ss->ss_family != dev->type)
> return -EINVAL;
> if (!netif_device_present(dev))
> return -ENODEV;
> - err = dev_pre_changeaddr_notify(dev, sa->sa_data, extack);
> + err = dev_pre_changeaddr_notify(dev, ss->__data, extack);
> if (err)
> return err;
> - if (memcmp(dev->dev_addr, sa->sa_data, dev->addr_len)) {
> - err = ops->ndo_set_mac_address(dev, sa);
> + if (memcmp(dev->dev_addr, ss->__data, dev->addr_len)) {
> + err = ops->ndo_set_mac_address(dev, ss);
> if (err)
> return err;
> }
> diff --git a/net/core/dev_api.c b/net/core/dev_api.c
> index f9a160ab596f..b5f293e637d9 100644
> --- a/net/core/dev_api.c
> +++ b/net/core/dev_api.c
> @@ -91,7 +91,7 @@ int dev_set_mac_address_user(struct net_device *dev, struct sockaddr *sa,
>
> down_write(&dev_addr_sem);
> netdev_lock_ops(dev);
> - ret = netif_set_mac_address(dev, sa, extack);
> + ret = netif_set_mac_address(dev, (struct sockaddr_storage *)sa, extack);
> netdev_unlock_ops(dev);
> up_write(&dev_addr_sem);
>
> @@ -332,7 +332,7 @@ int dev_set_mac_address(struct net_device *dev, struct sockaddr *sa,
> int ret;
>
> netdev_lock_ops(dev);
> - ret = netif_set_mac_address(dev, sa, extack);
> + ret = netif_set_mac_address(dev, (struct sockaddr_storage *)sa, extack);
> netdev_unlock_ops(dev);
>
> return ret;
> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
> index 8a914b37ef6e..9743f1c2ae3c 100644
> --- a/net/core/rtnetlink.c
> +++ b/net/core/rtnetlink.c
> @@ -3100,7 +3100,7 @@ static int do_setlink(const struct sk_buff *skb, struct net_device *dev,
>
> memcpy(sa->sa_data, nla_data(tb[IFLA_ADDRESS]),
> dev->addr_len);
> - err = netif_set_mac_address(dev, sa, extack);
> + err = netif_set_mac_address(dev, (struct sockaddr_storage *)sa, extack);
> kfree(sa);
> if (err) {
> up_write(&dev_addr_sem);
^ permalink raw reply
* [PATCH 4/7] ieee802154: Use struct sockaddr_storage with dev_set_mac_address()
From: Kees Cook @ 2025-05-20 22:31 UTC (permalink / raw)
To: Kuniyuki Iwashima
Cc: Kees Cook, Alexander Aring, Stefan Schmidt, Miquel Raynal,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Simon Horman, linux-wpan, netdev, Christoph Hellwig,
Sagi Grimberg, Chaitanya Kulkarni, Martin K. Petersen,
Mike Christie, Max Gurtovoy, Maurizio Lombardi, Dmitry Bogdanov,
Mingzhe Zou, Christophe Leroy, Dr. David Alan Gilbert,
Andrew Lunn, Stanislav Fomichev, Cosmin Ratiu, Lei Yang,
Ido Schimmel, Samuel Mendoza-Jonas, Paul Fertser, Hayes Wang,
Douglas Anderson, Grant Grundler, Jay Vosburgh, K. Y. Srinivasan,
Haiyang Zhang, Wei Liu, Dexuan Cui, Jiri Pirko, Eric Biggers,
Milan Broz, Philipp Hahn, Ard Biesheuvel, Al Viro, Ahmed Zaki,
Alexander Lobakin, Xiao Liang, linux-kernel, linux-nvme,
linux-scsi, target-devel, linux-usb, linux-hyperv,
linux-hardening
In-Reply-To: <20250520222452.work.063-kees@kernel.org>
Switch to struct sockaddr_storage for calling dev_set_mac_address(). Add
a temporary cast to struct sockaddr, which will be removed in a
subsequent patch.
Signed-off-by: Kees Cook <kees@kernel.org>
---
Cc: Alexander Aring <alex.aring@gmail.com>
Cc: Stefan Schmidt <stefan@datenfreihafen.org>
Cc: Miquel Raynal <miquel.raynal@bootlin.com>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Eric Dumazet <edumazet@google.com>
Cc: Jakub Kicinski <kuba@kernel.org>
Cc: Paolo Abeni <pabeni@redhat.com>
Cc: Simon Horman <horms@kernel.org>
Cc: <linux-wpan@vger.kernel.org>
Cc: <netdev@vger.kernel.org>
---
net/ieee802154/nl-phy.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/net/ieee802154/nl-phy.c b/net/ieee802154/nl-phy.c
index 359249ab77bf..ee2b190e8e0d 100644
--- a/net/ieee802154/nl-phy.c
+++ b/net/ieee802154/nl-phy.c
@@ -224,17 +224,17 @@ int ieee802154_add_iface(struct sk_buff *skb, struct genl_info *info)
dev_hold(dev);
if (info->attrs[IEEE802154_ATTR_HW_ADDR]) {
- struct sockaddr addr;
+ struct sockaddr_storage addr;
- addr.sa_family = ARPHRD_IEEE802154;
- nla_memcpy(&addr.sa_data, info->attrs[IEEE802154_ATTR_HW_ADDR],
+ addr.ss_family = ARPHRD_IEEE802154;
+ nla_memcpy(&addr.__data, info->attrs[IEEE802154_ATTR_HW_ADDR],
IEEE802154_ADDR_LEN);
/* strangely enough, some callbacks (inetdev_event) from
* dev_set_mac_address require RTNL_LOCK
*/
rtnl_lock();
- rc = dev_set_mac_address(dev, &addr, NULL);
+ rc = dev_set_mac_address(dev, (struct sockaddr *)&addr, NULL);
rtnl_unlock();
if (rc)
goto dev_unregister;
--
2.34.1
^ permalink raw reply related
* [PATCH 7/7] rtnetlink: do_setlink: Use struct sockaddr_storage
From: Kees Cook @ 2025-05-20 22:31 UTC (permalink / raw)
To: Kuniyuki Iwashima
Cc: Kees Cook, Eric Dumazet, Jakub Kicinski, David S. Miller,
Paolo Abeni, Simon Horman, Ido Schimmel, netdev,
Christoph Hellwig, Sagi Grimberg, Chaitanya Kulkarni,
Martin K. Petersen, Mike Christie, Max Gurtovoy,
Maurizio Lombardi, Dmitry Bogdanov, Mingzhe Zou, Christophe Leroy,
Dr. David Alan Gilbert, Andrew Lunn, Stanislav Fomichev,
Cosmin Ratiu, Lei Yang, Samuel Mendoza-Jonas, Paul Fertser,
Alexander Aring, Stefan Schmidt, Miquel Raynal, Hayes Wang,
Douglas Anderson, Grant Grundler, Jay Vosburgh, K. Y. Srinivasan,
Haiyang Zhang, Wei Liu, Dexuan Cui, Jiri Pirko, Eric Biggers,
Milan Broz, Philipp Hahn, Ard Biesheuvel, Al Viro, Ahmed Zaki,
Alexander Lobakin, Xiao Liang, linux-kernel, linux-nvme,
linux-scsi, target-devel, linux-wpan, linux-usb, linux-hyperv,
linux-hardening
In-Reply-To: <20250520222452.work.063-kees@kernel.org>
Instead of a heap allocating a variably sized struct sockaddr and lying
about the type in the call to netif_set_mac_address(), use a stack
allocated struct sockaddr_storage. This lets us drop the cast and avoid
the allocation.
Putting "ss" on the stack means it will get a reused stack slot since
it is the same size (128B) as other existing single-scope stack variables,
like the vfinfo array (128B), so no additional stack space is used by
this function.
Signed-off-by: Kees Cook <kees@kernel.org>
---
Cc: Kuniyuki Iwashima <kuniyu@amazon.com>
Cc: Eric Dumazet <edumazet@google.com>
Cc: Jakub Kicinski <kuba@kernel.org>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Paolo Abeni <pabeni@redhat.com>
Cc: Simon Horman <horms@kernel.org>
Cc: Ido Schimmel <idosch@nvidia.com>
Cc: <netdev@vger.kernel.org>
---
net/core/rtnetlink.c | 19 ++++---------------
1 file changed, 4 insertions(+), 15 deletions(-)
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index 9743f1c2ae3c..f9a35bdc58ad 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -3080,17 +3080,7 @@ static int do_setlink(const struct sk_buff *skb, struct net_device *dev,
}
if (tb[IFLA_ADDRESS]) {
- struct sockaddr *sa;
- int len;
-
- len = sizeof(sa_family_t) + max_t(size_t, dev->addr_len,
- sizeof(*sa));
- sa = kmalloc(len, GFP_KERNEL);
- if (!sa) {
- err = -ENOMEM;
- goto errout;
- }
- sa->sa_family = dev->type;
+ struct sockaddr_storage ss = { };
netdev_unlock_ops(dev);
@@ -3098,10 +3088,9 @@ static int do_setlink(const struct sk_buff *skb, struct net_device *dev,
down_write(&dev_addr_sem);
netdev_lock_ops(dev);
- memcpy(sa->sa_data, nla_data(tb[IFLA_ADDRESS]),
- dev->addr_len);
- err = netif_set_mac_address(dev, (struct sockaddr_storage *)sa, extack);
- kfree(sa);
+ ss.ss_family = dev->type;
+ memcpy(ss.__data, nla_data(tb[IFLA_ADDRESS]), dev->addr_len);
+ err = netif_set_mac_address(dev, &ss, extack);
if (err) {
up_write(&dev_addr_sem);
goto errout;
--
2.34.1
^ permalink raw reply related
* [PATCH 5/7] net: usb: r8152: Convert to use struct sockaddr_storage internally
From: Kees Cook @ 2025-05-20 22:31 UTC (permalink / raw)
To: Kuniyuki Iwashima
Cc: Kees Cook, Andrew Lunn, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, Hayes Wang, Douglas Anderson,
Grant Grundler, linux-usb, netdev, Christoph Hellwig,
Sagi Grimberg, Chaitanya Kulkarni, Martin K. Petersen,
Mike Christie, Max Gurtovoy, Maurizio Lombardi, Dmitry Bogdanov,
Mingzhe Zou, Christophe Leroy, Simon Horman,
Dr. David Alan Gilbert, Stanislav Fomichev, Cosmin Ratiu,
Lei Yang, Ido Schimmel, Samuel Mendoza-Jonas, Paul Fertser,
Alexander Aring, Stefan Schmidt, Miquel Raynal, Jay Vosburgh,
K. Y. Srinivasan, Haiyang Zhang, Wei Liu, Dexuan Cui, Jiri Pirko,
Eric Biggers, Milan Broz, Philipp Hahn, Ard Biesheuvel, Al Viro,
Ahmed Zaki, Alexander Lobakin, Xiao Liang, linux-kernel,
linux-nvme, linux-scsi, target-devel, linux-wpan, linux-hyperv,
linux-hardening
In-Reply-To: <20250520222452.work.063-kees@kernel.org>
To support coming API type changes, switch to sockaddr_storage usage
internally.
Signed-off-by: Kees Cook <kees@kernel.org>
---
Cc: Andrew Lunn <andrew+netdev@lunn.ch>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Eric Dumazet <edumazet@google.com>
Cc: Jakub Kicinski <kuba@kernel.org>
Cc: Paolo Abeni <pabeni@redhat.com>
Cc: Hayes Wang <hayeswang@realtek.com>
Cc: Douglas Anderson <dianders@chromium.org>
Cc: Grant Grundler <grundler@chromium.org>
Cc: <linux-usb@vger.kernel.org>
Cc: <netdev@vger.kernel.org>
---
drivers/net/usb/r8152.c | 52 +++++++++++++++++++++--------------------
1 file changed, 27 insertions(+), 25 deletions(-)
diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index 67f5d30ffcba..b18dee1b1bb3 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -1665,14 +1665,14 @@ static int
rtl8152_set_speed(struct r8152 *tp, u8 autoneg, u32 speed, u8 duplex,
u32 advertising);
-static int __rtl8152_set_mac_address(struct net_device *netdev, void *p,
+static int __rtl8152_set_mac_address(struct net_device *netdev,
+ struct sockaddr_storage *addr,
bool in_resume)
{
struct r8152 *tp = netdev_priv(netdev);
- struct sockaddr *addr = p;
int ret = -EADDRNOTAVAIL;
- if (!is_valid_ether_addr(addr->sa_data))
+ if (!is_valid_ether_addr(addr->__data))
goto out1;
if (!in_resume) {
@@ -1683,10 +1683,10 @@ static int __rtl8152_set_mac_address(struct net_device *netdev, void *p,
mutex_lock(&tp->control);
- eth_hw_addr_set(netdev, addr->sa_data);
+ eth_hw_addr_set(netdev, addr->__data);
ocp_write_byte(tp, MCU_TYPE_PLA, PLA_CRWECR, CRWECR_CONFIG);
- pla_ocp_write(tp, PLA_IDR, BYTE_EN_SIX_BYTES, 8, addr->sa_data);
+ pla_ocp_write(tp, PLA_IDR, BYTE_EN_SIX_BYTES, 8, addr->__data);
ocp_write_byte(tp, MCU_TYPE_PLA, PLA_CRWECR, CRWECR_NORAML);
mutex_unlock(&tp->control);
@@ -1706,7 +1706,8 @@ static int rtl8152_set_mac_address(struct net_device *netdev, void *p)
* host system provided MAC address.
* Examples of this are Dell TB15 and Dell WD15 docks
*/
-static int vendor_mac_passthru_addr_read(struct r8152 *tp, struct sockaddr *sa)
+static int vendor_mac_passthru_addr_read(struct r8152 *tp,
+ struct sockaddr_storage *ss)
{
acpi_status status;
struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
@@ -1774,47 +1775,48 @@ static int vendor_mac_passthru_addr_read(struct r8152 *tp, struct sockaddr *sa)
ret = -EINVAL;
goto amacout;
}
- memcpy(sa->sa_data, buf, 6);
+ memcpy(ss->__data, buf, 6);
tp->netdev->addr_assign_type = NET_ADDR_STOLEN;
netif_info(tp, probe, tp->netdev,
- "Using pass-thru MAC addr %pM\n", sa->sa_data);
+ "Using pass-thru MAC addr %pM\n", ss->__data);
amacout:
kfree(obj);
return ret;
}
-static int determine_ethernet_addr(struct r8152 *tp, struct sockaddr *sa)
+static int determine_ethernet_addr(struct r8152 *tp,
+ struct sockaddr_storage *ss)
{
struct net_device *dev = tp->netdev;
int ret;
- sa->sa_family = dev->type;
+ ss->ss_family = dev->type;
- ret = eth_platform_get_mac_address(&tp->udev->dev, sa->sa_data);
+ ret = eth_platform_get_mac_address(&tp->udev->dev, ss->__data);
if (ret < 0) {
if (tp->version == RTL_VER_01) {
- ret = pla_ocp_read(tp, PLA_IDR, 8, sa->sa_data);
+ ret = pla_ocp_read(tp, PLA_IDR, 8, ss->__data);
} else {
/* if device doesn't support MAC pass through this will
* be expected to be non-zero
*/
- ret = vendor_mac_passthru_addr_read(tp, sa);
+ ret = vendor_mac_passthru_addr_read(tp, ss);
if (ret < 0)
ret = pla_ocp_read(tp, PLA_BACKUP, 8,
- sa->sa_data);
+ ss->__data);
}
}
if (ret < 0) {
netif_err(tp, probe, dev, "Get ether addr fail\n");
- } else if (!is_valid_ether_addr(sa->sa_data)) {
+ } else if (!is_valid_ether_addr(ss->__data)) {
netif_err(tp, probe, dev, "Invalid ether addr %pM\n",
- sa->sa_data);
+ ss->__data);
eth_hw_addr_random(dev);
- ether_addr_copy(sa->sa_data, dev->dev_addr);
+ ether_addr_copy(ss->__data, dev->dev_addr);
netif_info(tp, probe, dev, "Random ether addr %pM\n",
- sa->sa_data);
+ ss->__data);
return 0;
}
@@ -1824,17 +1826,17 @@ static int determine_ethernet_addr(struct r8152 *tp, struct sockaddr *sa)
static int set_ethernet_addr(struct r8152 *tp, bool in_resume)
{
struct net_device *dev = tp->netdev;
- struct sockaddr sa;
+ struct sockaddr_storage ss;
int ret;
- ret = determine_ethernet_addr(tp, &sa);
+ ret = determine_ethernet_addr(tp, &ss);
if (ret < 0)
return ret;
if (tp->version == RTL_VER_01)
- eth_hw_addr_set(dev, sa.sa_data);
+ eth_hw_addr_set(dev, ss.__data);
else
- ret = __rtl8152_set_mac_address(dev, &sa, in_resume);
+ ret = __rtl8152_set_mac_address(dev, &ss, in_resume);
return ret;
}
@@ -8421,7 +8423,7 @@ static int rtl8152_post_reset(struct usb_interface *intf)
{
struct r8152 *tp = usb_get_intfdata(intf);
struct net_device *netdev;
- struct sockaddr sa;
+ struct sockaddr_storage ss;
if (!tp || !test_bit(PROBED_WITH_NO_ERRORS, &tp->flags))
goto exit;
@@ -8429,8 +8431,8 @@ static int rtl8152_post_reset(struct usb_interface *intf)
rtl_set_accessible(tp);
/* reset the MAC address in case of policy change */
- if (determine_ethernet_addr(tp, &sa) >= 0)
- dev_set_mac_address (tp->netdev, &sa, NULL);
+ if (determine_ethernet_addr(tp, &ss) >= 0)
+ dev_set_mac_address(tp->netdev, (struct sockaddr *)&ss, NULL);
netdev = tp->netdev;
if (!netif_running(netdev))
--
2.34.1
^ permalink raw reply related
* [PATCH 6/7] net: core: Convert dev_set_mac_address() to struct sockaddr_storage
From: Kees Cook @ 2025-05-20 22:31 UTC (permalink / raw)
To: Kuniyuki Iwashima
Cc: Kees Cook, Jakub Kicinski, Jay Vosburgh, Andrew Lunn,
David S. Miller, Eric Dumazet, Paolo Abeni, K. Y. Srinivasan,
Haiyang Zhang, Wei Liu, Dexuan Cui, Jiri Pirko, Simon Horman,
Alexander Aring, Stefan Schmidt, Miquel Raynal,
Samuel Mendoza-Jonas, Paul Fertser, Hayes Wang, Douglas Anderson,
Grant Grundler, Stanislav Fomichev, Cosmin Ratiu, Lei Yang,
netdev, linux-hyperv, linux-usb, linux-wpan, Christoph Hellwig,
Sagi Grimberg, Chaitanya Kulkarni, Martin K. Petersen,
Mike Christie, Max Gurtovoy, Maurizio Lombardi, Dmitry Bogdanov,
Mingzhe Zou, Christophe Leroy, Dr. David Alan Gilbert,
Ido Schimmel, Eric Biggers, Milan Broz, Philipp Hahn,
Ard Biesheuvel, Al Viro, Ahmed Zaki, Alexander Lobakin,
Xiao Liang, linux-kernel, linux-nvme, linux-scsi, target-devel,
linux-hardening
In-Reply-To: <20250520222452.work.063-kees@kernel.org>
All users of dev_set_mac_address() are now using a struct sockaddr_storage.
Convert the internal data type to struct sockaddr_storage, drop the casts,
and update pointer types.
Signed-off-by: Kees Cook <kees@kernel.org>
---
Cc: Jakub Kicinski <kuba@kernel.org>
Cc: Jay Vosburgh <jv@jvosburgh.net>
Cc: Andrew Lunn <andrew+netdev@lunn.ch>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Eric Dumazet <edumazet@google.com>
Cc: Paolo Abeni <pabeni@redhat.com>
Cc: "K. Y. Srinivasan" <kys@microsoft.com>
Cc: Haiyang Zhang <haiyangz@microsoft.com>
Cc: Wei Liu <wei.liu@kernel.org>
Cc: Dexuan Cui <decui@microsoft.com>
Cc: Jiri Pirko <jiri@resnulli.us>
Cc: Simon Horman <horms@kernel.org>
Cc: Alexander Aring <alex.aring@gmail.com>
Cc: Stefan Schmidt <stefan@datenfreihafen.org>
Cc: Miquel Raynal <miquel.raynal@bootlin.com>
Cc: Samuel Mendoza-Jonas <sam@mendozajonas.com>
Cc: Paul Fertser <fercerpav@gmail.com>
Cc: Hayes Wang <hayeswang@realtek.com>
Cc: Douglas Anderson <dianders@chromium.org>
Cc: Grant Grundler <grundler@chromium.org>
Cc: Stanislav Fomichev <sdf@fomichev.me>
Cc: Cosmin Ratiu <cratiu@nvidia.com>
Cc: Lei Yang <leiyang@redhat.com>
Cc: <netdev@vger.kernel.org>
Cc: <linux-hyperv@vger.kernel.org>
Cc: <linux-usb@vger.kernel.org>
Cc: <linux-wpan@vger.kernel.org>
---
include/linux/netdevice.h | 2 +-
drivers/net/bonding/bond_alb.c | 8 +++-----
drivers/net/bonding/bond_main.c | 10 ++++------
drivers/net/hyperv/netvsc_drv.c | 6 +++---
drivers/net/macvlan.c | 10 +++++-----
drivers/net/team/team_core.c | 2 +-
drivers/net/usb/r8152.c | 2 +-
net/core/dev.c | 1 +
net/core/dev_api.c | 6 +++---
net/ieee802154/nl-phy.c | 2 +-
net/ncsi/ncsi-manage.c | 2 +-
11 files changed, 24 insertions(+), 27 deletions(-)
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 47200a394a02..b4242b997373 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -4214,7 +4214,7 @@ int dev_pre_changeaddr_notify(struct net_device *dev, const char *addr,
struct netlink_ext_ack *extack);
int netif_set_mac_address(struct net_device *dev, struct sockaddr_storage *ss,
struct netlink_ext_ack *extack);
-int dev_set_mac_address(struct net_device *dev, struct sockaddr *sa,
+int dev_set_mac_address(struct net_device *dev, struct sockaddr_storage *ss,
struct netlink_ext_ack *extack);
int dev_set_mac_address_user(struct net_device *dev, struct sockaddr *sa,
struct netlink_ext_ack *extack);
diff --git a/drivers/net/bonding/bond_alb.c b/drivers/net/bonding/bond_alb.c
index 7edf0fd58c34..2d37b07c8215 100644
--- a/drivers/net/bonding/bond_alb.c
+++ b/drivers/net/bonding/bond_alb.c
@@ -1035,7 +1035,7 @@ static int alb_set_slave_mac_addr(struct slave *slave, const u8 addr[],
*/
memcpy(ss.__data, addr, len);
ss.ss_family = dev->type;
- if (dev_set_mac_address(dev, (struct sockaddr *)&ss, NULL)) {
+ if (dev_set_mac_address(dev, &ss, NULL)) {
slave_err(slave->bond->dev, dev, "dev_set_mac_address on slave failed! ALB mode requires that the base driver support setting the hw address also when the network device's interface is open\n");
return -EOPNOTSUPP;
}
@@ -1273,8 +1273,7 @@ static int alb_set_mac_address(struct bonding *bond, void *addr)
break;
bond_hw_addr_copy(tmp_addr, rollback_slave->dev->dev_addr,
rollback_slave->dev->addr_len);
- dev_set_mac_address(rollback_slave->dev,
- (struct sockaddr *)&ss, NULL);
+ dev_set_mac_address(rollback_slave->dev, &ss, NULL);
dev_addr_set(rollback_slave->dev, tmp_addr);
}
@@ -1763,8 +1762,7 @@ void bond_alb_handle_active_change(struct bonding *bond, struct slave *new_slave
bond->dev->addr_len);
ss.ss_family = bond->dev->type;
/* we don't care if it can't change its mac, best effort */
- dev_set_mac_address(new_slave->dev, (struct sockaddr *)&ss,
- NULL);
+ dev_set_mac_address(new_slave->dev, &ss, NULL);
dev_addr_set(new_slave->dev, tmp_addr);
}
diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 98cf4486fcee..b92e8935d686 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -1112,8 +1112,7 @@ static void bond_do_fail_over_mac(struct bonding *bond,
ss.ss_family = bond->dev->type;
}
- rv = dev_set_mac_address(new_active->dev,
- (struct sockaddr *)&ss, NULL);
+ rv = dev_set_mac_address(new_active->dev, &ss, NULL);
if (rv) {
slave_err(bond->dev, new_active->dev, "Error %d setting MAC of new active slave\n",
-rv);
@@ -1127,8 +1126,7 @@ static void bond_do_fail_over_mac(struct bonding *bond,
new_active->dev->addr_len);
ss.ss_family = old_active->dev->type;
- rv = dev_set_mac_address(old_active->dev,
- (struct sockaddr *)&ss, NULL);
+ rv = dev_set_mac_address(old_active->dev, &ss, NULL);
if (rv)
slave_err(bond->dev, old_active->dev, "Error %d setting MAC of old active slave\n",
-rv);
@@ -2455,7 +2453,7 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev,
bond_hw_addr_copy(ss.__data, new_slave->perm_hwaddr,
new_slave->dev->addr_len);
ss.ss_family = slave_dev->type;
- dev_set_mac_address(slave_dev, (struct sockaddr *)&ss, NULL);
+ dev_set_mac_address(slave_dev, &ss, NULL);
}
err_restore_mtu:
@@ -2649,7 +2647,7 @@ static int __bond_release_one(struct net_device *bond_dev,
bond_hw_addr_copy(ss.__data, slave->perm_hwaddr,
slave->dev->addr_len);
ss.ss_family = slave_dev->type;
- dev_set_mac_address(slave_dev, (struct sockaddr *)&ss, NULL);
+ dev_set_mac_address(slave_dev, &ss, NULL);
}
if (unregister) {
diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c
index d8b169ac0343..14a0d04e21ae 100644
--- a/drivers/net/hyperv/netvsc_drv.c
+++ b/drivers/net/hyperv/netvsc_drv.c
@@ -1371,7 +1371,7 @@ static int netvsc_set_mac_addr(struct net_device *ndev, void *p)
struct net_device_context *ndc = netdev_priv(ndev);
struct net_device *vf_netdev = rtnl_dereference(ndc->vf_netdev);
struct netvsc_device *nvdev = rtnl_dereference(ndc->nvdev);
- struct sockaddr *addr = p;
+ struct sockaddr_storage *addr = p;
int err;
err = eth_prepare_mac_addr_change(ndev, p);
@@ -1387,12 +1387,12 @@ static int netvsc_set_mac_addr(struct net_device *ndev, void *p)
return err;
}
- err = rndis_filter_set_device_mac(nvdev, addr->sa_data);
+ err = rndis_filter_set_device_mac(nvdev, addr->__data);
if (!err) {
eth_commit_mac_addr_change(ndev, p);
} else if (vf_netdev) {
/* rollback change on VF */
- memcpy(addr->sa_data, ndev->dev_addr, ETH_ALEN);
+ memcpy(addr->__data, ndev->dev_addr, ETH_ALEN);
dev_set_mac_address(vf_netdev, addr, NULL);
}
diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c
index 7045b1d58754..69e879780c36 100644
--- a/drivers/net/macvlan.c
+++ b/drivers/net/macvlan.c
@@ -754,13 +754,13 @@ static int macvlan_sync_address(struct net_device *dev,
static int macvlan_set_mac_address(struct net_device *dev, void *p)
{
struct macvlan_dev *vlan = netdev_priv(dev);
- struct sockaddr *addr = p;
+ struct sockaddr_storage *addr = p;
- if (!is_valid_ether_addr(addr->sa_data))
+ if (!is_valid_ether_addr(addr->__data))
return -EADDRNOTAVAIL;
/* If the addresses are the same, this is a no-op */
- if (ether_addr_equal(dev->dev_addr, addr->sa_data))
+ if (ether_addr_equal(dev->dev_addr, addr->__data))
return 0;
if (vlan->mode == MACVLAN_MODE_PASSTHRU) {
@@ -768,10 +768,10 @@ static int macvlan_set_mac_address(struct net_device *dev, void *p)
return dev_set_mac_address(vlan->lowerdev, addr, NULL);
}
- if (macvlan_addr_busy(vlan->port, addr->sa_data))
+ if (macvlan_addr_busy(vlan->port, addr->__data))
return -EADDRINUSE;
- return macvlan_sync_address(dev, addr->sa_data);
+ return macvlan_sync_address(dev, addr->__data);
}
static void macvlan_change_rx_flags(struct net_device *dev, int change)
diff --git a/drivers/net/team/team_core.c b/drivers/net/team/team_core.c
index d8fc0c79745d..a64e661c21a1 100644
--- a/drivers/net/team/team_core.c
+++ b/drivers/net/team/team_core.c
@@ -55,7 +55,7 @@ static int __set_port_dev_addr(struct net_device *port_dev,
memcpy(addr.__data, dev_addr, port_dev->addr_len);
addr.ss_family = port_dev->type;
- return dev_set_mac_address(port_dev, (struct sockaddr *)&addr, NULL);
+ return dev_set_mac_address(port_dev, &addr, NULL);
}
static int team_port_set_orig_dev_addr(struct team_port *port)
diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index b18dee1b1bb3..d6589b24c68d 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -8432,7 +8432,7 @@ static int rtl8152_post_reset(struct usb_interface *intf)
/* reset the MAC address in case of policy change */
if (determine_ethernet_addr(tp, &ss) >= 0)
- dev_set_mac_address(tp->netdev, (struct sockaddr *)&ss, NULL);
+ dev_set_mac_address(tp->netdev, &ss, NULL);
netdev = tp->netdev;
if (!netif_running(netdev))
diff --git a/net/core/dev.c b/net/core/dev.c
index f8c8aad7df2e..1f1900ec26b2 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -9683,6 +9683,7 @@ int netif_set_mac_address(struct net_device *dev, struct sockaddr_storage *ss,
DECLARE_RWSEM(dev_addr_sem);
+/* "sa" is a true struct sockaddr with limited "sa_data" member. */
int dev_get_mac_address(struct sockaddr *sa, struct net *net, char *dev_name)
{
size_t size = sizeof(sa->sa_data_min);
diff --git a/net/core/dev_api.c b/net/core/dev_api.c
index b5f293e637d9..e80404e76ca9 100644
--- a/net/core/dev_api.c
+++ b/net/core/dev_api.c
@@ -319,20 +319,20 @@ EXPORT_SYMBOL(dev_set_allmulti);
/**
* dev_set_mac_address() - change Media Access Control Address
* @dev: device
- * @sa: new address
+ * @ss: new address
* @extack: netlink extended ack
*
* Change the hardware (MAC) address of the device
*
* Return: 0 on success, -errno on failure.
*/
-int dev_set_mac_address(struct net_device *dev, struct sockaddr *sa,
+int dev_set_mac_address(struct net_device *dev, struct sockaddr_storage *ss,
struct netlink_ext_ack *extack)
{
int ret;
netdev_lock_ops(dev);
- ret = netif_set_mac_address(dev, (struct sockaddr_storage *)sa, extack);
+ ret = netif_set_mac_address(dev, sa, extack);
netdev_unlock_ops(dev);
return ret;
diff --git a/net/ieee802154/nl-phy.c b/net/ieee802154/nl-phy.c
index ee2b190e8e0d..4c07a475c567 100644
--- a/net/ieee802154/nl-phy.c
+++ b/net/ieee802154/nl-phy.c
@@ -234,7 +234,7 @@ int ieee802154_add_iface(struct sk_buff *skb, struct genl_info *info)
* dev_set_mac_address require RTNL_LOCK
*/
rtnl_lock();
- rc = dev_set_mac_address(dev, (struct sockaddr *)&addr, NULL);
+ rc = dev_set_mac_address(dev, &addr, NULL);
rtnl_unlock();
if (rc)
goto dev_unregister;
diff --git a/net/ncsi/ncsi-manage.c b/net/ncsi/ncsi-manage.c
index 0202db2aea3e..b36947063783 100644
--- a/net/ncsi/ncsi-manage.c
+++ b/net/ncsi/ncsi-manage.c
@@ -1058,7 +1058,7 @@ static void ncsi_configure_channel(struct ncsi_dev_priv *ndp)
break;
case ncsi_dev_state_config_apply_mac:
rtnl_lock();
- ret = dev_set_mac_address(dev, (struct sockaddr *)&ndp->pending_mac, NULL);
+ ret = dev_set_mac_address(dev, &ndp->pending_mac, NULL);
rtnl_unlock();
if (ret < 0)
netdev_warn(dev, "NCSI: 'Writing MAC address to device failed\n");
--
2.34.1
^ permalink raw reply related
* [PATCH 3/7] net/ncsi: Use struct sockaddr_storage for pending_mac
From: Kees Cook @ 2025-05-20 22:31 UTC (permalink / raw)
To: Kuniyuki Iwashima
Cc: Kees Cook, Samuel Mendoza-Jonas, Paul Fertser, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, Simon Horman, netdev,
Christoph Hellwig, Sagi Grimberg, Chaitanya Kulkarni,
Martin K. Petersen, Mike Christie, Max Gurtovoy,
Maurizio Lombardi, Dmitry Bogdanov, Mingzhe Zou, Christophe Leroy,
Dr. David Alan Gilbert, Andrew Lunn, Stanislav Fomichev,
Cosmin Ratiu, Lei Yang, Ido Schimmel, Alexander Aring,
Stefan Schmidt, Miquel Raynal, Hayes Wang, Douglas Anderson,
Grant Grundler, Jay Vosburgh, K. Y. Srinivasan, Haiyang Zhang,
Wei Liu, Dexuan Cui, Jiri Pirko, Eric Biggers, Milan Broz,
Philipp Hahn, Ard Biesheuvel, Al Viro, Ahmed Zaki,
Alexander Lobakin, Xiao Liang, linux-kernel, linux-nvme,
linux-scsi, target-devel, linux-wpan, linux-usb, linux-hyperv,
linux-hardening
In-Reply-To: <20250520222452.work.063-kees@kernel.org>
To avoid future casting with coming API type changes, switch struct
ncsi_dev_priv::pending_mac to a full struct sockaddr_storage.
Signed-off-by: Kees Cook <kees@kernel.org>
---
Cc: Samuel Mendoza-Jonas <sam@mendozajonas.com>
Cc: Paul Fertser <fercerpav@gmail.com>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Eric Dumazet <edumazet@google.com>
Cc: Jakub Kicinski <kuba@kernel.org>
Cc: Paolo Abeni <pabeni@redhat.com>
Cc: Simon Horman <horms@kernel.org>
Cc: <netdev@vger.kernel.org>
---
net/ncsi/internal.h | 2 +-
net/ncsi/ncsi-manage.c | 2 +-
net/ncsi/ncsi-rsp.c | 18 +++++++++---------
3 files changed, 11 insertions(+), 11 deletions(-)
diff --git a/net/ncsi/internal.h b/net/ncsi/internal.h
index 2c260f33b55c..e76c6de0c784 100644
--- a/net/ncsi/internal.h
+++ b/net/ncsi/internal.h
@@ -322,7 +322,7 @@ struct ncsi_dev_priv {
#define NCSI_DEV_RESHUFFLE 4
#define NCSI_DEV_RESET 8 /* Reset state of NC */
unsigned int gma_flag; /* OEM GMA flag */
- struct sockaddr pending_mac; /* MAC address received from GMA */
+ struct sockaddr_storage pending_mac; /* MAC address received from GMA */
spinlock_t lock; /* Protect the NCSI device */
unsigned int package_probe_id;/* Current ID during probe */
unsigned int package_num; /* Number of packages */
diff --git a/net/ncsi/ncsi-manage.c b/net/ncsi/ncsi-manage.c
index b36947063783..0202db2aea3e 100644
--- a/net/ncsi/ncsi-manage.c
+++ b/net/ncsi/ncsi-manage.c
@@ -1058,7 +1058,7 @@ static void ncsi_configure_channel(struct ncsi_dev_priv *ndp)
break;
case ncsi_dev_state_config_apply_mac:
rtnl_lock();
- ret = dev_set_mac_address(dev, &ndp->pending_mac, NULL);
+ ret = dev_set_mac_address(dev, (struct sockaddr *)&ndp->pending_mac, NULL);
rtnl_unlock();
if (ret < 0)
netdev_warn(dev, "NCSI: 'Writing MAC address to device failed\n");
diff --git a/net/ncsi/ncsi-rsp.c b/net/ncsi/ncsi-rsp.c
index 8668888c5a2f..472cc68ad86f 100644
--- a/net/ncsi/ncsi-rsp.c
+++ b/net/ncsi/ncsi-rsp.c
@@ -628,7 +628,7 @@ static int ncsi_rsp_handler_snfc(struct ncsi_request *nr)
static int ncsi_rsp_handler_oem_gma(struct ncsi_request *nr, int mfr_id)
{
struct ncsi_dev_priv *ndp = nr->ndp;
- struct sockaddr *saddr = &ndp->pending_mac;
+ struct sockaddr_storage *saddr = &ndp->pending_mac;
struct net_device *ndev = ndp->ndev.dev;
struct ncsi_rsp_oem_pkt *rsp;
u32 mac_addr_off = 0;
@@ -644,11 +644,11 @@ static int ncsi_rsp_handler_oem_gma(struct ncsi_request *nr, int mfr_id)
else if (mfr_id == NCSI_OEM_MFR_INTEL_ID)
mac_addr_off = INTEL_MAC_ADDR_OFFSET;
- saddr->sa_family = ndev->type;
- memcpy(saddr->sa_data, &rsp->data[mac_addr_off], ETH_ALEN);
+ saddr->ss_family = ndev->type;
+ memcpy(saddr->__data, &rsp->data[mac_addr_off], ETH_ALEN);
if (mfr_id == NCSI_OEM_MFR_BCM_ID || mfr_id == NCSI_OEM_MFR_INTEL_ID)
- eth_addr_inc((u8 *)saddr->sa_data);
- if (!is_valid_ether_addr((const u8 *)saddr->sa_data))
+ eth_addr_inc(saddr->__data);
+ if (!is_valid_ether_addr(saddr->__data))
return -ENXIO;
/* Set the flag for GMA command which should only be called once */
@@ -1088,7 +1088,7 @@ static int ncsi_rsp_handler_netlink(struct ncsi_request *nr)
static int ncsi_rsp_handler_gmcma(struct ncsi_request *nr)
{
struct ncsi_dev_priv *ndp = nr->ndp;
- struct sockaddr *saddr = &ndp->pending_mac;
+ struct sockaddr_storage *saddr = &ndp->pending_mac;
struct net_device *ndev = ndp->ndev.dev;
struct ncsi_rsp_gmcma_pkt *rsp;
int i;
@@ -1105,15 +1105,15 @@ static int ncsi_rsp_handler_gmcma(struct ncsi_request *nr)
rsp->addresses[i][4], rsp->addresses[i][5]);
}
- saddr->sa_family = ndev->type;
+ saddr->ss_family = ndev->type;
for (i = 0; i < rsp->address_count; i++) {
if (!is_valid_ether_addr(rsp->addresses[i])) {
netdev_warn(ndev, "NCSI: Unable to assign %pM to device\n",
rsp->addresses[i]);
continue;
}
- memcpy(saddr->sa_data, rsp->addresses[i], ETH_ALEN);
- netdev_warn(ndev, "NCSI: Will set MAC address to %pM\n", saddr->sa_data);
+ memcpy(saddr->__data, rsp->addresses[i], ETH_ALEN);
+ netdev_warn(ndev, "NCSI: Will set MAC address to %pM\n", saddr->__data);
break;
}
--
2.34.1
^ permalink raw reply related
* [PATCH 1/7] net: core: Convert inet_addr_is_any() to sockaddr_storage
From: Kees Cook @ 2025-05-20 22:31 UTC (permalink / raw)
To: Kuniyuki Iwashima
Cc: Kees Cook, Gustavo A . R . Silva, Christoph Hellwig,
Sagi Grimberg, Chaitanya Kulkarni, Martin K. Petersen,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Mike Christie, Max Gurtovoy, Maurizio Lombardi, Dmitry Bogdanov,
Mingzhe Zou, Christophe Leroy, Simon Horman,
Dr. David Alan Gilbert, linux-nvme, linux-scsi, target-devel,
netdev, Andrew Lunn, Stanislav Fomichev, Cosmin Ratiu, Lei Yang,
Ido Schimmel, Samuel Mendoza-Jonas, Paul Fertser, Alexander Aring,
Stefan Schmidt, Miquel Raynal, Hayes Wang, Douglas Anderson,
Grant Grundler, Jay Vosburgh, K. Y. Srinivasan, Haiyang Zhang,
Wei Liu, Dexuan Cui, Jiri Pirko, Eric Biggers, Milan Broz,
Philipp Hahn, Ard Biesheuvel, Al Viro, Ahmed Zaki,
Alexander Lobakin, Xiao Liang, linux-kernel, linux-wpan,
linux-usb, linux-hyperv, linux-hardening
In-Reply-To: <20250520222452.work.063-kees@kernel.org>
All the callers of inet_addr_is_any() have a sockaddr_storage-backed
sockaddr. Avoid casts and switch prototype to the actual object being
used.
Acked-by: Gustavo A. R. Silva <gustavoars@kernel.org>
Signed-off-by: Kees Cook <kees@kernel.org>
---
Cc: Christoph Hellwig <hch@lst.de>
Cc: Sagi Grimberg <sagi@grimberg.me>
Cc: Chaitanya Kulkarni <kch@nvidia.com>
Cc: "Martin K. Petersen" <martin.petersen@oracle.com>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Eric Dumazet <edumazet@google.com>
Cc: Jakub Kicinski <kuba@kernel.org>
Cc: Paolo Abeni <pabeni@redhat.com>
Cc: Mike Christie <michael.christie@oracle.com>
Cc: Max Gurtovoy <mgurtovoy@nvidia.com>
Cc: Maurizio Lombardi <mlombard@redhat.com>
Cc: Dmitry Bogdanov <d.bogdanov@yadro.com>
Cc: Mingzhe Zou <mingzhe.zou@easystack.cn>
Cc: Christophe Leroy <christophe.leroy@csgroup.eu>
Cc: Simon Horman <horms@kernel.org>
Cc: "Dr. David Alan Gilbert" <linux@treblig.org>
Cc: linux-nvme@lists.infradead.org
Cc: linux-scsi@vger.kernel.org
Cc: target-devel@vger.kernel.org
Cc: netdev@vger.kernel.org
---
include/linux/inet.h | 2 +-
drivers/nvme/target/rdma.c | 2 +-
drivers/nvme/target/tcp.c | 2 +-
drivers/target/iscsi/iscsi_target.c | 2 +-
net/core/utils.c | 8 ++++----
5 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/include/linux/inet.h b/include/linux/inet.h
index bd8276e96e60..9158772f3559 100644
--- a/include/linux/inet.h
+++ b/include/linux/inet.h
@@ -55,6 +55,6 @@ extern int in6_pton(const char *src, int srclen, u8 *dst, int delim, const char
extern int inet_pton_with_scope(struct net *net, unsigned short af,
const char *src, const char *port, struct sockaddr_storage *addr);
-extern bool inet_addr_is_any(struct sockaddr *addr);
+bool inet_addr_is_any(struct sockaddr_storage *addr);
#endif /* _LINUX_INET_H */
diff --git a/drivers/nvme/target/rdma.c b/drivers/nvme/target/rdma.c
index 2a4536ef6184..79a5aad2e9d0 100644
--- a/drivers/nvme/target/rdma.c
+++ b/drivers/nvme/target/rdma.c
@@ -1999,7 +1999,7 @@ static void nvmet_rdma_disc_port_addr(struct nvmet_req *req,
struct nvmet_rdma_port *port = nport->priv;
struct rdma_cm_id *cm_id = port->cm_id;
- if (inet_addr_is_any((struct sockaddr *)&cm_id->route.addr.src_addr)) {
+ if (inet_addr_is_any(&cm_id->route.addr.src_addr)) {
struct nvmet_rdma_rsp *rsp =
container_of(req, struct nvmet_rdma_rsp, req);
struct rdma_cm_id *req_cm_id = rsp->queue->cm_id;
diff --git a/drivers/nvme/target/tcp.c b/drivers/nvme/target/tcp.c
index 12a5cb8641ca..5cd1cf74f8ff 100644
--- a/drivers/nvme/target/tcp.c
+++ b/drivers/nvme/target/tcp.c
@@ -2194,7 +2194,7 @@ static void nvmet_tcp_disc_port_addr(struct nvmet_req *req,
{
struct nvmet_tcp_port *port = nport->priv;
- if (inet_addr_is_any((struct sockaddr *)&port->addr)) {
+ if (inet_addr_is_any(&port->addr)) {
struct nvmet_tcp_cmd *cmd =
container_of(req, struct nvmet_tcp_cmd, req);
struct nvmet_tcp_queue *queue = cmd->queue;
diff --git a/drivers/target/iscsi/iscsi_target.c b/drivers/target/iscsi/iscsi_target.c
index 620ba6e0ab07..a2dde08c8a62 100644
--- a/drivers/target/iscsi/iscsi_target.c
+++ b/drivers/target/iscsi/iscsi_target.c
@@ -3419,7 +3419,7 @@ iscsit_build_sendtargets_response(struct iscsit_cmd *cmd,
}
}
- if (inet_addr_is_any((struct sockaddr *)&np->np_sockaddr))
+ if (inet_addr_is_any(&np->np_sockaddr))
sockaddr = &conn->local_sockaddr;
else
sockaddr = &np->np_sockaddr;
diff --git a/net/core/utils.c b/net/core/utils.c
index 27f4cffaae05..e47feeaa5a49 100644
--- a/net/core/utils.c
+++ b/net/core/utils.c
@@ -399,9 +399,9 @@ int inet_pton_with_scope(struct net *net, __kernel_sa_family_t af,
}
EXPORT_SYMBOL(inet_pton_with_scope);
-bool inet_addr_is_any(struct sockaddr *addr)
+bool inet_addr_is_any(struct sockaddr_storage *addr)
{
- if (addr->sa_family == AF_INET6) {
+ if (addr->ss_family == AF_INET6) {
struct sockaddr_in6 *in6 = (struct sockaddr_in6 *)addr;
const struct sockaddr_in6 in6_any =
{ .sin6_addr = IN6ADDR_ANY_INIT };
@@ -409,13 +409,13 @@ bool inet_addr_is_any(struct sockaddr *addr)
if (!memcmp(in6->sin6_addr.s6_addr,
in6_any.sin6_addr.s6_addr, 16))
return true;
- } else if (addr->sa_family == AF_INET) {
+ } else if (addr->ss_family == AF_INET) {
struct sockaddr_in *in = (struct sockaddr_in *)addr;
if (in->sin_addr.s_addr == htonl(INADDR_ANY))
return true;
} else {
- pr_warn("unexpected address family %u\n", addr->sa_family);
+ pr_warn("unexpected address family %u\n", addr->ss_family);
}
return false;
--
2.34.1
^ permalink raw reply related
* [PATCH 2/7] net: core: Switch netif_set_mac_address() to struct sockaddr_storage
From: Kees Cook @ 2025-05-20 22:31 UTC (permalink / raw)
To: Kuniyuki Iwashima
Cc: Kees Cook, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Simon Horman, Andrew Lunn, Stanislav Fomichev,
Cosmin Ratiu, Lei Yang, Ido Schimmel, netdev, Christoph Hellwig,
Sagi Grimberg, Chaitanya Kulkarni, Martin K. Petersen,
Mike Christie, Max Gurtovoy, Maurizio Lombardi, Dmitry Bogdanov,
Mingzhe Zou, Christophe Leroy, Dr. David Alan Gilbert,
Samuel Mendoza-Jonas, Paul Fertser, Alexander Aring,
Stefan Schmidt, Miquel Raynal, Hayes Wang, Douglas Anderson,
Grant Grundler, Jay Vosburgh, K. Y. Srinivasan, Haiyang Zhang,
Wei Liu, Dexuan Cui, Jiri Pirko, Eric Biggers, Milan Broz,
Philipp Hahn, Ard Biesheuvel, Al Viro, Ahmed Zaki,
Alexander Lobakin, Xiao Liang, linux-kernel, linux-nvme,
linux-scsi, target-devel, linux-wpan, linux-usb, linux-hyperv,
linux-hardening
In-Reply-To: <20250520222452.work.063-kees@kernel.org>
In order to avoid passing around struct sockaddr that has a size the
compiler cannot reason about (nor track at runtime), convert
netif_set_mac_address() to take struct sockaddr_storage. This is just a
cast conversion, so there is are no binary changes. Following patches
will make actual allocation changes.
Signed-off-by: Kees Cook <kees@kernel.org>
---
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Eric Dumazet <edumazet@google.com>
Cc: Jakub Kicinski <kuba@kernel.org>
Cc: Paolo Abeni <pabeni@redhat.com>
Cc: Simon Horman <horms@kernel.org>
Cc: Andrew Lunn <andrew+netdev@lunn.ch>
Cc: Stanislav Fomichev <sdf@fomichev.me>
Cc: Cosmin Ratiu <cratiu@nvidia.com>
Cc: Lei Yang <leiyang@redhat.com>
Cc: Kuniyuki Iwashima <kuniyu@amazon.com>
Cc: Ido Schimmel <idosch@nvidia.com>
Cc: <netdev@vger.kernel.org>
---
include/linux/netdevice.h | 2 +-
net/core/dev.c | 10 +++++-----
net/core/dev_api.c | 4 ++--
net/core/rtnetlink.c | 2 +-
4 files changed, 9 insertions(+), 9 deletions(-)
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index ea9d335de130..47200a394a02 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -4212,7 +4212,7 @@ int netif_set_mtu(struct net_device *dev, int new_mtu);
int dev_set_mtu(struct net_device *, int);
int dev_pre_changeaddr_notify(struct net_device *dev, const char *addr,
struct netlink_ext_ack *extack);
-int netif_set_mac_address(struct net_device *dev, struct sockaddr *sa,
+int netif_set_mac_address(struct net_device *dev, struct sockaddr_storage *ss,
struct netlink_ext_ack *extack);
int dev_set_mac_address(struct net_device *dev, struct sockaddr *sa,
struct netlink_ext_ack *extack);
diff --git a/net/core/dev.c b/net/core/dev.c
index fccf2167b235..f8c8aad7df2e 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -9655,7 +9655,7 @@ int dev_pre_changeaddr_notify(struct net_device *dev, const char *addr,
}
EXPORT_SYMBOL(dev_pre_changeaddr_notify);
-int netif_set_mac_address(struct net_device *dev, struct sockaddr *sa,
+int netif_set_mac_address(struct net_device *dev, struct sockaddr_storage *ss,
struct netlink_ext_ack *extack)
{
const struct net_device_ops *ops = dev->netdev_ops;
@@ -9663,15 +9663,15 @@ int netif_set_mac_address(struct net_device *dev, struct sockaddr *sa,
if (!ops->ndo_set_mac_address)
return -EOPNOTSUPP;
- if (sa->sa_family != dev->type)
+ if (ss->ss_family != dev->type)
return -EINVAL;
if (!netif_device_present(dev))
return -ENODEV;
- err = dev_pre_changeaddr_notify(dev, sa->sa_data, extack);
+ err = dev_pre_changeaddr_notify(dev, ss->__data, extack);
if (err)
return err;
- if (memcmp(dev->dev_addr, sa->sa_data, dev->addr_len)) {
- err = ops->ndo_set_mac_address(dev, sa);
+ if (memcmp(dev->dev_addr, ss->__data, dev->addr_len)) {
+ err = ops->ndo_set_mac_address(dev, ss);
if (err)
return err;
}
diff --git a/net/core/dev_api.c b/net/core/dev_api.c
index f9a160ab596f..b5f293e637d9 100644
--- a/net/core/dev_api.c
+++ b/net/core/dev_api.c
@@ -91,7 +91,7 @@ int dev_set_mac_address_user(struct net_device *dev, struct sockaddr *sa,
down_write(&dev_addr_sem);
netdev_lock_ops(dev);
- ret = netif_set_mac_address(dev, sa, extack);
+ ret = netif_set_mac_address(dev, (struct sockaddr_storage *)sa, extack);
netdev_unlock_ops(dev);
up_write(&dev_addr_sem);
@@ -332,7 +332,7 @@ int dev_set_mac_address(struct net_device *dev, struct sockaddr *sa,
int ret;
netdev_lock_ops(dev);
- ret = netif_set_mac_address(dev, sa, extack);
+ ret = netif_set_mac_address(dev, (struct sockaddr_storage *)sa, extack);
netdev_unlock_ops(dev);
return ret;
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index 8a914b37ef6e..9743f1c2ae3c 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -3100,7 +3100,7 @@ static int do_setlink(const struct sk_buff *skb, struct net_device *dev,
memcpy(sa->sa_data, nla_data(tb[IFLA_ADDRESS]),
dev->addr_len);
- err = netif_set_mac_address(dev, sa, extack);
+ err = netif_set_mac_address(dev, (struct sockaddr_storage *)sa, extack);
kfree(sa);
if (err) {
up_write(&dev_addr_sem);
--
2.34.1
^ permalink raw reply related
* [PATCH 0/7] net: Convert dev_set_mac_address() to struct sockaddr_storage
From: Kees Cook @ 2025-05-20 22:30 UTC (permalink / raw)
To: Kuniyuki Iwashima
Cc: Kees Cook, Christoph Hellwig, Sagi Grimberg, Chaitanya Kulkarni,
Martin K. Petersen, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Mike Christie, Max Gurtovoy, Maurizio Lombardi,
Dmitry Bogdanov, Mingzhe Zou, Christophe Leroy, Simon Horman,
Dr. David Alan Gilbert, Andrew Lunn, Stanislav Fomichev,
Cosmin Ratiu, Lei Yang, Ido Schimmel, Samuel Mendoza-Jonas,
Paul Fertser, Alexander Aring, Stefan Schmidt, Miquel Raynal,
Hayes Wang, Douglas Anderson, Grant Grundler, Jay Vosburgh,
K. Y. Srinivasan, Haiyang Zhang, Wei Liu, Dexuan Cui, Jiri Pirko,
Eric Biggers, Milan Broz, Philipp Hahn, Ard Biesheuvel, Al Viro,
Ahmed Zaki, Alexander Lobakin, Xiao Liang, linux-kernel,
linux-nvme, linux-scsi, target-devel, netdev, linux-wpan,
linux-usb, linux-hyperv, linux-hardening
Hi,
As part of the effort to allow the compiler to reason about object sizes,
we need to deal with the problematic variably sized struct sockaddr,
which has no internal runtime size tracking. In much of the network
stack the use of struct sockaddr_storage has been adopted. Continue the
transition toward this for more of the internal APIs. Specifically:
- inet_addr_is_any()
- netif_set_mac_address()
- dev_set_mac_address()
Only 3 callers of dev_set_mac_address() needed adjustment; all others
were already using struct sockaddr_storage internally.
-Kees
Kees Cook (7):
net: core: Convert inet_addr_is_any() to sockaddr_storage
net: core: Switch netif_set_mac_address() to struct sockaddr_storage
net/ncsi: Use struct sockaddr_storage for pending_mac
ieee802154: Use struct sockaddr_storage with dev_set_mac_address()
net: usb: r8152: Convert to use struct sockaddr_storage internally
net: core: Convert dev_set_mac_address() to struct sockaddr_storage
rtnetlink: do_setlink: Use struct sockaddr_storage
include/linux/inet.h | 2 +-
include/linux/netdevice.h | 4 +--
net/ncsi/internal.h | 2 +-
drivers/net/bonding/bond_alb.c | 8 ++---
drivers/net/bonding/bond_main.c | 10 +++---
drivers/net/hyperv/netvsc_drv.c | 6 ++--
drivers/net/macvlan.c | 10 +++---
drivers/net/team/team_core.c | 2 +-
drivers/net/usb/r8152.c | 52 +++++++++++++++--------------
drivers/nvme/target/rdma.c | 2 +-
drivers/nvme/target/tcp.c | 2 +-
drivers/target/iscsi/iscsi_target.c | 2 +-
net/core/dev.c | 11 +++---
net/core/dev_api.c | 6 ++--
net/core/rtnetlink.c | 19 +++--------
net/core/utils.c | 8 ++---
net/ieee802154/nl-phy.c | 6 ++--
net/ncsi/ncsi-rsp.c | 18 +++++-----
18 files changed, 79 insertions(+), 91 deletions(-)
--
2.34.1
^ permalink raw reply
* Re: [PATCH v3 2/2] Drivers: hv: Introduce mshv_vtl driver
From: Stanislav Kinsburskii @ 2025-05-20 18:55 UTC (permalink / raw)
To: Naman Jain
Cc: K . Y . Srinivasan, Haiyang Zhang, Wei Liu, Dexuan Cui,
Roman Kisel, Anirudh Rayabharam, Saurabh Sengar, Nuno Das Neves,
ALOK TIWARI, linux-kernel, linux-hyperv
In-Reply-To: <20250519045642.50609-3-namjain@linux.microsoft.com>
On Mon, May 19, 2025 at 10:26:42AM +0530, Naman Jain wrote:
> Provide an interface for Virtual Machine Monitor like OpenVMM and its
> use as OpenHCL paravisor to control VTL0 (Virtual trust Level).
> Expose devices and support IOCTLs for features like VTL creation,
> VTL0 memory management, context switch, making hypercalls,
> mapping VTL0 address space to VTL2 userspace, getting new VMBus
> messages and channel events in VTL2 etc.
>
> Co-developed-by: Roman Kisel <romank@linux.microsoft.com>
> Signed-off-by: Roman Kisel <romank@linux.microsoft.com>
> Co-developed-by: Saurabh Sengar <ssengar@linux.microsoft.com>
> Signed-off-by: Saurabh Sengar <ssengar@linux.microsoft.com>
> Reviewed-by: Roman Kisel <romank@linux.microsoft.com>
> Reviewed-by: Alok Tiwari <alok.a.tiwari@oracle.com>
> Message-ID: <20250512140432.2387503-3-namjain@linux.microsoft.com>
> Signed-off-by: Naman Jain <namjain@linux.microsoft.com>
> ---
> drivers/hv/Kconfig | 20 +
> drivers/hv/Makefile | 7 +-
> drivers/hv/mshv_vtl.h | 52 +
> drivers/hv/mshv_vtl_main.c | 1783 +++++++++++++++++++++++++++++++++++
> include/hyperv/hvgdk_mini.h | 81 ++
> include/hyperv/hvhdk.h | 1 +
> include/uapi/linux/mshv.h | 82 ++
> 7 files changed, 2025 insertions(+), 1 deletion(-)
> create mode 100644 drivers/hv/mshv_vtl.h
> create mode 100644 drivers/hv/mshv_vtl_main.c
>
> diff --git a/drivers/hv/Kconfig b/drivers/hv/Kconfig
> index eefa0b559b73..21cee5564d70 100644
> --- a/drivers/hv/Kconfig
> +++ b/drivers/hv/Kconfig
> @@ -72,4 +72,24 @@ config MSHV_ROOT
>
> If unsure, say N.
>
> +config MSHV_VTL
> + tristate "Microsoft Hyper-V VTL driver"
> + depends on HYPERV && X86_64
> + depends on TRANSPARENT_HUGEPAGE
Why does it depend on TRANSPARENT_HUGEPAGE?
<snip>
> diff --git a/include/hyperv/hvgdk_mini.h b/include/hyperv/hvgdk_mini.h
> index 1be7f6a02304..cc11000e39f4 100644
> --- a/include/hyperv/hvgdk_mini.h
> +++ b/include/hyperv/hvgdk_mini.h
> @@ -882,6 +882,23 @@ struct hv_get_vp_from_apic_id_in {
> u32 apic_ids[];
> } __packed;
>
> +union hv_register_vsm_partition_config {
> + __u64 as_u64;
Please, follow the file pattern: as_u64 -> as_uint64
> + struct {
> + __u64 enable_vtl_protection : 1;
Ditto: __u64 -> u64
> + __u64 default_vtl_protection_mask : 4;
> + __u64 zero_memory_on_reset : 1;
> + __u64 deny_lower_vtl_startup : 1;
> + __u64 intercept_acceptance : 1;
> + __u64 intercept_enable_vtl_protection : 1;
> + __u64 intercept_vp_startup : 1;
> + __u64 intercept_cpuid_unimplemented : 1;
> + __u64 intercept_unrecoverable_exception : 1;
> + __u64 intercept_page : 1;
> + __u64 mbz : 51;
> + };
> +};
> +
> /*
> diff --git a/include/hyperv/hvhdk.h b/include/hyperv/hvhdk.h
> index b4067ada02cf..9b890126e8e8 100644
> --- a/include/hyperv/hvhdk.h
> +++ b/include/hyperv/hvhdk.h
> @@ -479,6 +479,7 @@ struct hv_connection_info {
> #define HV_EVENT_FLAGS_COUNT (256 * 8)
> #define HV_EVENT_FLAGS_BYTE_COUNT (256)
> #define HV_EVENT_FLAGS32_COUNT (256 / sizeof(u32))
> +#define HV_EVENT_FLAGS_LONG_COUNT (HV_EVENT_FLAGS_BYTE_COUNT / sizeof(__u64))
Ditto
>
> /* linux side we create long version of flags to use long bit ops on flags */
> #define HV_EVENT_FLAGS_UL_COUNT (256 / sizeof(ulong))
> diff --git a/include/uapi/linux/mshv.h b/include/uapi/linux/mshv.h
> index 876bfe4e4227..a8c39b08b39a 100644
> --- a/include/uapi/linux/mshv.h
> +++ b/include/uapi/linux/mshv.h
> @@ -288,4 +288,86 @@ struct mshv_get_set_vp_state {
> * #define MSHV_ROOT_HVCALL _IOWR(MSHV_IOCTL, 0x07, struct mshv_root_hvcall)
> */
>
> +/* Structure definitions, macros and IOCTLs for mshv_vtl */
> +
> +#define MSHV_CAP_CORE_API_STABLE 0x0
> +#define MSHV_CAP_REGISTER_PAGE 0x1
> +#define MSHV_CAP_VTL_RETURN_ACTION 0x2
> +#define MSHV_CAP_DR6_SHARED 0x3
> +#define MSHV_MAX_RUN_MSG_SIZE 256
> +
> +#define MSHV_VP_MAX_REGISTERS 128
> +
> +struct mshv_vp_registers {
> + __u32 count; /* at most MSHV_VP_MAX_REGISTERS */
Same here: __u{32,64} -> u{32,64}.
Please, address everywhere.
<snip>
> +
> +/* vtl device */
> +#define MSHV_CREATE_VTL _IOR(MSHV_IOCTL, 0x1D, char)
> +#define MSHV_VTL_ADD_VTL0_MEMORY _IOW(MSHV_IOCTL, 0x21, struct mshv_vtl_ram_disposition)
> +#define MSHV_VTL_SET_POLL_FILE _IOW(MSHV_IOCTL, 0x25, struct mshv_vtl_set_poll_file)
> +#define MSHV_VTL_RETURN_TO_LOWER_VTL _IO(MSHV_IOCTL, 0x27)
> +#define MSHV_GET_VP_REGISTERS _IOWR(MSHV_IOCTL, 0x05, struct mshv_vp_registers)
> +#define MSHV_SET_VP_REGISTERS _IOW(MSHV_IOCTL, 0x06, struct mshv_vp_registers)
> +
> +/* VMBus device IOCTLs */
> +#define MSHV_SINT_SIGNAL_EVENT _IOW(MSHV_IOCTL, 0x22, struct mshv_vtl_signal_event)
> +#define MSHV_SINT_POST_MESSAGE _IOW(MSHV_IOCTL, 0x23, struct mshv_vtl_sint_post_msg)
> +#define MSHV_SINT_SET_EVENTFD _IOW(MSHV_IOCTL, 0x24, struct mshv_vtl_set_eventfd)
> +#define MSHV_SINT_PAUSE_MESSAGE_STREAM _IOW(MSHV_IOCTL, 0x25, struct mshv_sint_mask)
> +
> +/* hv_hvcall device */
> +#define MSHV_HVCALL_SETUP _IOW(MSHV_IOCTL, 0x1E, struct mshv_vtl_hvcall_setup)
> +#define MSHV_HVCALL _IOWR(MSHV_IOCTL, 0x1F, struct mshv_vtl_hvcall)
How many of these ioctls are actually used by the mshv root driver?
Should those which are VTl-specific be named as such (like MSHV_VTL_SET_POLL_FILE)?
Another option would be to keep all the names generic.
Thanks,
Stanislav
> #endif
> --
> 2.34.1
>
^ permalink raw reply
* RE: [PATCH 1/3] x86/mm: Unexport tlb_state_shared
From: Michael Kelley @ 2025-05-20 15:27 UTC (permalink / raw)
To: Peter Zijlstra, x86@kernel.org
Cc: linux-kernel@vger.kernel.org, kys@microsoft.com,
haiyangz@microsoft.com, wei.liu@kernel.org, decui@microsoft.com,
tglx@linutronix.de, mingo@redhat.com, bp@alien8.de,
dave.hansen@linux.intel.com, hpa@zytor.com, luto@kernel.org,
linux-hyperv@vger.kernel.org
In-Reply-To: <20250520110632.054673615@infradead.org>
From: Peter Zijlstra <peterz@infradead.org> Sent: Tuesday, May 20, 2025 3:56 AM
>
> Never export data; modules have no business being able to change tlb
> state.
>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
> arch/x86/hyperv/mmu.c | 9 ++-------
> arch/x86/include/asm/tlbflush.h | 2 ++
> arch/x86/mm/tlb.c | 7 ++++++-
> 3 files changed, 10 insertions(+), 8 deletions(-)
>
> --- a/arch/x86/hyperv/mmu.c
> +++ b/arch/x86/hyperv/mmu.c
> @@ -51,11 +51,6 @@ static inline int fill_gva_list(u64 gva_
> return gva_n - offset;
> }
>
> -static bool cpu_is_lazy(int cpu)
> -{
> - return per_cpu(cpu_tlbstate_shared.is_lazy, cpu);
> -}
> -
> static void hyperv_flush_tlb_multi(const struct cpumask *cpus,
> const struct flush_tlb_info *info)
> {
> @@ -113,7 +108,7 @@ static void hyperv_flush_tlb_multi(const
> goto do_ex_hypercall;
>
> for_each_cpu(cpu, cpus) {
> - if (do_lazy && cpu_is_lazy(cpu))
> + if (do_lazy && cpu_tlbstate_is_lazy(cpu))
> continue;
> vcpu = hv_cpu_number_to_vp_number(cpu);
> if (vcpu == VP_INVAL) {
> @@ -198,7 +193,7 @@ static u64 hyperv_flush_tlb_others_ex(co
>
> flush->hv_vp_set.format = HV_GENERIC_SET_SPARSE_4K;
> nr_bank = cpumask_to_vpset_skip(&flush->hv_vp_set, cpus,
> - info->freed_tables ? NULL : cpu_is_lazy);
> + info->freed_tables ? NULL : cpu_tlbstate_is_lazy);
> if (nr_bank < 0)
> return HV_STATUS_INVALID_PARAMETER;
>
> --- a/arch/x86/include/asm/tlbflush.h
> +++ b/arch/x86/include/asm/tlbflush.h
> @@ -172,6 +172,8 @@ struct tlb_state_shared {
> };
> DECLARE_PER_CPU_SHARED_ALIGNED(struct tlb_state_shared, cpu_tlbstate_shared);
>
> +bool cpu_tlbstate_is_lazy(int cpu);
> +
> bool nmi_uaccess_okay(void);
> #define nmi_uaccess_okay nmi_uaccess_okay
>
> --- a/arch/x86/mm/tlb.c
> +++ b/arch/x86/mm/tlb.c
> @@ -1322,7 +1322,12 @@ static bool should_trim_cpumask(struct m
> }
>
> DEFINE_PER_CPU_SHARED_ALIGNED(struct tlb_state_shared, cpu_tlbstate_shared);
> -EXPORT_PER_CPU_SYMBOL(cpu_tlbstate_shared);
FWIW, this EXPORT wasn't there for Hyper-V code. The EXPORT was
added in commit 2f4305b19fe6a, and it's not clear why. That commit
was 2 years before the Hyper-V MMU code started checking the lazy
flag.
> +
> +bool cpu_tlbstate_is_lazy(int cpu)
> +{
> + return per_cpu(cpu_tlbstate_shared.is_lazy, cpu);
> +}
> +EXPORT_SYMBOL_GPL(cpu_tlbstate_is_lazy);
This EXPORT isn't needed for Hyper-V. The Hyper-V MMU code is
never built as a module, even if CONFIG_HYPERV=m. And I can't
see any other reason the EXPORT would be needed.
In any case,
Reviewed-by: Michael Kelley <mhklinux@outlook.com>
>
> STATIC_NOPV void native_flush_tlb_multi(const struct cpumask *cpumask,
> const struct flush_tlb_info *info)
>
>
^ permalink raw reply
* [PATCH 0/3] x86/mm: Cleanups
From: Peter Zijlstra @ 2025-05-20 10:55 UTC (permalink / raw)
To: x86
Cc: linux-kernel, peterz, kys, haiyangz, wei.liu, decui, tglx, mingo,
bp, dave.hansen, hpa, luto, linux-hyperv
Hi!
When reviewing fea4e317f9e7 ("x86/mm: Eliminate window where TLB flushes may be
inadvertently skipped") various other issues were raised. None of these are
critical, so here goes.
^ permalink raw reply
* [PATCH 1/3] x86/mm: Unexport tlb_state_shared
From: Peter Zijlstra @ 2025-05-20 10:55 UTC (permalink / raw)
To: x86
Cc: linux-kernel, peterz, kys, haiyangz, wei.liu, decui, tglx, mingo,
bp, dave.hansen, hpa, luto, linux-hyperv
In-Reply-To: <20250520105542.283166629@infradead.org>
Never export data; modules have no business being able to change tlb
state.
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
arch/x86/hyperv/mmu.c | 9 ++-------
arch/x86/include/asm/tlbflush.h | 2 ++
arch/x86/mm/tlb.c | 7 ++++++-
3 files changed, 10 insertions(+), 8 deletions(-)
--- a/arch/x86/hyperv/mmu.c
+++ b/arch/x86/hyperv/mmu.c
@@ -51,11 +51,6 @@ static inline int fill_gva_list(u64 gva_
return gva_n - offset;
}
-static bool cpu_is_lazy(int cpu)
-{
- return per_cpu(cpu_tlbstate_shared.is_lazy, cpu);
-}
-
static void hyperv_flush_tlb_multi(const struct cpumask *cpus,
const struct flush_tlb_info *info)
{
@@ -113,7 +108,7 @@ static void hyperv_flush_tlb_multi(const
goto do_ex_hypercall;
for_each_cpu(cpu, cpus) {
- if (do_lazy && cpu_is_lazy(cpu))
+ if (do_lazy && cpu_tlbstate_is_lazy(cpu))
continue;
vcpu = hv_cpu_number_to_vp_number(cpu);
if (vcpu == VP_INVAL) {
@@ -198,7 +193,7 @@ static u64 hyperv_flush_tlb_others_ex(co
flush->hv_vp_set.format = HV_GENERIC_SET_SPARSE_4K;
nr_bank = cpumask_to_vpset_skip(&flush->hv_vp_set, cpus,
- info->freed_tables ? NULL : cpu_is_lazy);
+ info->freed_tables ? NULL : cpu_tlbstate_is_lazy);
if (nr_bank < 0)
return HV_STATUS_INVALID_PARAMETER;
--- a/arch/x86/include/asm/tlbflush.h
+++ b/arch/x86/include/asm/tlbflush.h
@@ -172,6 +172,8 @@ struct tlb_state_shared {
};
DECLARE_PER_CPU_SHARED_ALIGNED(struct tlb_state_shared, cpu_tlbstate_shared);
+bool cpu_tlbstate_is_lazy(int cpu);
+
bool nmi_uaccess_okay(void);
#define nmi_uaccess_okay nmi_uaccess_okay
--- a/arch/x86/mm/tlb.c
+++ b/arch/x86/mm/tlb.c
@@ -1322,7 +1322,12 @@ static bool should_trim_cpumask(struct m
}
DEFINE_PER_CPU_SHARED_ALIGNED(struct tlb_state_shared, cpu_tlbstate_shared);
-EXPORT_PER_CPU_SYMBOL(cpu_tlbstate_shared);
+
+bool cpu_tlbstate_is_lazy(int cpu)
+{
+ return per_cpu(cpu_tlbstate_shared.is_lazy, cpu);
+}
+EXPORT_SYMBOL_GPL(cpu_tlbstate_is_lazy);
STATIC_NOPV void native_flush_tlb_multi(const struct cpumask *cpumask,
const struct flush_tlb_info *info)
^ permalink raw reply
* [PATCH 2/3] x86/mm: Avoid repeated this_cpu_*() ops in switch_mm_irqs_off()
From: Peter Zijlstra @ 2025-05-20 10:55 UTC (permalink / raw)
To: x86
Cc: linux-kernel, peterz, kys, haiyangz, wei.liu, decui, tglx, mingo,
bp, dave.hansen, hpa, luto, linux-hyperv
In-Reply-To: <20250520105542.283166629@infradead.org>
Aside from generating slightly better code for not having to use %fs
prefixed ops, the real purpose is to clarify code by switching some to
smp_store_release() later on.
Notably, this_cpu_{read,write}() imply {READ,WRITE}_ONCE().
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
arch/x86/mm/tlb.c | 21 +++++++++++----------
1 file changed, 11 insertions(+), 10 deletions(-)
--- a/arch/x86/mm/tlb.c
+++ b/arch/x86/mm/tlb.c
@@ -51,7 +51,7 @@
/*
* Bits to mangle the TIF_SPEC_* state into the mm pointer which is
- * stored in cpu_tlb_state.last_user_mm_spec.
+ * stored in cpu_tlbstate.last_user_mm_spec.
*/
#define LAST_USER_MM_IBPB 0x1UL
#define LAST_USER_MM_L1D_FLUSH 0x2UL
@@ -782,8 +782,9 @@ static inline void cr4_update_pce_mm(str
void switch_mm_irqs_off(struct mm_struct *unused, struct mm_struct *next,
struct task_struct *tsk)
{
- struct mm_struct *prev = this_cpu_read(cpu_tlbstate.loaded_mm);
- u16 prev_asid = this_cpu_read(cpu_tlbstate.loaded_mm_asid);
+ struct tlb_state *this_tlbstate = this_cpu_ptr(&cpu_tlbstate);
+ struct mm_struct *prev = READ_ONCE(this_tlbstate->loaded_mm);
+ u16 prev_asid = READ_ONCE(this_tlbstate->loaded_mm_asid);
bool was_lazy = this_cpu_read(cpu_tlbstate_shared.is_lazy);
unsigned cpu = smp_processor_id();
unsigned long new_lam;
@@ -840,7 +841,7 @@ void switch_mm_irqs_off(struct mm_struct
if (prev == next) {
/* Not actually switching mm's */
VM_WARN_ON(is_dyn_asid(prev_asid) &&
- this_cpu_read(cpu_tlbstate.ctxs[prev_asid].ctx_id) !=
+ READ_ONCE(this_tlbstate->ctxs[prev_asid].ctx_id) !=
next->context.ctx_id);
/*
@@ -888,7 +889,7 @@ void switch_mm_irqs_off(struct mm_struct
*/
smp_mb();
next_tlb_gen = atomic64_read(&next->context.tlb_gen);
- if (this_cpu_read(cpu_tlbstate.ctxs[prev_asid].tlb_gen) ==
+ if (READ_ONCE(this_tlbstate->ctxs[prev_asid].tlb_gen) ==
next_tlb_gen)
return;
@@ -910,7 +911,7 @@ void switch_mm_irqs_off(struct mm_struct
* and others are sensitive to the window where mm_cpumask(),
* CR3 and cpu_tlbstate.loaded_mm are not all in sync.
*/
- this_cpu_write(cpu_tlbstate.loaded_mm, LOADED_MM_SWITCHING);
+ WRITE_ONCE(this_tlbstate->loaded_mm, LOADED_MM_SWITCHING);
barrier();
/* Start receiving IPIs and then read tlb_gen (and LAM below) */
@@ -925,8 +926,8 @@ void switch_mm_irqs_off(struct mm_struct
new_lam = mm_lam_cr3_mask(next);
if (ns.need_flush) {
VM_WARN_ON_ONCE(is_global_asid(ns.asid));
- this_cpu_write(cpu_tlbstate.ctxs[ns.asid].ctx_id, next->context.ctx_id);
- this_cpu_write(cpu_tlbstate.ctxs[ns.asid].tlb_gen, next_tlb_gen);
+ WRITE_ONCE(this_tlbstate->ctxs[ns.asid].ctx_id, next->context.ctx_id);
+ WRITE_ONCE(this_tlbstate->ctxs[ns.asid].tlb_gen, next_tlb_gen);
load_new_mm_cr3(next->pgd, ns.asid, new_lam, true);
trace_tlb_flush(TLB_FLUSH_ON_TASK_SWITCH, TLB_FLUSH_ALL);
@@ -940,8 +941,8 @@ void switch_mm_irqs_off(struct mm_struct
/* Make sure we write CR3 before loaded_mm. */
barrier();
- this_cpu_write(cpu_tlbstate.loaded_mm, next);
- this_cpu_write(cpu_tlbstate.loaded_mm_asid, ns.asid);
+ WRITE_ONCE(this_tlbstate->loaded_mm, next);
+ WRITE_ONCE(this_tlbstate->loaded_mm_asid, ns.asid);
cpu_tlbstate_update_lam(new_lam, mm_untag_mask(next));
if (next != prev) {
^ permalink raw reply
* [PATCH 3/3] x86/mm: Clarify should_flush_tlb() ordering
From: Peter Zijlstra @ 2025-05-20 10:55 UTC (permalink / raw)
To: x86
Cc: linux-kernel, peterz, kys, haiyangz, wei.liu, decui, tglx, mingo,
bp, dave.hansen, hpa, luto, linux-hyperv
In-Reply-To: <20250520105542.283166629@infradead.org>
The ordering in should_flush_tlb() is entirely non-obvious and is only
correct because x86 is TSO. Clarify the situation by replacing two
WRITE_ONCE()s with smp_store_release(), which on x86 is cosmetic.
Additionally, clarify the comment on should_flush_tlb().
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
arch/x86/mm/tlb.c | 30 ++++++++++++++++++++++--------
1 file changed, 22 insertions(+), 8 deletions(-)
--- a/arch/x86/mm/tlb.c
+++ b/arch/x86/mm/tlb.c
@@ -910,8 +910,10 @@ void switch_mm_irqs_off(struct mm_struct
* Indicate that CR3 is about to change. nmi_uaccess_okay()
* and others are sensitive to the window where mm_cpumask(),
* CR3 and cpu_tlbstate.loaded_mm are not all in sync.
+ *
+ * Also, see should_flush_tlb().
*/
- WRITE_ONCE(this_tlbstate->loaded_mm, LOADED_MM_SWITCHING);
+ smp_store_release(&this_tlbstate->loaded_mm, LOADED_MM_SWITCHING);
barrier();
/* Start receiving IPIs and then read tlb_gen (and LAM below) */
@@ -938,10 +940,11 @@ void switch_mm_irqs_off(struct mm_struct
trace_tlb_flush(TLB_FLUSH_ON_TASK_SWITCH, 0);
}
- /* Make sure we write CR3 before loaded_mm. */
- barrier();
-
- WRITE_ONCE(this_tlbstate->loaded_mm, next);
+ /*
+ * Make sure we write CR3 before loaded_mm.
+ * See nmi_uaccess_okay() and should_flush_tlb().
+ */
+ smp_store_release(&this_tlbstate->loaded_mm, next);
WRITE_ONCE(this_tlbstate->loaded_mm_asid, ns.asid);
cpu_tlbstate_update_lam(new_lam, mm_untag_mask(next));
@@ -1280,9 +1283,20 @@ static bool should_flush_tlb(int cpu, vo
struct flush_tlb_info *info = data;
/*
- * Order the 'loaded_mm' and 'is_lazy' against their
- * write ordering in switch_mm_irqs_off(). Ensure
- * 'is_lazy' is at least as new as 'loaded_mm'.
+ * switch_mm_irqs_off() should_flush_tlb()
+ * WRITE_ONCE(is_lazy, false); loaded_mm = READ_ONCE(loaded_mm);
+ * smp_store_release(loaded_mm, SWITCHING); smp_rmb();
+ * mov-cr3
+ * smp_store_release(loaded_mm, next)
+ * if (READ_ONCE(is_lazy))
+ * return false;
+ *
+ * Where smp_rmb() matches against either smp_store_release() to
+ * ensure that if we observe loaded_mm to be either SWITCHING or next
+ * we must also observe is_lazy == false.
+ *
+ * If this were not so, it would be possible to falsely return false
+ * and miss sending an invalidation IPI.
*/
smp_rmb();
^ permalink raw reply
* [PATCH 1/1] Documentation: hyperv: Update VMBus doc with new features and info
From: mhkelley58 @ 2025-05-20 4:44 UTC (permalink / raw)
To: haiyangz, wei.liu, decui, corbet, linux-kernel, linux-hyperv,
linux-doc
From: Michael Kelley <mhklinux@outlook.com>
Starting in the 6.15 kernel, VMBus interrupts are automatically
assigned away from a CPU that is being taken offline. Add documentation
describing this case.
Also add details of Hyper-V behavior when the primary channel of
a VMBus device is closed as the result of unbinding the device's
driver. This behavior has not changed, but it was not previously
documented.
Signed-off-by: Michael Kelley <mhklinux@outlook.com>
---
Documentation/virt/hyperv/vmbus.rst | 28 ++++++++++++++++++++++++----
1 file changed, 24 insertions(+), 4 deletions(-)
diff --git a/Documentation/virt/hyperv/vmbus.rst b/Documentation/virt/hyperv/vmbus.rst
index 1dcef6a7fda3..654bb4849972 100644
--- a/Documentation/virt/hyperv/vmbus.rst
+++ b/Documentation/virt/hyperv/vmbus.rst
@@ -250,10 +250,18 @@ interrupts are not Linux IRQs, there are no entries in /proc/interrupts
or /proc/irq corresponding to individual VMBus channel interrupts.
An online CPU in a Linux guest may not be taken offline if it has
-VMBus channel interrupts assigned to it. Any such channel
-interrupts must first be manually reassigned to another CPU as
-described above. When no channel interrupts are assigned to the
-CPU, it can be taken offline.
+VMBus channel interrupts assigned to it. Starting in kernel v6.15,
+any such interrupts are automatically reassigned to some other CPU
+at the time of offlining. The "other" CPU is chosen by the
+implementation and is not load balanced or otherwise intelligently
+determined. If the CPU is onlined again, channel interrupts previously
+assigned to it are not moved back. As a result, after multiple CPUs
+have been offlined, and perhaps onlined again, the interrupt-to-CPU
+mapping may be scrambled and non-optimal. In such a case, optimal
+assignments must be re-established manually. For kernels v6.14 and
+earlier, any conflicting channel interrupts must first be manually
+reassigned to another CPU as described above. Then when no channel
+interrupts are assigned to the CPU, it can be taken offline.
The VMBus channel interrupt handling code is designed to work
correctly even if an interrupt is received on a CPU other than the
@@ -324,3 +332,15 @@ rescinded, neither Hyper-V nor Linux retains any state about
its previous existence. Such a device might be re-added later,
in which case it is treated as an entirely new device. See
vmbus_onoffer_rescind().
+
+For some devices, such as the KVP device, Hyper-V automatically
+sends a rescind message when the primary channel is closed,
+likely as a result of unbinding the device from its driver.
+The rescind causes Linux to remove the device. But then Hyper-V
+immediately reoffers the device to the guest, causing a new
+instance of the device to be created in Linux. For other
+devices, such as the synthetic SCSI and NIC devices, closing the
+primary channel does *not* result in Hyper-V sending a rescind
+message. The device continues to exist in Linux on the VMBus,
+but with no driver bound to it. The same driver or a new driver
+can subsequently be bound to the existing instance of the device.
--
2.25.1
^ permalink raw reply related
* [PATCH v2 1/1] Drivers: hv: Always select CONFIG_SYSFB for Hyper-V guests
From: mhkelley58 @ 2025-05-20 4:01 UTC (permalink / raw)
To: kys, haiyangz, wei.liu, decui, deller, javierm, arnd
Cc: linux-kernel, linux-hyperv, stable
From: Michael Kelley <mhklinux@outlook.com>
The Hyper-V host provides guest VMs with a range of MMIO addresses
that guest VMBus drivers can use. The VMBus driver in Linux manages
that MMIO space, and allocates portions to drivers upon request. As
part of managing that MMIO space in a Generation 2 VM, the VMBus
driver must reserve the portion of the MMIO space that Hyper-V has
designated for the synthetic frame buffer, and not allocate this
space to VMBus drivers other than graphics framebuffer drivers. The
synthetic frame buffer MMIO area is described by the screen_info data
structure that is passed to the Linux kernel at boot time, so the
VMBus driver must access screen_info for Generation 2 VMs. (In
Generation 1 VMs, the framebuffer MMIO space is communicated to
the guest via a PCI pseudo-device, and access to screen_info is
not needed.)
In commit a07b50d80ab6 ("hyperv: avoid dependency on screen_info")
the VMBus driver's access to screen_info is restricted to when
CONFIG_SYSFB is enabled. CONFIG_SYSFB is typically enabled in kernels
built for Hyper-V by virtue of having at least one of CONFIG_FB_EFI,
CONFIG_FB_VESA, or CONFIG_SYSFB_SIMPLEFB enabled, so the restriction
doesn't usually affect anything. But it's valid to have none of these
enabled, in which case CONFIG_SYSFB is not enabled, and the VMBus driver
is unable to properly reserve the framebuffer MMIO space for graphics
framebuffer drivers. The framebuffer MMIO space may be assigned to
some other VMBus driver, with undefined results. As an example, if
a VM is using a PCI pass-thru NVMe controller to host the OS disk,
the PCI NVMe controller is probed before any graphics devices, and the
NVMe controller is assigned a portion of the framebuffer MMIO space.
Hyper-V reports an error to Linux during the probe, and the OS disk
fails to get setup. Then Linux fails to boot in the VM.
Fix this by having CONFIG_HYPERV always select SYSFB. Then the
VMBus driver in a Gen 2 VM can always reserve the MMIO space for the
graphics framebuffer driver, and prevent the undefined behavior. But
don't select SYSFB when building for HYPERV_VTL_MODE as VTLs other
than VTL 0 don't have a framebuffer and aren't subject to the issue.
Adding SYSFB in such cases is harmless, but would increase the image
size for no purpose.
Fixes: a07b50d80ab6 ("hyperv: avoid dependency on screen_info")
Signed-off-by: Michael Kelley <mhklinux@outlook.com>
Reviewed-by: Saurabh Sengar <ssengar@linux.microsoft.com>
---
Changes in v2:
* Made "select SYSFB" conditional on not being a build for
VTL mode (Saurabh Sengar)
drivers/hv/Kconfig | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/hv/Kconfig b/drivers/hv/Kconfig
index eefa0b559b73..1cd188b73b74 100644
--- a/drivers/hv/Kconfig
+++ b/drivers/hv/Kconfig
@@ -9,6 +9,7 @@ config HYPERV
select PARAVIRT
select X86_HV_CALLBACK_VECTOR if X86
select OF_EARLY_FLATTREE if OF
+ select SYSFB if !HYPERV_VTL_MODE
help
Select this option to run Linux as a Hyper-V client operating
system.
--
2.25.1
^ permalink raw reply related
* Re: [EXTERNAL] [PATCH 1/1] Drivers: hv: Always select CONFIG_SYSFB for Hyper-V guests
From: Saurabh Singh Sengar @ 2025-05-20 3:32 UTC (permalink / raw)
To: Michael Kelley
Cc: Saurabh Singh Sengar, KY Srinivasan, Haiyang Zhang,
wei.liu@kernel.org, Dexuan Cui, deller@gmx.de, javierm@redhat.com,
arnd@arndb.de, linux-kernel@vger.kernel.org,
linux-hyperv@vger.kernel.org, stable@vger.kernel.org
In-Reply-To: <SN6PR02MB4157FD9AB9114C8DD6FF4B82D49FA@SN6PR02MB4157.namprd02.prod.outlook.com>
On Tue, May 20, 2025 at 02:07:48AM +0000, Michael Kelley wrote:
> From: Saurabh Singh Sengar <ssengar@linux.microsoft.com> Sent: Monday, May 19, 2025 9:55 AM
> >
> > On Sat, May 17, 2025 at 06:47:22PM +0000, Michael Kelley wrote:
> > > From: Saurabh Singh Sengar <ssengar@linux.microsoft.com> Sent: Saturday, May 17, 2025 9:14 AM
> > > >
> > > > On Sat, May 17, 2025 at 01:34:20PM +0000, Michael Kelley wrote:
> > > > > From: Saurabh Singh Sengar <ssengar@microsoft.com> Sent: Friday, May 16, 2025 9:38 PM
> > > > > >
> > > > > > > From: Michael Kelley <mhklinux@outlook.com>
> > > > > > >
> > > > > > > The Hyper-V host provides guest VMs with a range of MMIO addresses that
> > > > > > > guest VMBus drivers can use. The VMBus driver in Linux manages that MMIO
> > > > > > > space, and allocates portions to drivers upon request. As part of managing
> > > > > > > that MMIO space in a Generation 2 VM, the VMBus driver must reserve the
> > > > > > > portion of the MMIO space that Hyper-V has designated for the synthetic
> > > > > > > frame buffer, and not allocate this space to VMBus drivers other than graphics
> > > > > > > framebuffer drivers. The synthetic frame buffer MMIO area is described by
> > > > > > > the screen_info data structure that is passed to the Linux kernel at boot time,
> > > > > > > so the VMBus driver must access screen_info for Generation 2 VMs. (In
> > > > > > > Generation 1 VMs, the framebuffer MMIO space is communicated to the
> > > > > > > guest via a PCI pseudo-device, and access to screen_info is not needed.)
> > > > > > >
> > > > > > > In commit a07b50d80ab6 ("hyperv: avoid dependency on screen_info") the
> > > > > > > VMBus driver's access to screen_info is restricted to when CONFIG_SYSFB is
> > > > > > > enabled. CONFIG_SYSFB is typically enabled in kernels built for Hyper-V by
> > > > > > > virtue of having at least one of CONFIG_FB_EFI, CONFIG_FB_VESA, or
> > > > > > > CONFIG_SYSFB_SIMPLEFB enabled, so the restriction doesn't usually affect
> > > > > > > anything. But it's valid to have none of these enabled, in which case
> > > > > > > CONFIG_SYSFB is not enabled, and the VMBus driver is unable to properly
> > > > > > > reserve the framebuffer MMIO space for graphics framebuffer drivers. The
> > > > > > > framebuffer MMIO space may be assigned to some other VMBus driver, with
> > > > > > > undefined results. As an example, if a VM is using a PCI pass-thru NVMe
> > > > > > > controller to host the OS disk, the PCI NVMe controller is probed before any
> > > > > > > graphic devices, and the NVMe controller is assigned a portion of the
> > > > > > > framebuffer MMIO space.
> > > > > > > Hyper-V reports an error to Linux during the probe, and the OS disk fails to
> > > > > > > get setup. Then Linux fails to boot in the VM.
> > > > > > >
> > > > > > > Fix this by having CONFIG_HYPERV always select SYSFB. Then the VMBus
> > > > > > > driver in a Gen 2 VM can always reserve the MMIO space for the graphics
> > > > > > > framebuffer driver, and prevent the undefined behavior.
> > > > > >
> > > > > > One question: Shouldn't the SYSFB be selected by actual graphics framebuffer driver
> > > > > > which is expected to use it. With this patch this option will be enabled irrespective
> > > > > > if there is any user for it or not, wondering if we can better optimize it for such systems.
> > > > > >
> > > > >
> > > > > That approach doesn't work. For a cloud-based server, it might make
> > > > > sense to build a kernel image without either of the Hyper-V graphics
> > > > > framebuffer drivers (DRM_HYPERV or HYPERV_FB) since in that case the
> > > > > Linux console is the serial console. But the problem could still occur
> > > > > where a PCI pass-thru NVMe controller tries to use the MMIO space
> > > > > that Hyper-V intends for the framebuffer. That problem is directly tied
> > > > > to CONFIG_SYSFB because it's the VMBus driver that must treat the
> > > > > framebuffer MMIO space as special. The absence or presence of a
> > > > > framebuffer driver isn't the key factor, though we've been (incorrectly)
> > > > > relying on the presence of a framebuffer driver to set CONFIG_SYSFB.
> > > > >
> > > >
> > > > Thank you for the clarification. I was concerned because SYSFB is not currently
> > > > enabled in the OpenHCL kernel, and our goal is to keep the OpenHCL configuration
> > > > as minimal as possible. I haven't yet looked into the details to determine
> > > > whether this might have any impact on the kernel binary size or runtime memory
> > > > usage. I trust this won't affect negatively.
> > > >
> > > > OpenHCL Config Ref:
> > > > https://github.com/microsoft/OHCL-Linux-Kernel/blob/product/hcl-main/6.12/Microsoft/hcl-x64.config
> > > >
> > >
> > > Good point.
> > >
> > > The OpenHCL code tree has commit a07b50d80ab6 that restricts the
> > > screen_info to being available only when CONFIG_SYSFB is enabled.
> > > But since OpenHCL in VTL2 gets its firmware info via OF instead of ACPI,
> > > I'm unsure what the Hyper-V host tells it about available MMIO space,
> > > and whether that space includes MMIO space for a framebuffer. If it
> > > doesn't, then OpenHCL won't have the problem I describe above, and
> > > it won't need CONFIG_SYSFB. This patch could be modified to do
> > >
> > > select SYSFB if !HYPERV_VTL_MODE
> >
> > I am worried that this is not very scalable, there could be more such
> > Hyper-V systems in future.
>
> I could see scalability being a problem if there were 20 more such
> Hyper-V systems in the future. But if there are just 2 or 3 more, that
> seems like it would be manageable.
>
> Regardless, I'm OK with doing this with or without the
> "if !HYPERV_VTL_MODE". I don't think we should just drop this
> entirely. When playing around with various framebuffers drivers
> a few weeks back, I personally encountered the problem of having
> built a kernel that wouldn't boot in an Azure VM with an NVMe OS
> disk. I couldn't figure out why probing the NVMe controller failed.
> It took me an hour to sort out what was happening, and I was
> familiar with the Hyper-V PCI driver. I'd like to prevent such a
> problem from happening to someone else.
I agree we want to fix this.
>
> >
> > >
> > > Can you find out what MMIO space Hyper-V provides to VTL2 via OF?
> > > It would make sense if no framebuffer is provided. And maybe
> > > screen_info itself is not set up when VTL2 is loaded, which would
> > > also make adding CONFIG_SYSFB pointless for VTL2.
> >
> > I can only see below address range passed for MMIO to VMBus driver:
> > ranges = <0x0f 0xf0000000 0x0f 0xf0000000 0x10000000>;
>
> I'm guessing the above text is what shows up in DT? I'm not sure
> how to interpret it. In normal guests, Hyper-V offers a "low MMIO"
> range that is below the 4 GiB line, and a "high MMIO" range that
> is just before the 64 GiB line. In a normal guest in Azure, I see the
> MLX driver using 0xfc0000000, which would be just below the 64 GiB
> line, and in the "high MMIO" range. The "0x0f 0xf0000000" in DT might
> be physical address 0xff0000000, which is consistent with the
> "high MMIO" range. I'm not sure how to interpret the second
> occurrence of "0xf 0xf0000000". I'm guessing the 0x10000000
> (256 Mib) is the length of the available range, which would also
> make sense.
>
> The framebuffer address is always in the "low MMIO" range. So
> if my interpretation is anywhere close to correct, DT isn't
> specifying any MMIO space for a framebuffer, and there's
> no need for CONFIG_SYSFB in a kernel running in VTL2.
>
> What's your preference for how to proceed? Adding CONFIG_SYSFB
> probably *will* increase the kernel code size, but I don't know
> by how much. I can do a measurement.
My primary preference is to ensure that OpenHCL remains unaffected.
And since there are no better alternatives I can think of, I am fine
proceeding with !HYPERV_VTL_MODE
with that,
Reviewed-by: Saurabh Sengar <ssengar@linux.microsoft.com>
- Saurabh
^ permalink raw reply
* RE: [EXTERNAL] [PATCH 1/1] Drivers: hv: Always select CONFIG_SYSFB for Hyper-V guests
From: Michael Kelley @ 2025-05-20 2:07 UTC (permalink / raw)
To: Saurabh Singh Sengar
Cc: Saurabh Singh Sengar, KY Srinivasan, Haiyang Zhang,
wei.liu@kernel.org, Dexuan Cui, deller@gmx.de, javierm@redhat.com,
arnd@arndb.de, linux-kernel@vger.kernel.org,
linux-hyperv@vger.kernel.org, stable@vger.kernel.org
In-Reply-To: <20250519165454.GA11708@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net>
From: Saurabh Singh Sengar <ssengar@linux.microsoft.com> Sent: Monday, May 19, 2025 9:55 AM
>
> On Sat, May 17, 2025 at 06:47:22PM +0000, Michael Kelley wrote:
> > From: Saurabh Singh Sengar <ssengar@linux.microsoft.com> Sent: Saturday, May 17, 2025 9:14 AM
> > >
> > > On Sat, May 17, 2025 at 01:34:20PM +0000, Michael Kelley wrote:
> > > > From: Saurabh Singh Sengar <ssengar@microsoft.com> Sent: Friday, May 16, 2025 9:38 PM
> > > > >
> > > > > > From: Michael Kelley <mhklinux@outlook.com>
> > > > > >
> > > > > > The Hyper-V host provides guest VMs with a range of MMIO addresses that
> > > > > > guest VMBus drivers can use. The VMBus driver in Linux manages that MMIO
> > > > > > space, and allocates portions to drivers upon request. As part of managing
> > > > > > that MMIO space in a Generation 2 VM, the VMBus driver must reserve the
> > > > > > portion of the MMIO space that Hyper-V has designated for the synthetic
> > > > > > frame buffer, and not allocate this space to VMBus drivers other than graphics
> > > > > > framebuffer drivers. The synthetic frame buffer MMIO area is described by
> > > > > > the screen_info data structure that is passed to the Linux kernel at boot time,
> > > > > > so the VMBus driver must access screen_info for Generation 2 VMs. (In
> > > > > > Generation 1 VMs, the framebuffer MMIO space is communicated to the
> > > > > > guest via a PCI pseudo-device, and access to screen_info is not needed.)
> > > > > >
> > > > > > In commit a07b50d80ab6 ("hyperv: avoid dependency on screen_info") the
> > > > > > VMBus driver's access to screen_info is restricted to when CONFIG_SYSFB is
> > > > > > enabled. CONFIG_SYSFB is typically enabled in kernels built for Hyper-V by
> > > > > > virtue of having at least one of CONFIG_FB_EFI, CONFIG_FB_VESA, or
> > > > > > CONFIG_SYSFB_SIMPLEFB enabled, so the restriction doesn't usually affect
> > > > > > anything. But it's valid to have none of these enabled, in which case
> > > > > > CONFIG_SYSFB is not enabled, and the VMBus driver is unable to properly
> > > > > > reserve the framebuffer MMIO space for graphics framebuffer drivers. The
> > > > > > framebuffer MMIO space may be assigned to some other VMBus driver, with
> > > > > > undefined results. As an example, if a VM is using a PCI pass-thru NVMe
> > > > > > controller to host the OS disk, the PCI NVMe controller is probed before any
> > > > > > graphic devices, and the NVMe controller is assigned a portion of the
> > > > > > framebuffer MMIO space.
> > > > > > Hyper-V reports an error to Linux during the probe, and the OS disk fails to
> > > > > > get setup. Then Linux fails to boot in the VM.
> > > > > >
> > > > > > Fix this by having CONFIG_HYPERV always select SYSFB. Then the VMBus
> > > > > > driver in a Gen 2 VM can always reserve the MMIO space for the graphics
> > > > > > framebuffer driver, and prevent the undefined behavior.
> > > > >
> > > > > One question: Shouldn't the SYSFB be selected by actual graphics framebuffer driver
> > > > > which is expected to use it. With this patch this option will be enabled irrespective
> > > > > if there is any user for it or not, wondering if we can better optimize it for such systems.
> > > > >
> > > >
> > > > That approach doesn't work. For a cloud-based server, it might make
> > > > sense to build a kernel image without either of the Hyper-V graphics
> > > > framebuffer drivers (DRM_HYPERV or HYPERV_FB) since in that case the
> > > > Linux console is the serial console. But the problem could still occur
> > > > where a PCI pass-thru NVMe controller tries to use the MMIO space
> > > > that Hyper-V intends for the framebuffer. That problem is directly tied
> > > > to CONFIG_SYSFB because it's the VMBus driver that must treat the
> > > > framebuffer MMIO space as special. The absence or presence of a
> > > > framebuffer driver isn't the key factor, though we've been (incorrectly)
> > > > relying on the presence of a framebuffer driver to set CONFIG_SYSFB.
> > > >
> > >
> > > Thank you for the clarification. I was concerned because SYSFB is not currently
> > > enabled in the OpenHCL kernel, and our goal is to keep the OpenHCL configuration
> > > as minimal as possible. I haven't yet looked into the details to determine
> > > whether this might have any impact on the kernel binary size or runtime memory
> > > usage. I trust this won't affect negatively.
> > >
> > > OpenHCL Config Ref:
> > > https://github.com/microsoft/OHCL-Linux-Kernel/blob/product/hcl-main/6.12/Microsoft/hcl-x64.config
> > >
> >
> > Good point.
> >
> > The OpenHCL code tree has commit a07b50d80ab6 that restricts the
> > screen_info to being available only when CONFIG_SYSFB is enabled.
> > But since OpenHCL in VTL2 gets its firmware info via OF instead of ACPI,
> > I'm unsure what the Hyper-V host tells it about available MMIO space,
> > and whether that space includes MMIO space for a framebuffer. If it
> > doesn't, then OpenHCL won't have the problem I describe above, and
> > it won't need CONFIG_SYSFB. This patch could be modified to do
> >
> > select SYSFB if !HYPERV_VTL_MODE
>
> I am worried that this is not very scalable, there could be more such
> Hyper-V systems in future.
I could see scalability being a problem if there were 20 more such
Hyper-V systems in the future. But if there are just 2 or 3 more, that
seems like it would be manageable.
Regardless, I'm OK with doing this with or without the
"if !HYPERV_VTL_MODE". I don't think we should just drop this
entirely. When playing around with various framebuffers drivers
a few weeks back, I personally encountered the problem of having
built a kernel that wouldn't boot in an Azure VM with an NVMe OS
disk. I couldn't figure out why probing the NVMe controller failed.
It took me an hour to sort out what was happening, and I was
familiar with the Hyper-V PCI driver. I'd like to prevent such a
problem from happening to someone else.
>
> >
> > Can you find out what MMIO space Hyper-V provides to VTL2 via OF?
> > It would make sense if no framebuffer is provided. And maybe
> > screen_info itself is not set up when VTL2 is loaded, which would
> > also make adding CONFIG_SYSFB pointless for VTL2.
>
> I can only see below address range passed for MMIO to VMBus driver:
> ranges = <0x0f 0xf0000000 0x0f 0xf0000000 0x10000000>;
I'm guessing the above text is what shows up in DT? I'm not sure
how to interpret it. In normal guests, Hyper-V offers a "low MMIO"
range that is below the 4 GiB line, and a "high MMIO" range that
is just before the 64 GiB line. In a normal guest in Azure, I see the
MLX driver using 0xfc0000000, which would be just below the 64 GiB
line, and in the "high MMIO" range. The "0x0f 0xf0000000" in DT might
be physical address 0xff0000000, which is consistent with the
"high MMIO" range. I'm not sure how to interpret the second
occurrence of "0xf 0xf0000000". I'm guessing the 0x10000000
(256 Mib) is the length of the available range, which would also
make sense.
The framebuffer address is always in the "low MMIO" range. So
if my interpretation is anywhere close to correct, DT isn't
specifying any MMIO space for a framebuffer, and there's
no need for CONFIG_SYSFB in a kernel running in VTL2.
What's your preference for how to proceed? Adding CONFIG_SYSFB
probably *will* increase the kernel code size, but I don't know
by how much. I can do a measurement.
Michael
>
> I don't think we have any use of scrren_info or framebuffer in OpenHCL.
>
> - Saurabh
^ permalink raw reply
* RE: [PATCH v3 13/13] x86/hyperv/vtl: Use the wakeup mailbox to boot secondary CPUs
From: Michael Kelley @ 2025-05-20 1:35 UTC (permalink / raw)
To: Ricardo Neri, x86@kernel.org, Krzysztof Kozlowski, Conor Dooley,
Rob Herring, K. Y. Srinivasan, Haiyang Zhang, Wei Liu, Dexuan Cui
Cc: devicetree@vger.kernel.org, Saurabh Sengar, Chris Oo,
linux-hyperv@vger.kernel.org, Kirill A. Shutemov,
linux-acpi@vger.kernel.org, linux-kernel@vger.kernel.org,
Ravi V. Shankar, Ricardo Neri
In-Reply-To: <20250503191515.24041-14-ricardo.neri-calderon@linux.intel.com>
From: Ricardo Neri <ricardo.neri-calderon@linux.intel.com> Sent: Saturday, May 3, 2025 12:15 PM
>
> The hypervisor is an untrusted entity for TDX guests. It cannot be used
> to boot secondary CPUs. The function hv_vtl_wakeup_secondary_cpu() cannot
> be used.
>
> Instead, the virtual firmware boots the secondary CPUs and places them in
> a state to transfer control to the kernel using the wakeup mailbox.
>
> The kernel updates the APIC callback wakeup_secondary_cpu_64() to use
> the mailbox if detected early during boot (enumerated via either an ACPI
> table or a DeviceTree node).
>
> Signed-off-by: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
> ---
> Changes since v2:
> - Unconditionally use the wakeup mailbox in a TDX confidential VM.
> (Michael).
> - Edited the commit message for clarity.
>
> Changes since v1:
> - None
> ---
> arch/x86/hyperv/hv_vtl.c | 10 +++++++++-
> 1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/hyperv/hv_vtl.c b/arch/x86/hyperv/hv_vtl.c
> index cd48bedd21f0..30a5a0c156c1 100644
> --- a/arch/x86/hyperv/hv_vtl.c
> +++ b/arch/x86/hyperv/hv_vtl.c
> @@ -299,7 +299,15 @@ int __init hv_vtl_early_init(void)
> panic("XSAVE has to be disabled as it is not supported by this module.\n"
> "Please add 'noxsave' to the kernel command line.\n");
>
> - apic_update_callback(wakeup_secondary_cpu_64, hv_vtl_wakeup_secondary_cpu);
> + /*
> + * TDX confidential VMs do not trust the hypervisor and cannot use it to
> + * boot secondary CPUs. Instead, they will be booted using the wakeup
> + * mailbox if detected during boot. See setup_arch().
> + *
> + * There is no paravisor present if we are here.
> + */
> + if (!hv_isolation_type_tdx())
> + apic_update_callback(wakeup_secondary_cpu_64, hv_vtl_wakeup_secondary_cpu);
>
> return 0;
> }
> --
> 2.43.0
Reviewed-by: Michael Kelley <mhklinux@outlook.com>
^ permalink raw reply
* RE: [PATCH v3 12/13] x86/hyperv/vtl: Mark the wakeup mailbox page as private
From: Michael Kelley @ 2025-05-20 1:33 UTC (permalink / raw)
To: Ricardo Neri, x86@kernel.org, Krzysztof Kozlowski, Conor Dooley,
Rob Herring, K. Y. Srinivasan, Haiyang Zhang, Wei Liu, Dexuan Cui
Cc: devicetree@vger.kernel.org, Saurabh Sengar, Chris Oo,
linux-hyperv@vger.kernel.org, Kirill A. Shutemov,
linux-acpi@vger.kernel.org, linux-kernel@vger.kernel.org,
Ravi V. Shankar, Ricardo Neri
In-Reply-To: <20250503191515.24041-13-ricardo.neri-calderon@linux.intel.com>
From: Ricardo Neri <ricardo.neri-calderon@linux.intel.com> Sent: Saturday, May 3, 2025 12:15 PM
>
> From: Yunhong Jiang <yunhong.jiang@linux.intel.com>
>
> The current code maps MMIO devices as shared (decrypted) by default in a
> confidential computing VM.
>
> In a TDX environment, secondary CPUs are booted using the Multiprocessor
> Wakeup Structure defined in the ACPI specification. The virtual firmware
> and the operating system function in the guest context, without
> intervention from the VMM. Map the physical memory of the mailbox as
> private. Use the is_private_mmio() callback.
>
> Signed-off-by: Yunhong Jiang <yunhong.jiang@linux.intel.com>
> Signed-off-by: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
> ---
> Changes since v2:
> - Use the new helper function get_mp_wakeup_mailbox_paddr().
> - Edited the commit message for clarity.
>
> Changes since v1:
> - Added the helper function within_page() to improve readability
> - Override the is_private_mmio() callback when detecting a TDX
> environment. The address of the mailbox is checked in
> hv_is_private_mmio_tdx().
> ---
> arch/x86/hyperv/hv_vtl.c | 14 ++++++++++++++
> 1 file changed, 14 insertions(+)
>
> diff --git a/arch/x86/hyperv/hv_vtl.c b/arch/x86/hyperv/hv_vtl.c
> index 8b497c8292d3..cd48bedd21f0 100644
> --- a/arch/x86/hyperv/hv_vtl.c
> +++ b/arch/x86/hyperv/hv_vtl.c
> @@ -54,6 +54,18 @@ static void __noreturn hv_vtl_restart(char __maybe_unused *cmd)
> hv_vtl_emergency_restart();
> }
>
> +static inline bool within_page(u64 addr, u64 start)
> +{
> + return addr >= start && addr < (start + PAGE_SIZE);
> +}
> +
> +static bool hv_vtl_is_private_mmio_tdx(u64 addr)
> +{
> + u64 mb_addr = get_mp_wakeup_mailbox_paddr();
> +
> + return mb_addr && within_page(addr, mb_addr);
> +}
> +
> void __init hv_vtl_init_platform(void)
> {
> pr_info("Linux runs in Hyper-V Virtual Trust Level\n");
> @@ -61,6 +73,8 @@ void __init hv_vtl_init_platform(void)
> /* There is no paravisor present if we are here. */
> if (hv_isolation_type_tdx()) {
> x86_init.resources.realmode_limit = SZ_4G;
> + x86_platform.hyper.is_private_mmio = hv_vtl_is_private_mmio_tdx;
> +
> } else {
> x86_platform.realmode_reserve = x86_init_noop;
> x86_platform.realmode_init = x86_init_noop;
> --
> 2.43.0
Reviewed-by: Michael Kelley <mhklinux@outlook.com>
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox