* [PATCH v3 10/13] KVM: Drop sanity check that per-VM list of irqfds is unique
From: Sean Christopherson @ 2025-05-22 23:52 UTC (permalink / raw)
To: K. Y. Srinivasan, Haiyang Zhang, Wei Liu, Dexuan Cui,
Juergen Gross, Stefano Stabellini, Paolo Bonzini, Ingo Molnar,
Peter Zijlstra, Juri Lelli, Vincent Guittot, Shuah Khan,
Marc Zyngier, Oliver Upton, Sean Christopherson
Cc: linux-kernel, linux-hyperv, xen-devel, kvm, linux-kselftest,
linux-arm-kernel, kvmarm, K Prateek Nayak, David Matlack
In-Reply-To: <20250522235223.3178519-1-seanjc@google.com>
Now that the eventfd's waitqueue ensures it has at most one priority
waiter, i.e. prevents KVM from binding multiple irqfds to one eventfd,
drop KVM's sanity check that eventfds are unique for a single VM.
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
virt/kvm/eventfd.c | 11 -----------
1 file changed, 11 deletions(-)
diff --git a/virt/kvm/eventfd.c b/virt/kvm/eventfd.c
index 7b2e1f858f6d..d5258fd16033 100644
--- a/virt/kvm/eventfd.c
+++ b/virt/kvm/eventfd.c
@@ -288,7 +288,6 @@ static void kvm_irqfd_register(struct file *file, wait_queue_head_t *wqh,
{
struct kvm_irqfd_pt *p = container_of(pt, struct kvm_irqfd_pt, pt);
struct kvm_kernel_irqfd *irqfd = p->irqfd;
- struct kvm_kernel_irqfd *tmp;
struct kvm *kvm = p->kvm;
/*
@@ -328,16 +327,6 @@ static void kvm_irqfd_register(struct file *file, wait_queue_head_t *wqh,
if (p->ret)
goto out;
- list_for_each_entry(tmp, &kvm->irqfds.items, list) {
- if (irqfd->eventfd != tmp->eventfd)
- continue;
-
- WARN_ON_ONCE(1);
- /* This fd is used for another irq already. */
- p->ret = -EBUSY;
- goto out;
- }
-
list_add_tail(&irqfd->list, &kvm->irqfds.items);
out:
--
2.49.0.1151.ga128411c76-goog
^ permalink raw reply related
* [PATCH v3 09/13] KVM: Disallow binding multiple irqfds to an eventfd with a priority waiter
From: Sean Christopherson @ 2025-05-22 23:52 UTC (permalink / raw)
To: K. Y. Srinivasan, Haiyang Zhang, Wei Liu, Dexuan Cui,
Juergen Gross, Stefano Stabellini, Paolo Bonzini, Ingo Molnar,
Peter Zijlstra, Juri Lelli, Vincent Guittot, Shuah Khan,
Marc Zyngier, Oliver Upton, Sean Christopherson
Cc: linux-kernel, linux-hyperv, xen-devel, kvm, linux-kselftest,
linux-arm-kernel, kvmarm, K Prateek Nayak, David Matlack
In-Reply-To: <20250522235223.3178519-1-seanjc@google.com>
Disallow binding an irqfd to an eventfd that already has a priority waiter,
i.e. to an eventfd that already has an attached irqfd. KVM always
operates in exclusive mode for EPOLL_IN (unconditionally returns '1'),
i.e. only the first waiter will be notified.
KVM already disallows binding multiple irqfds to an eventfd in a single
VM, but doesn't guard against multiple VMs binding to an eventfd. Adding
the extra protection reduces the pain of a userspace VMM bug, e.g. if
userspace fails to de-assign before re-assigning when transferring state
for intra-host migration, then the migration will explicitly fail as
opposed to dropping IRQs on the destination VM.
Temporarily keep KVM's manual check on irqfds.items, but add a WARN, e.g.
to allow sanity checking the waitqueue enforcement.
Cc: Oliver Upton <oliver.upton@linux.dev>
Cc: David Matlack <dmatlack@google.com>
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
virt/kvm/eventfd.c | 55 +++++++++++++++++++++++++++++++---------------
1 file changed, 37 insertions(+), 18 deletions(-)
diff --git a/virt/kvm/eventfd.c b/virt/kvm/eventfd.c
index c7969904637a..7b2e1f858f6d 100644
--- a/virt/kvm/eventfd.c
+++ b/virt/kvm/eventfd.c
@@ -291,38 +291,57 @@ static void kvm_irqfd_register(struct file *file, wait_queue_head_t *wqh,
struct kvm_kernel_irqfd *tmp;
struct kvm *kvm = p->kvm;
+ /*
+ * Note, irqfds.lock protects the irqfd's irq_entry, i.e. its routing,
+ * and irqfds.items. It does NOT protect registering with the eventfd.
+ */
spin_lock_irq(&kvm->irqfds.lock);
- list_for_each_entry(tmp, &kvm->irqfds.items, list) {
- if (irqfd->eventfd != tmp->eventfd)
- continue;
- /* This fd is used for another irq already. */
- p->ret = -EBUSY;
- spin_unlock_irq(&kvm->irqfds.lock);
- return;
- }
-
+ /*
+ * Initialize the routing information prior to adding the irqfd to the
+ * eventfd's waitqueue, as irqfd_wakeup() can be invoked as soon as the
+ * irqfd is registered.
+ */
irqfd_update(kvm, irqfd);
- list_add_tail(&irqfd->list, &kvm->irqfds.items);
-
/*
* Add the irqfd as a priority waiter on the eventfd, with a custom
* wake-up handler, so that KVM *and only KVM* is notified whenever the
- * underlying eventfd is signaled. Temporarily lie to lockdep about
- * holding irqfds.lock to avoid a false positive regarding potential
- * deadlock with irqfd_wakeup() (see irqfd_wakeup() for details).
+ * underlying eventfd is signaled.
*/
init_waitqueue_func_entry(&irqfd->wait, irqfd_wakeup);
+ /*
+ * Temporarily lie to lockdep about holding irqfds.lock to avoid a
+ * false positive regarding potential deadlock with irqfd_wakeup()
+ * (see irqfd_wakeup() for details).
+ *
+ * Adding to the wait queue will fail if there is already a priority
+ * waiter, i.e. if the eventfd is associated with another irqfd (in any
+ * VM). Note, kvm_irqfd_deassign() waits for all in-flight shutdown
+ * jobs to complete, i.e. ensures the irqfd has been removed from the
+ * eventfd's waitqueue before returning to userspace.
+ */
spin_release(&kvm->irqfds.lock.dep_map, _RET_IP_);
- irqfd->wait.flags |= WQ_FLAG_EXCLUSIVE;
- add_wait_queue_priority(wqh, &irqfd->wait);
+ p->ret = add_wait_queue_priority_exclusive(wqh, &irqfd->wait);
spin_acquire(&kvm->irqfds.lock.dep_map, 0, 0, _RET_IP_);
+ if (p->ret)
+ goto out;
+ list_for_each_entry(tmp, &kvm->irqfds.items, list) {
+ if (irqfd->eventfd != tmp->eventfd)
+ continue;
+
+ WARN_ON_ONCE(1);
+ /* This fd is used for another irq already. */
+ p->ret = -EBUSY;
+ goto out;
+ }
+
+ list_add_tail(&irqfd->list, &kvm->irqfds.items);
+
+out:
spin_unlock_irq(&kvm->irqfds.lock);
-
- p->ret = 0;
}
#if IS_ENABLED(CONFIG_HAVE_KVM_IRQ_BYPASS)
--
2.49.0.1151.ga128411c76-goog
^ permalink raw reply related
* [PATCH v3 08/13] sched/wait: Add a waitqueue helper for fully exclusive priority waiters
From: Sean Christopherson @ 2025-05-22 23:52 UTC (permalink / raw)
To: K. Y. Srinivasan, Haiyang Zhang, Wei Liu, Dexuan Cui,
Juergen Gross, Stefano Stabellini, Paolo Bonzini, Ingo Molnar,
Peter Zijlstra, Juri Lelli, Vincent Guittot, Shuah Khan,
Marc Zyngier, Oliver Upton, Sean Christopherson
Cc: linux-kernel, linux-hyperv, xen-devel, kvm, linux-kselftest,
linux-arm-kernel, kvmarm, K Prateek Nayak, David Matlack
In-Reply-To: <20250522235223.3178519-1-seanjc@google.com>
Add a waitqueue helper to add a priority waiter that requires exclusive
wakeups, i.e. that requires that it be the _only_ priority waiter. The
API will be used by KVM to ensure that at most one of KVM's irqfds is
bound to a single eventfd (across the entire kernel).
Open code the helper instead of using __add_wait_queue() so that the
common path doesn't need to "handle" impossible failures.
Cc: K Prateek Nayak <kprateek.nayak@amd.com>
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
include/linux/wait.h | 2 ++
kernel/sched/wait.c | 18 ++++++++++++++++++
2 files changed, 20 insertions(+)
diff --git a/include/linux/wait.h b/include/linux/wait.h
index 965a19809c7e..09855d819418 100644
--- a/include/linux/wait.h
+++ b/include/linux/wait.h
@@ -164,6 +164,8 @@ static inline bool wq_has_sleeper(struct wait_queue_head *wq_head)
extern void add_wait_queue(struct wait_queue_head *wq_head, struct wait_queue_entry *wq_entry);
extern void add_wait_queue_exclusive(struct wait_queue_head *wq_head, struct wait_queue_entry *wq_entry);
extern void add_wait_queue_priority(struct wait_queue_head *wq_head, struct wait_queue_entry *wq_entry);
+extern int add_wait_queue_priority_exclusive(struct wait_queue_head *wq_head,
+ struct wait_queue_entry *wq_entry);
extern void remove_wait_queue(struct wait_queue_head *wq_head, struct wait_queue_entry *wq_entry);
static inline void __add_wait_queue(struct wait_queue_head *wq_head, struct wait_queue_entry *wq_entry)
diff --git a/kernel/sched/wait.c b/kernel/sched/wait.c
index 4ab3ab195277..15632c89c4f2 100644
--- a/kernel/sched/wait.c
+++ b/kernel/sched/wait.c
@@ -47,6 +47,24 @@ void add_wait_queue_priority(struct wait_queue_head *wq_head, struct wait_queue_
}
EXPORT_SYMBOL_GPL(add_wait_queue_priority);
+int add_wait_queue_priority_exclusive(struct wait_queue_head *wq_head,
+ struct wait_queue_entry *wq_entry)
+{
+ struct list_head *head = &wq_head->head;
+
+ wq_entry->flags |= WQ_FLAG_EXCLUSIVE | WQ_FLAG_PRIORITY;
+
+ guard(spinlock_irqsave)(&wq_head->lock);
+
+ if (!list_empty(head) &&
+ (list_first_entry(head, typeof(*wq_entry), entry)->flags & WQ_FLAG_PRIORITY))
+ return -EBUSY;
+
+ list_add(&wq_entry->entry, head);
+ return 0;
+}
+EXPORT_SYMBOL_GPL(add_wait_queue_priority_exclusive);
+
void remove_wait_queue(struct wait_queue_head *wq_head, struct wait_queue_entry *wq_entry)
{
unsigned long flags;
--
2.49.0.1151.ga128411c76-goog
^ permalink raw reply related
* [PATCH v3 07/13] xen: privcmd: Don't mark eventfd waiter as EXCLUSIVE
From: Sean Christopherson @ 2025-05-22 23:52 UTC (permalink / raw)
To: K. Y. Srinivasan, Haiyang Zhang, Wei Liu, Dexuan Cui,
Juergen Gross, Stefano Stabellini, Paolo Bonzini, Ingo Molnar,
Peter Zijlstra, Juri Lelli, Vincent Guittot, Shuah Khan,
Marc Zyngier, Oliver Upton, Sean Christopherson
Cc: linux-kernel, linux-hyperv, xen-devel, kvm, linux-kselftest,
linux-arm-kernel, kvmarm, K Prateek Nayak, David Matlack
In-Reply-To: <20250522235223.3178519-1-seanjc@google.com>
Don't set WQ_FLAG_EXCLUSIVE when adding an irqfd to a wait queue, as
irqfd_wakeup() unconditionally returns '0', i.e. doesn't actually operate
in exclusive mode.
Note, the use of WQ_FLAG_PRIORITY is also dubious, but that's a problem
for another day.
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
drivers/xen/privcmd.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/xen/privcmd.c b/drivers/xen/privcmd.c
index c08ec8a7d27c..13a10f3294a8 100644
--- a/drivers/xen/privcmd.c
+++ b/drivers/xen/privcmd.c
@@ -957,7 +957,6 @@ irqfd_poll_func(struct file *file, wait_queue_head_t *wqh, poll_table *pt)
struct privcmd_kernel_irqfd *kirqfd =
container_of(pt, struct privcmd_kernel_irqfd, pt);
- kirqfd->wait.flags |= WQ_FLAG_EXCLUSIVE;
add_wait_queue_priority(wqh, &kirqfd->wait);
}
--
2.49.0.1151.ga128411c76-goog
^ permalink raw reply related
* [PATCH v3 06/13] sched/wait: Drop WQ_FLAG_EXCLUSIVE from add_wait_queue_priority()
From: Sean Christopherson @ 2025-05-22 23:52 UTC (permalink / raw)
To: K. Y. Srinivasan, Haiyang Zhang, Wei Liu, Dexuan Cui,
Juergen Gross, Stefano Stabellini, Paolo Bonzini, Ingo Molnar,
Peter Zijlstra, Juri Lelli, Vincent Guittot, Shuah Khan,
Marc Zyngier, Oliver Upton, Sean Christopherson
Cc: linux-kernel, linux-hyperv, xen-devel, kvm, linux-kselftest,
linux-arm-kernel, kvmarm, K Prateek Nayak, David Matlack
In-Reply-To: <20250522235223.3178519-1-seanjc@google.com>
Drop the setting of WQ_FLAG_EXCLUSIVE from add_wait_queue_priority() and
instead have callers manually add the flag prior to adding their structure
to the queue. Blindly setting WQ_FLAG_EXCLUSIVE is flawed, as the nature
of exclusive, priority waiters means that only the first waiter added will
ever receive notifications.
Pushing the flawed behavior to callers will allow fixing the problem one
hypervisor at a time (KVM added the flawed API, and then KVM's code was
copy+pasted nearly verbatim by Xen and Hyper-V), and will also allow for
adding an API that provides true exclusivity, i.e. that guarantees at most
one priority waiter is in the queue.
Opportunistically add a comment in Hyper-V to call out the mess. Xen
privcmd's irqfd_wakefup() doesn't actually operate in exclusive mode, i.e.
can be "fixed" simply by dropping WQ_FLAG_EXCLUSIVE. And KVM is primed to
switch to the aforementioned fully exclusive API, i.e. won't be carrying
the flawed code for long.
No functional change intended.
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
drivers/hv/mshv_eventfd.c | 8 ++++++++
drivers/xen/privcmd.c | 1 +
kernel/sched/wait.c | 4 ++--
virt/kvm/eventfd.c | 1 +
4 files changed, 12 insertions(+), 2 deletions(-)
diff --git a/drivers/hv/mshv_eventfd.c b/drivers/hv/mshv_eventfd.c
index 8dd22be2ca0b..b348928871c2 100644
--- a/drivers/hv/mshv_eventfd.c
+++ b/drivers/hv/mshv_eventfd.c
@@ -368,6 +368,14 @@ static void mshv_irqfd_queue_proc(struct file *file, wait_queue_head_t *wqh,
container_of(polltbl, struct mshv_irqfd, irqfd_polltbl);
irqfd->irqfd_wqh = wqh;
+
+ /*
+ * TODO: Ensure there isn't already an exclusive, priority waiter, e.g.
+ * that the irqfd isn't already bound to another partition. Only the
+ * first exclusive waiter encountered will be notified, and
+ * add_wait_queue_priority() doesn't enforce exclusivity.
+ */
+ irqfd->irqfd_wait.flags |= WQ_FLAG_EXCLUSIVE;
add_wait_queue_priority(wqh, &irqfd->irqfd_wait);
}
diff --git a/drivers/xen/privcmd.c b/drivers/xen/privcmd.c
index 13a10f3294a8..c08ec8a7d27c 100644
--- a/drivers/xen/privcmd.c
+++ b/drivers/xen/privcmd.c
@@ -957,6 +957,7 @@ irqfd_poll_func(struct file *file, wait_queue_head_t *wqh, poll_table *pt)
struct privcmd_kernel_irqfd *kirqfd =
container_of(pt, struct privcmd_kernel_irqfd, pt);
+ kirqfd->wait.flags |= WQ_FLAG_EXCLUSIVE;
add_wait_queue_priority(wqh, &kirqfd->wait);
}
diff --git a/kernel/sched/wait.c b/kernel/sched/wait.c
index 51e38f5f4701..4ab3ab195277 100644
--- a/kernel/sched/wait.c
+++ b/kernel/sched/wait.c
@@ -40,7 +40,7 @@ void add_wait_queue_priority(struct wait_queue_head *wq_head, struct wait_queue_
{
unsigned long flags;
- wq_entry->flags |= WQ_FLAG_EXCLUSIVE | WQ_FLAG_PRIORITY;
+ wq_entry->flags |= WQ_FLAG_PRIORITY;
spin_lock_irqsave(&wq_head->lock, flags);
__add_wait_queue(wq_head, wq_entry);
spin_unlock_irqrestore(&wq_head->lock, flags);
@@ -64,7 +64,7 @@ EXPORT_SYMBOL(remove_wait_queue);
* the non-exclusive tasks. Normally, exclusive tasks will be at the end of
* the list and any non-exclusive tasks will be woken first. A priority task
* may be at the head of the list, and can consume the event without any other
- * tasks being woken.
+ * tasks being woken if it's also an exclusive task.
*
* There are circumstances in which we can try to wake a task which has already
* started to run but is not in state TASK_RUNNING. try_to_wake_up() returns
diff --git a/virt/kvm/eventfd.c b/virt/kvm/eventfd.c
index 04877b297267..c7969904637a 100644
--- a/virt/kvm/eventfd.c
+++ b/virt/kvm/eventfd.c
@@ -316,6 +316,7 @@ static void kvm_irqfd_register(struct file *file, wait_queue_head_t *wqh,
init_waitqueue_func_entry(&irqfd->wait, irqfd_wakeup);
spin_release(&kvm->irqfds.lock.dep_map, _RET_IP_);
+ irqfd->wait.flags |= WQ_FLAG_EXCLUSIVE;
add_wait_queue_priority(wqh, &irqfd->wait);
spin_acquire(&kvm->irqfds.lock.dep_map, 0, 0, _RET_IP_);
--
2.49.0.1151.ga128411c76-goog
^ permalink raw reply related
* [PATCH v3 05/13] KVM: Add irqfd to eventfd's waitqueue while holding irqfds.lock
From: Sean Christopherson @ 2025-05-22 23:52 UTC (permalink / raw)
To: K. Y. Srinivasan, Haiyang Zhang, Wei Liu, Dexuan Cui,
Juergen Gross, Stefano Stabellini, Paolo Bonzini, Ingo Molnar,
Peter Zijlstra, Juri Lelli, Vincent Guittot, Shuah Khan,
Marc Zyngier, Oliver Upton, Sean Christopherson
Cc: linux-kernel, linux-hyperv, xen-devel, kvm, linux-kselftest,
linux-arm-kernel, kvmarm, K Prateek Nayak, David Matlack
In-Reply-To: <20250522235223.3178519-1-seanjc@google.com>
Add an irqfd to its target eventfd's waitqueue while holding irqfds.lock,
which is mildly terrifying but functionally safe. irqfds.lock is taken
inside the waitqueue's lock, but if and only if the eventfd is being
released, i.e. that path is mutually exclusive with registration as KVM
holds a reference to the eventfd (and obviously must do so to avoid UAF).
This will allow using the eventfd's waitqueue to enforce KVM's requirement
that eventfd is assigned to at most one irqfd, without introducing races.
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
virt/kvm/eventfd.c | 21 ++++++++++++++++++---
1 file changed, 18 insertions(+), 3 deletions(-)
diff --git a/virt/kvm/eventfd.c b/virt/kvm/eventfd.c
index 99274d60335d..04877b297267 100644
--- a/virt/kvm/eventfd.c
+++ b/virt/kvm/eventfd.c
@@ -204,6 +204,11 @@ irqfd_wakeup(wait_queue_entry_t *wait, unsigned mode, int sync, void *key)
int ret = 0;
if (flags & EPOLLIN) {
+ /*
+ * WARNING: Do NOT take irqfds.lock in any path except EPOLLHUP,
+ * as KVM holds irqfds.lock when registering the irqfd with the
+ * eventfd.
+ */
u64 cnt;
eventfd_ctx_do_read(irqfd->eventfd, &cnt);
@@ -225,6 +230,11 @@ irqfd_wakeup(wait_queue_entry_t *wait, unsigned mode, int sync, void *key)
/* The eventfd is closing, detach from KVM */
unsigned long iflags;
+ /*
+ * Taking irqfds.lock is safe here, as KVM holds a reference to
+ * the eventfd when registering the irqfd, i.e. this path can't
+ * be reached while kvm_irqfd_add() is running.
+ */
spin_lock_irqsave(&kvm->irqfds.lock, iflags);
/*
@@ -296,16 +306,21 @@ static void kvm_irqfd_register(struct file *file, wait_queue_head_t *wqh,
list_add_tail(&irqfd->list, &kvm->irqfds.items);
- spin_unlock_irq(&kvm->irqfds.lock);
-
/*
* Add the irqfd as a priority waiter on the eventfd, with a custom
* wake-up handler, so that KVM *and only KVM* is notified whenever the
- * underlying eventfd is signaled.
+ * underlying eventfd is signaled. Temporarily lie to lockdep about
+ * holding irqfds.lock to avoid a false positive regarding potential
+ * deadlock with irqfd_wakeup() (see irqfd_wakeup() for details).
*/
init_waitqueue_func_entry(&irqfd->wait, irqfd_wakeup);
+ spin_release(&kvm->irqfds.lock.dep_map, _RET_IP_);
add_wait_queue_priority(wqh, &irqfd->wait);
+ spin_acquire(&kvm->irqfds.lock.dep_map, 0, 0, _RET_IP_);
+
+ spin_unlock_irq(&kvm->irqfds.lock);
+
p->ret = 0;
}
--
2.49.0.1151.ga128411c76-goog
^ permalink raw reply related
* [PATCH v3 04/13] KVM: Add irqfd to KVM's list via the vfs_poll() callback
From: Sean Christopherson @ 2025-05-22 23:52 UTC (permalink / raw)
To: K. Y. Srinivasan, Haiyang Zhang, Wei Liu, Dexuan Cui,
Juergen Gross, Stefano Stabellini, Paolo Bonzini, Ingo Molnar,
Peter Zijlstra, Juri Lelli, Vincent Guittot, Shuah Khan,
Marc Zyngier, Oliver Upton, Sean Christopherson
Cc: linux-kernel, linux-hyperv, xen-devel, kvm, linux-kselftest,
linux-arm-kernel, kvmarm, K Prateek Nayak, David Matlack
In-Reply-To: <20250522235223.3178519-1-seanjc@google.com>
Add the irqfd structure to KVM's list of irqfds in kvm_irqfd_register(),
i.e. via the vfs_poll() callback. This will allow taking irqfds.lock
across the entire registration sequence (add to waitqueue, add to list),
and more importantly will allow inserting into KVM's list if and only if
adding to the waitqueue succeeds (spoiler alert), without needing to
juggle return codes in weird ways.
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
virt/kvm/eventfd.c | 102 +++++++++++++++++++++++++--------------------
1 file changed, 57 insertions(+), 45 deletions(-)
diff --git a/virt/kvm/eventfd.c b/virt/kvm/eventfd.c
index 8b9a87daa2bb..99274d60335d 100644
--- a/virt/kvm/eventfd.c
+++ b/virt/kvm/eventfd.c
@@ -245,34 +245,14 @@ irqfd_wakeup(wait_queue_entry_t *wait, unsigned mode, int sync, void *key)
return ret;
}
-struct kvm_irqfd_pt {
- struct kvm_kernel_irqfd *irqfd;
- poll_table pt;
-};
-
-static void kvm_irqfd_register(struct file *file, wait_queue_head_t *wqh,
- poll_table *pt)
-{
- struct kvm_irqfd_pt *p = container_of(pt, struct kvm_irqfd_pt, pt);
- struct kvm_kernel_irqfd *irqfd = p->irqfd;
-
- /*
- * Add the irqfd as a priority waiter on the eventfd, with a custom
- * wake-up handler, so that KVM *and only KVM* is notified whenever the
- * underlying eventfd is signaled.
- */
- init_waitqueue_func_entry(&irqfd->wait, irqfd_wakeup);
-
- add_wait_queue_priority(wqh, &irqfd->wait);
-}
-
-/* Must be called under irqfds.lock */
static void irqfd_update(struct kvm *kvm, struct kvm_kernel_irqfd *irqfd)
{
struct kvm_kernel_irq_routing_entry *e;
struct kvm_kernel_irq_routing_entry entries[KVM_NR_IRQCHIPS];
int n_entries;
+ lockdep_assert_held(&kvm->irqfds.lock);
+
n_entries = kvm_irq_map_gsi(kvm, entries, irqfd->gsi);
write_seqcount_begin(&irqfd->irq_entry_sc);
@@ -286,6 +266,49 @@ static void irqfd_update(struct kvm *kvm, struct kvm_kernel_irqfd *irqfd)
write_seqcount_end(&irqfd->irq_entry_sc);
}
+struct kvm_irqfd_pt {
+ struct kvm_kernel_irqfd *irqfd;
+ struct kvm *kvm;
+ poll_table pt;
+ int ret;
+};
+
+static void kvm_irqfd_register(struct file *file, wait_queue_head_t *wqh,
+ poll_table *pt)
+{
+ struct kvm_irqfd_pt *p = container_of(pt, struct kvm_irqfd_pt, pt);
+ struct kvm_kernel_irqfd *irqfd = p->irqfd;
+ struct kvm_kernel_irqfd *tmp;
+ struct kvm *kvm = p->kvm;
+
+ spin_lock_irq(&kvm->irqfds.lock);
+
+ list_for_each_entry(tmp, &kvm->irqfds.items, list) {
+ if (irqfd->eventfd != tmp->eventfd)
+ continue;
+ /* This fd is used for another irq already. */
+ p->ret = -EBUSY;
+ spin_unlock_irq(&kvm->irqfds.lock);
+ return;
+ }
+
+ irqfd_update(kvm, irqfd);
+
+ list_add_tail(&irqfd->list, &kvm->irqfds.items);
+
+ spin_unlock_irq(&kvm->irqfds.lock);
+
+ /*
+ * Add the irqfd as a priority waiter on the eventfd, with a custom
+ * wake-up handler, so that KVM *and only KVM* is notified whenever the
+ * underlying eventfd is signaled.
+ */
+ init_waitqueue_func_entry(&irqfd->wait, irqfd_wakeup);
+
+ add_wait_queue_priority(wqh, &irqfd->wait);
+ p->ret = 0;
+}
+
#if IS_ENABLED(CONFIG_HAVE_KVM_IRQ_BYPASS)
void __attribute__((weak)) kvm_arch_irq_bypass_stop(
struct irq_bypass_consumer *cons)
@@ -315,7 +338,7 @@ bool __attribute__((weak)) kvm_arch_irqfd_route_changed(
static int
kvm_irqfd_assign(struct kvm *kvm, struct kvm_irqfd *args)
{
- struct kvm_kernel_irqfd *irqfd, *tmp;
+ struct kvm_kernel_irqfd *irqfd;
struct eventfd_ctx *eventfd = NULL, *resamplefd = NULL;
struct kvm_irqfd_pt irqfd_pt;
int ret;
@@ -414,32 +437,22 @@ kvm_irqfd_assign(struct kvm *kvm, struct kvm_irqfd *args)
*/
idx = srcu_read_lock(&kvm->irq_srcu);
- spin_lock_irq(&kvm->irqfds.lock);
-
- ret = 0;
- list_for_each_entry(tmp, &kvm->irqfds.items, list) {
- if (irqfd->eventfd != tmp->eventfd)
- continue;
- /* This fd is used for another irq already. */
- ret = -EBUSY;
- goto fail_duplicate;
- }
-
- irqfd_update(kvm, irqfd);
-
- list_add_tail(&irqfd->list, &kvm->irqfds.items);
-
- spin_unlock_irq(&kvm->irqfds.lock);
-
/*
- * Register the irqfd with the eventfd by polling on the eventfd. If
- * there was en event pending on the eventfd prior to registering,
- * manually trigger IRQ injection.
+ * Register the irqfd with the eventfd by polling on the eventfd, and
+ * simultaneously and the irqfd to KVM's list. If there was en event
+ * pending on the eventfd prior to registering, manually trigger IRQ
+ * injection.
*/
irqfd_pt.irqfd = irqfd;
+ irqfd_pt.kvm = kvm;
init_poll_funcptr(&irqfd_pt.pt, kvm_irqfd_register);
events = vfs_poll(fd_file(f), &irqfd_pt.pt);
+
+ ret = irqfd_pt.ret;
+ if (ret)
+ goto fail_poll;
+
if (events & EPOLLIN)
schedule_work(&irqfd->inject);
@@ -460,8 +473,7 @@ kvm_irqfd_assign(struct kvm *kvm, struct kvm_irqfd *args)
srcu_read_unlock(&kvm->irq_srcu, idx);
return 0;
-fail_duplicate:
- spin_unlock_irq(&kvm->irqfds.lock);
+fail_poll:
srcu_read_unlock(&kvm->irq_srcu, idx);
fail:
if (irqfd->resampler)
--
2.49.0.1151.ga128411c76-goog
^ permalink raw reply related
* [PATCH v3 03/13] KVM: Initialize irqfd waitqueue callback when adding to the queue
From: Sean Christopherson @ 2025-05-22 23:52 UTC (permalink / raw)
To: K. Y. Srinivasan, Haiyang Zhang, Wei Liu, Dexuan Cui,
Juergen Gross, Stefano Stabellini, Paolo Bonzini, Ingo Molnar,
Peter Zijlstra, Juri Lelli, Vincent Guittot, Shuah Khan,
Marc Zyngier, Oliver Upton, Sean Christopherson
Cc: linux-kernel, linux-hyperv, xen-devel, kvm, linux-kselftest,
linux-arm-kernel, kvmarm, K Prateek Nayak, David Matlack
In-Reply-To: <20250522235223.3178519-1-seanjc@google.com>
Initialize the irqfd waitqueue callback immediately prior to inserting the
irqfd into the eventfd's waitqueue. Pre-initializing the state in a
completely different context is all kinds of confusing, and incorrectly
suggests that the waitqueue function needs to be initialize prior to
vfs_poll().
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
virt/kvm/eventfd.c | 13 +++++++------
1 file changed, 7 insertions(+), 6 deletions(-)
diff --git a/virt/kvm/eventfd.c b/virt/kvm/eventfd.c
index 42c02c35e542..8b9a87daa2bb 100644
--- a/virt/kvm/eventfd.c
+++ b/virt/kvm/eventfd.c
@@ -256,6 +256,13 @@ static void kvm_irqfd_register(struct file *file, wait_queue_head_t *wqh,
struct kvm_irqfd_pt *p = container_of(pt, struct kvm_irqfd_pt, pt);
struct kvm_kernel_irqfd *irqfd = p->irqfd;
+ /*
+ * Add the irqfd as a priority waiter on the eventfd, with a custom
+ * wake-up handler, so that KVM *and only KVM* is notified whenever the
+ * underlying eventfd is signaled.
+ */
+ init_waitqueue_func_entry(&irqfd->wait, irqfd_wakeup);
+
add_wait_queue_priority(wqh, &irqfd->wait);
}
@@ -395,12 +402,6 @@ kvm_irqfd_assign(struct kvm *kvm, struct kvm_irqfd *args)
mutex_unlock(&kvm->irqfds.resampler_lock);
}
- /*
- * Install our own custom wake-up handling so we are notified via
- * a callback whenever someone signals the underlying eventfd
- */
- init_waitqueue_func_entry(&irqfd->wait, irqfd_wakeup);
-
/*
* Set the irqfd routing and add it to KVM's list before registering
* the irqfd with the eventfd, so that the routing information is valid
--
2.49.0.1151.ga128411c76-goog
^ permalink raw reply related
* [PATCH v3 02/13] KVM: Acquire SCRU lock outside of irqfds.lock during assignment
From: Sean Christopherson @ 2025-05-22 23:52 UTC (permalink / raw)
To: K. Y. Srinivasan, Haiyang Zhang, Wei Liu, Dexuan Cui,
Juergen Gross, Stefano Stabellini, Paolo Bonzini, Ingo Molnar,
Peter Zijlstra, Juri Lelli, Vincent Guittot, Shuah Khan,
Marc Zyngier, Oliver Upton, Sean Christopherson
Cc: linux-kernel, linux-hyperv, xen-devel, kvm, linux-kselftest,
linux-arm-kernel, kvmarm, K Prateek Nayak, David Matlack
In-Reply-To: <20250522235223.3178519-1-seanjc@google.com>
Acquire SRCU outside of irqfds.lock so that the locking is symmetrical,
and add a comment explaining why on earth KVM holds SRCU for so long.
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
virt/kvm/eventfd.c | 19 ++++++++++++++++---
1 file changed, 16 insertions(+), 3 deletions(-)
diff --git a/virt/kvm/eventfd.c b/virt/kvm/eventfd.c
index 39e42b19d9f7..42c02c35e542 100644
--- a/virt/kvm/eventfd.c
+++ b/virt/kvm/eventfd.c
@@ -401,6 +401,18 @@ kvm_irqfd_assign(struct kvm *kvm, struct kvm_irqfd *args)
*/
init_waitqueue_func_entry(&irqfd->wait, irqfd_wakeup);
+ /*
+ * Set the irqfd routing and add it to KVM's list before registering
+ * the irqfd with the eventfd, so that the routing information is valid
+ * and stays valid, e.g. if there are GSI routing changes, prior to
+ * making the irqfd visible, i.e. before it might be signaled.
+ *
+ * Note, holding SRCU ensures a stable read of routing information, and
+ * also prevents irqfd_shutdown() from freeing the irqfd before it's
+ * fully initialized.
+ */
+ idx = srcu_read_lock(&kvm->irq_srcu);
+
spin_lock_irq(&kvm->irqfds.lock);
ret = 0;
@@ -409,11 +421,9 @@ kvm_irqfd_assign(struct kvm *kvm, struct kvm_irqfd *args)
continue;
/* This fd is used for another irq already. */
ret = -EBUSY;
- spin_unlock_irq(&kvm->irqfds.lock);
- goto fail;
+ goto fail_duplicate;
}
- idx = srcu_read_lock(&kvm->irq_srcu);
irqfd_update(kvm, irqfd);
list_add_tail(&irqfd->list, &kvm->irqfds.items);
@@ -449,6 +459,9 @@ kvm_irqfd_assign(struct kvm *kvm, struct kvm_irqfd *args)
srcu_read_unlock(&kvm->irq_srcu, idx);
return 0;
+fail_duplicate:
+ spin_unlock_irq(&kvm->irqfds.lock);
+ srcu_read_unlock(&kvm->irq_srcu, idx);
fail:
if (irqfd->resampler)
irqfd_resampler_shutdown(irqfd);
--
2.49.0.1151.ga128411c76-goog
^ permalink raw reply related
* [PATCH v3 01/13] KVM: Use a local struct to do the initial vfs_poll() on an irqfd
From: Sean Christopherson @ 2025-05-22 23:52 UTC (permalink / raw)
To: K. Y. Srinivasan, Haiyang Zhang, Wei Liu, Dexuan Cui,
Juergen Gross, Stefano Stabellini, Paolo Bonzini, Ingo Molnar,
Peter Zijlstra, Juri Lelli, Vincent Guittot, Shuah Khan,
Marc Zyngier, Oliver Upton, Sean Christopherson
Cc: linux-kernel, linux-hyperv, xen-devel, kvm, linux-kselftest,
linux-arm-kernel, kvmarm, K Prateek Nayak, David Matlack
In-Reply-To: <20250522235223.3178519-1-seanjc@google.com>
Use a function-local struct for the poll_table passed to vfs_poll(), as
nothing in the vfs_poll() callchain grabs a long-term reference to the
structure, i.e. its lifetime doesn't need to be tied to the irqfd. Using
a local structure will also allow propagating failures out of the polling
callback without further polluting kvm_kernel_irqfd.
Opportunstically rename irqfd_ptable_queue_proc() to kvm_irqfd_register()
to capture what it actually does.
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
include/linux/kvm_irqfd.h | 1 -
virt/kvm/eventfd.c | 26 +++++++++++++++++---------
2 files changed, 17 insertions(+), 10 deletions(-)
diff --git a/include/linux/kvm_irqfd.h b/include/linux/kvm_irqfd.h
index 8ad43692e3bb..44fd2a20b09e 100644
--- a/include/linux/kvm_irqfd.h
+++ b/include/linux/kvm_irqfd.h
@@ -55,7 +55,6 @@ struct kvm_kernel_irqfd {
/* Used for setup/shutdown */
struct eventfd_ctx *eventfd;
struct list_head list;
- poll_table pt;
struct work_struct shutdown;
struct irq_bypass_consumer consumer;
struct irq_bypass_producer *producer;
diff --git a/virt/kvm/eventfd.c b/virt/kvm/eventfd.c
index 11e5d1e3f12e..39e42b19d9f7 100644
--- a/virt/kvm/eventfd.c
+++ b/virt/kvm/eventfd.c
@@ -245,12 +245,17 @@ irqfd_wakeup(wait_queue_entry_t *wait, unsigned mode, int sync, void *key)
return ret;
}
-static void
-irqfd_ptable_queue_proc(struct file *file, wait_queue_head_t *wqh,
- poll_table *pt)
+struct kvm_irqfd_pt {
+ struct kvm_kernel_irqfd *irqfd;
+ poll_table pt;
+};
+
+static void kvm_irqfd_register(struct file *file, wait_queue_head_t *wqh,
+ poll_table *pt)
{
- struct kvm_kernel_irqfd *irqfd =
- container_of(pt, struct kvm_kernel_irqfd, pt);
+ struct kvm_irqfd_pt *p = container_of(pt, struct kvm_irqfd_pt, pt);
+ struct kvm_kernel_irqfd *irqfd = p->irqfd;
+
add_wait_queue_priority(wqh, &irqfd->wait);
}
@@ -305,6 +310,7 @@ kvm_irqfd_assign(struct kvm *kvm, struct kvm_irqfd *args)
{
struct kvm_kernel_irqfd *irqfd, *tmp;
struct eventfd_ctx *eventfd = NULL, *resamplefd = NULL;
+ struct kvm_irqfd_pt irqfd_pt;
int ret;
__poll_t events;
int idx;
@@ -394,7 +400,6 @@ kvm_irqfd_assign(struct kvm *kvm, struct kvm_irqfd *args)
* a callback whenever someone signals the underlying eventfd
*/
init_waitqueue_func_entry(&irqfd->wait, irqfd_wakeup);
- init_poll_funcptr(&irqfd->pt, irqfd_ptable_queue_proc);
spin_lock_irq(&kvm->irqfds.lock);
@@ -416,11 +421,14 @@ kvm_irqfd_assign(struct kvm *kvm, struct kvm_irqfd *args)
spin_unlock_irq(&kvm->irqfds.lock);
/*
- * Check if there was an event already pending on the eventfd
- * before we registered, and trigger it as if we didn't miss it.
+ * Register the irqfd with the eventfd by polling on the eventfd. If
+ * there was en event pending on the eventfd prior to registering,
+ * manually trigger IRQ injection.
*/
- events = vfs_poll(fd_file(f), &irqfd->pt);
+ irqfd_pt.irqfd = irqfd;
+ init_poll_funcptr(&irqfd_pt.pt, kvm_irqfd_register);
+ events = vfs_poll(fd_file(f), &irqfd_pt.pt);
if (events & EPOLLIN)
schedule_work(&irqfd->inject);
--
2.49.0.1151.ga128411c76-goog
^ permalink raw reply related
* [PATCH v3 00/13] KVM: Make irqfd registration globally unique
From: Sean Christopherson @ 2025-05-22 23:52 UTC (permalink / raw)
To: K. Y. Srinivasan, Haiyang Zhang, Wei Liu, Dexuan Cui,
Juergen Gross, Stefano Stabellini, Paolo Bonzini, Ingo Molnar,
Peter Zijlstra, Juri Lelli, Vincent Guittot, Shuah Khan,
Marc Zyngier, Oliver Upton, Sean Christopherson
Cc: linux-kernel, linux-hyperv, xen-devel, kvm, linux-kselftest,
linux-arm-kernel, kvmarm, K Prateek Nayak, David Matlack
Non-KVM folks,
I am hoping to route this through the KVM tree (6.17 or later), as the non-KVM
changes should be glorified nops. Please holler if you object to that idea.
Hyper-V folks in particular, let me know if you want a stable topic branch/tag,
e.g. on the off chance you want to make similar changes to the Hyper-V code,
and I'll make sure that happens.
As for what this series actually does...
Rework KVM's irqfd registration to require that an eventfd is bound to at
most one irqfd throughout the entire system. KVM currently disallows
binding an eventfd to multiple irqfds for a single VM, but doesn't reject
attempts to bind an eventfd to multiple VMs.
This is obviously an ABI change, but I'm fairly confident that it won't
break userspace, because binding an eventfd to multiple irqfds hasn't
truly worked since commit e8dbf19508a1 ("kvm/eventfd: Use priority waitqueue
to catch events before userspace"). A somewhat undocumented, and perhaps
even unintentional, side effect of suppressing eventfd notifications for
userspace is that the priority+exclusive behavior also suppresses eventfd
notifications for any subsequent waiters, even if they are priority waiters.
I.e. only the first VM with an irqfd+eventfd binding will get notifications.
And for IRQ bypass, a.k.a. device posted interrupts, globally unique
bindings are a hard requirement (at least on x86; I assume other archs are
the same). KVM and the IRQ bypass manager kinda sorta handle this, but in
the absolute worst way possible (IMO). Instead of surfacing an error to
userspace, KVM silently ignores IRQ bypass registration errors.
The motivation for this series is to harden against userspace goofs. AFAIK,
we (Google) have never actually had a bug where userspace tries to assign
an eventfd to multiple VMs, but the possibility has come up in more than one
bug investigation (our intra-host, a.k.a. copyless, migration scheme
transfers eventfds from the old to the new VM when updating the host VMM).
v3:
- Retain WQ_FLAG_EXCLUSIVE in mshv_eventfd.c, which snuck in between v1
and v2. [Peter]
- Use EXPORT_SYMBOL_GPL. [Peter]
- Move WQ_FLAG_EXCLUSIVE out of add_wait_queue_priority() in a prep patch
so that the affected subsystems are more explicitly documented (and then
immediately drop the flag from drivers/xen/privcmd.c, which amusingly
hides that file from the diff stats).
v2:
- https://lore.kernel.org/all/20250519185514.2678456-1-seanjc@google.com
- Use guard(spinlock_irqsave). [Prateek]
v1: https://lore.kernel.org/all/20250401204425.904001-1-seanjc@google.com
Sean Christopherson (13):
KVM: Use a local struct to do the initial vfs_poll() on an irqfd
KVM: Acquire SCRU lock outside of irqfds.lock during assignment
KVM: Initialize irqfd waitqueue callback when adding to the queue
KVM: Add irqfd to KVM's list via the vfs_poll() callback
KVM: Add irqfd to eventfd's waitqueue while holding irqfds.lock
sched/wait: Drop WQ_FLAG_EXCLUSIVE from add_wait_queue_priority()
xen: privcmd: Don't mark eventfd waiter as EXCLUSIVE
sched/wait: Add a waitqueue helper for fully exclusive priority
waiters
KVM: Disallow binding multiple irqfds to an eventfd with a priority
waiter
KVM: Drop sanity check that per-VM list of irqfds is unique
KVM: selftests: Assert that eventfd() succeeds in Xen shinfo test
KVM: selftests: Add utilities to create eventfds and do KVM_IRQFD
KVM: selftests: Add a KVM_IRQFD test to verify uniqueness requirements
drivers/hv/mshv_eventfd.c | 8 ++
include/linux/kvm_irqfd.h | 1 -
include/linux/wait.h | 2 +
kernel/sched/wait.c | 22 ++-
tools/testing/selftests/kvm/Makefile.kvm | 1 +
tools/testing/selftests/kvm/arm64/vgic_irq.c | 12 +-
.../testing/selftests/kvm/include/kvm_util.h | 40 ++++++
tools/testing/selftests/kvm/irqfd_test.c | 130 ++++++++++++++++++
.../selftests/kvm/x86/xen_shinfo_test.c | 21 +--
virt/kvm/eventfd.c | 130 +++++++++++++-----
10 files changed, 302 insertions(+), 65 deletions(-)
create mode 100644 tools/testing/selftests/kvm/irqfd_test.c
base-commit: 45eb29140e68ffe8e93a5471006858a018480a45
--
2.49.0.1151.ga128411c76-goog
^ permalink raw reply
* Re: [PATCH net,v2] hv_netvsc: fix potential deadlock in netvsc_vf_setxdp()
From: Jakub Kicinski @ 2025-05-22 22:13 UTC (permalink / raw)
To: Saurabh Sengar
Cc: kys, haiyangz, wei.liu, decui, andrew+netdev, davem, edumazet,
pabeni, horms, ast, daniel, hawk, john.fastabend, sdf, kuniyu,
ahmed.zaki, aleksander.lobakin, linux-hyperv, netdev,
linux-kernel, bpf, ssengar, stable
In-Reply-To: <1747823103-3420-1-git-send-email-ssengar@linux.microsoft.com>
On Wed, 21 May 2025 03:25:03 -0700 Saurabh Sengar wrote:
> The MANA driver's probe registers netdevice via the following call chain:
>
> mana_probe()
> register_netdev()
> register_netdevice()
>
> register_netdevice() calls notifier callback for netvsc driver,
> holding the netdev mutex via netdev_lock_ops().
>
> Further this netvsc notifier callback end up attempting to acquire the
> same lock again in dev_xdp_propagate() leading to deadlock.
>
> netvsc_netdev_event()
> netvsc_vf_setxdp()
> dev_xdp_propagate()
>
> This deadlock was not observed so far because net_shaper_ops was never set,
The lock is on the VF, I think you meant to say that no device you use
in Azure is ops locked?
There's also the call to netvsc_register_vf() on probe path, please
fix or explain why it doesn't need locking in the commit message.
--
pw-bot: cr
^ permalink raw reply
* Re: [PATCH v3 2/2] Drivers: hv: Introduce mshv_vtl driver
From: Nuno Das Neves @ 2025-05-22 19:15 UTC (permalink / raw)
To: Naman Jain, Stanislav Kinsburskii
Cc: K . Y . Srinivasan, Haiyang Zhang, Wei Liu, Dexuan Cui,
Roman Kisel, Anirudh Rayabharam, Saurabh Sengar, ALOK TIWARI,
linux-kernel, linux-hyperv
In-Reply-To: <3ab2597b-3a64-497c-888e-dd5d0c0e5076@linux.microsoft.com>
On 5/21/2025 2:32 AM, Naman Jain wrote:
>
>
> On 5/21/2025 11:33 AM, Naman Jain wrote:
>>
>>
>> On 5/21/2025 12:25 AM, Stanislav Kinsburskii wrote:
>>> 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?
>>>
>>
>> Thanks for reviewing. This config is required for below functions which
>> are used for mshv_vtl_low device.
>>
>> vm_fault_t mshv_vtl_low_huge_fault ->
>> * vmf_insert_pfn_pmd
>> * vmf_insert_pfn_pud
>>
>>
>>> <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.
>>>
>>
>> I'll take care of all of these in my next patch.
>>
>
>
> One concern about this change in include/uapi/linux/mshv.h.
> I see the convention of using '__' family of data types in this file.
> Whatever we do here, will be applicable to existing code as well.
> Do you suggest we should change it to u{32,64} variants or keep
> it in current form?
>
uapi code uses the '__' versions of these types, so they should stay
as they are.
Thanks
Nuno
> Regards,
> Naman
>
>>> <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
>>
>> None of the IOCTLs in mshv_vtl section, introduced in this patch is used
>> by mshv_root driver. Since IOCTLs of mshv_root does not have MSHV_ROOT
>> prefix, I am OK with removing MSHV_VTL_* prefix from these IOCTL names.
>> You can let me know if you want me to prefix them with MSHV_VTL.
>>
>> Thanks again for reviewing.
>>
>> Regards,
>> Naman
>>
>>>
>>>> #endif
>>>> --
>>>> 2.34.1
>>>>
>>
^ permalink raw reply
* Re: [PATCH v3 2/2] Drivers: hv: Introduce mshv_vtl driver
From: Stanislav Kinsburskii @ 2025-05-22 18:10 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: <80853cdb-fd34-4a5e-99a0-1a71b8ce8226@linux.microsoft.com>
On Wed, May 21, 2025 at 11:33:29AM +0530, Naman Jain wrote:
>
>
> On 5/21/2025 12:25 AM, Stanislav Kinsburskii wrote:
> > 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?
> >
>
Let me rephrase: can this driver work without transparent huge pages?
If yes, then it shouldn't depend on the option and rather select it.
If not, then it should be either fixed to be able to or have an
expalanation why this dependecy was introduced.
> > > +
> > > +/* 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
>
> None of the IOCTLs in mshv_vtl section, introduced in this patch is used
> by mshv_root driver. Since IOCTLs of mshv_root does not have MSHV_ROOT
> prefix, I am OK with removing MSHV_VTL_* prefix from these IOCTL names.
> You can let me know if you want me to prefix them with MSHV_VTL.
>
> Thanks again for reviewing.
>
> Regards,
> Naman
>
As these ioctls share the same "namespace" (MSHV_IOCTL), removal os the
VTL suffix looks a better optio to me.
Thanks,
Stanislav.
^ permalink raw reply
* RE: [EXTERNAL] Re: [PATCH net-next,v2] net: mana: Add support for Multi Vports on Bare metal
From: Haiyang Zhang @ 2025-05-22 14:51 UTC (permalink / raw)
To: Paolo Abeni, Simon Horman
Cc: linux-hyperv@vger.kernel.org, netdev@vger.kernel.org, Dexuan Cui,
stephen@networkplumber.org, KY Srinivasan, Paul Rosswurm,
olaf@aepfle.de, vkuznets@redhat.com, davem@davemloft.net,
wei.liu@kernel.org, edumazet@google.com, kuba@kernel.org,
leon@kernel.org, Long Li, ssengar@linux.microsoft.com,
linux-rdma@vger.kernel.org, daniel@iogearbox.net,
john.fastabend@gmail.com, bpf@vger.kernel.org, ast@kernel.org,
hawk@kernel.org, tglx@linutronix.de,
shradhagupta@linux.microsoft.com, andrew+netdev@lunn.ch,
Konstantin Taranov, linux-kernel@vger.kernel.org
In-Reply-To: <5470f2d2-d3cb-4451-b8e8-5ee768ed9741@redhat.com>
> -----Original Message-----
> From: Paolo Abeni <pabeni@redhat.com>
> Sent: Thursday, May 22, 2025 9:44 AM
> To: Simon Horman <horms@kernel.org>; Haiyang Zhang
> <haiyangz@microsoft.com>
> >>>> static int mana_query_device_cfg(struct mana_context *ac, u32
> >>> proto_major_ver,
> >>>> u32 proto_minor_ver, u32 proto_micro_ver,
> >>>> - u16 *max_num_vports)
> >>>> + u16 *max_num_vports, u8 *bm_hostmode)
> >>>> {
> >>>> struct gdma_context *gc = ac->gdma_dev->gdma_context;
> >>>> struct mana_query_device_cfg_resp resp = {};
> >>>> @@ -932,7 +932,7 @@ static int mana_query_device_cfg(struct
> mana_context
> >>> *ac, u32 proto_major_ver,
> >>>> mana_gd_init_req_hdr(&req.hdr, MANA_QUERY_DEV_CONFIG,
> >>>> sizeof(req), sizeof(resp));
> >>>>
> >>>> - req.hdr.resp.msg_version = GDMA_MESSAGE_V2;
> >>>> + req.hdr.resp.msg_version = GDMA_MESSAGE_V3;
> >>>>
> >>>> req.proto_major_ver = proto_major_ver;
> >>>> req.proto_minor_ver = proto_minor_ver;
> >>>
> >>>> @@ -956,11 +956,16 @@ static int mana_query_device_cfg(struct
> >>> mana_context *ac, u32 proto_major_ver,
> >>>>
> >>>> *max_num_vports = resp.max_num_vports;
> >>>>
> >>>> - if (resp.hdr.response.msg_version == GDMA_MESSAGE_V2)
> >>>> + if (resp.hdr.response.msg_version >= GDMA_MESSAGE_V2)
> >>>> gc->adapter_mtu = resp.adapter_mtu;
> >>>> else
> >>>> gc->adapter_mtu = ETH_FRAME_LEN;
> >>>>
> >>>> + if (resp.hdr.response.msg_version >= GDMA_MESSAGE_V3)
> >>>> + *bm_hostmode = resp.bm_hostmode;
> >>>> + else
> >>>> + *bm_hostmode = 0;
> >>>
> >>> Hi,
> >>>
> >>> Perhaps not strictly related to this patch, but I see
> >>> that mana_verify_resp_hdr() is called a few lines above.
> >>> And that verifies a minimum msg_version. But I do not see
> >>> any verification of the maximum msg_version supported by the code.
> >>>
> >>> I am concerned about a hypothetical scenario where, say the as yet
> unknown
> >>> version 5 is sent as the version, and the above behaviour is used,
> while
> >>> not being correct.
> >>>
> >>> Could you shed some light on this?
> >>>
> >>
> >> In driver, we specify the expected reply msg version is v3 here:
> >> req.hdr.resp.msg_version = GDMA_MESSAGE_V3;
> >>
> >> If the HW side is upgraded, it won't send reply msg version higher
> >> than expected, which may break the driver.
> >
> > Thanks,
> >
> > If I understand things correctly the HW side will honour the
> > req.hdr.resp.msg_version and thus the SW won't receive anything
> > it doesn't expect. Is that right?
>
> @Haiyang, if Simon's interpretation is correct, please change the
> version checking in the driver from:
>
> if (resp.hdr.response.msg_version >= GDMA_MESSAGE_V3)
>
> to
> if (resp.hdr.response.msg_version == GDMA_MESSAGE_V3)
>
> As the current code is misleading.
Simon:
Yes, you are right. So newer HW can support older driver, and vice
versa.
Paolo:
The MANA protocol doesn't remove any existing fields during upgrades.
So (resp.hdr.response.msg_version >= GDMA_MESSAGE_V3) will continue
to work in the future. If we change it to
(resp.hdr.response.msg_version == GDMA_MESSAGE_V3),
we will have to remember to update it to something like:
(resp.hdr.response.msg_version >= GDMA_MESSAGE_V3 &&
resp.hdr.response.msg_version <= GDMA_MESSAGE_V5),
if the version is upgraded to v5 in the future. And keep on updating
the checks on existing fields every time when the version is
upgraded.
So, can I keep the ">=" condition, to avoid future bug if anyone
forget to update checks on all existing fields?
Thanks,
- Haiyang
^ permalink raw reply
* Re: [EXTERNAL] Re: [PATCH net-next,v2] net: mana: Add support for Multi Vports on Bare metal
From: Paolo Abeni @ 2025-05-22 13:43 UTC (permalink / raw)
To: Simon Horman, Haiyang Zhang
Cc: linux-hyperv@vger.kernel.org, netdev@vger.kernel.org, Dexuan Cui,
stephen@networkplumber.org, KY Srinivasan, Paul Rosswurm,
olaf@aepfle.de, vkuznets@redhat.com, davem@davemloft.net,
wei.liu@kernel.org, edumazet@google.com, kuba@kernel.org,
leon@kernel.org, Long Li, ssengar@linux.microsoft.com,
linux-rdma@vger.kernel.org, daniel@iogearbox.net,
john.fastabend@gmail.com, bpf@vger.kernel.org, ast@kernel.org,
hawk@kernel.org, tglx@linutronix.de,
shradhagupta@linux.microsoft.com, andrew+netdev@lunn.ch,
Konstantin Taranov, linux-kernel@vger.kernel.org
In-Reply-To: <20250522120229.GX365796@horms.kernel.org>
On 5/22/25 2:02 PM, Simon Horman wrote:
> On Wed, May 21, 2025 at 05:28:33PM +0000, Haiyang Zhang wrote:
>>> -----Original Message-----
>>> From: Simon Horman <horms@kernel.org>
>>> Sent: Wednesday, May 21, 2025 10:03 AM
>>> To: Haiyang Zhang <haiyangz@microsoft.com>
>>> Cc: linux-hyperv@vger.kernel.org; netdev@vger.kernel.org; Dexuan Cui
>>> <decui@microsoft.com>; stephen@networkplumber.org; KY Srinivasan
>>> <kys@microsoft.com>; Paul Rosswurm <paulros@microsoft.com>;
>>> olaf@aepfle.de; vkuznets@redhat.com; davem@davemloft.net;
>>> wei.liu@kernel.org; edumazet@google.com; kuba@kernel.org;
>>> pabeni@redhat.com; leon@kernel.org; Long Li <longli@microsoft.com>;
>>> ssengar@linux.microsoft.com; linux-rdma@vger.kernel.org;
>>> daniel@iogearbox.net; john.fastabend@gmail.com; bpf@vger.kernel.org;
>>> ast@kernel.org; hawk@kernel.org; tglx@linutronix.de;
>>> shradhagupta@linux.microsoft.com; andrew+netdev@lunn.ch; Konstantin
>>> Taranov <kotaranov@microsoft.com>; linux-kernel@vger.kernel.org
>>> Subject: [EXTERNAL] Re: [PATCH net-next,v2] net: mana: Add support for
>>> Multi Vports on Bare metal
>>>
>>> On Mon, May 19, 2025 at 09:20:36AM -0700, Haiyang Zhang wrote:
>>>> To support Multi Vports on Bare metal, increase the device config
>>> response
>>>> version. And, skip the register HW vport, and register filter steps,
>>> when
>>>> the Bare metal hostmode is set.
>>>>
>>>> Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
>>>> ---
>>>> v2:
>>>> Updated comments as suggested by ALOK TIWARI.
>>>> Fixed the version check.
>>>>
>>>> ---
>>>> drivers/net/ethernet/microsoft/mana/mana_en.c | 24 ++++++++++++-------
>>>> include/net/mana/mana.h | 4 +++-
>>>> 2 files changed, 19 insertions(+), 9 deletions(-)
>>>>
>>>> diff --git a/drivers/net/ethernet/microsoft/mana/mana_en.c
>>> b/drivers/net/ethernet/microsoft/mana/mana_en.c
>>>> index 2bac6be8f6a0..9c58d9e0bbb5 100644
>>>> --- a/drivers/net/ethernet/microsoft/mana/mana_en.c
>>>> +++ b/drivers/net/ethernet/microsoft/mana/mana_en.c
>>>> @@ -921,7 +921,7 @@ static void mana_pf_deregister_filter(struct
>>> mana_port_context *apc)
>>>>
>>>> static int mana_query_device_cfg(struct mana_context *ac, u32
>>> proto_major_ver,
>>>> u32 proto_minor_ver, u32 proto_micro_ver,
>>>> - u16 *max_num_vports)
>>>> + u16 *max_num_vports, u8 *bm_hostmode)
>>>> {
>>>> struct gdma_context *gc = ac->gdma_dev->gdma_context;
>>>> struct mana_query_device_cfg_resp resp = {};
>>>> @@ -932,7 +932,7 @@ static int mana_query_device_cfg(struct mana_context
>>> *ac, u32 proto_major_ver,
>>>> mana_gd_init_req_hdr(&req.hdr, MANA_QUERY_DEV_CONFIG,
>>>> sizeof(req), sizeof(resp));
>>>>
>>>> - req.hdr.resp.msg_version = GDMA_MESSAGE_V2;
>>>> + req.hdr.resp.msg_version = GDMA_MESSAGE_V3;
>>>>
>>>> req.proto_major_ver = proto_major_ver;
>>>> req.proto_minor_ver = proto_minor_ver;
>>>
>>>> @@ -956,11 +956,16 @@ static int mana_query_device_cfg(struct
>>> mana_context *ac, u32 proto_major_ver,
>>>>
>>>> *max_num_vports = resp.max_num_vports;
>>>>
>>>> - if (resp.hdr.response.msg_version == GDMA_MESSAGE_V2)
>>>> + if (resp.hdr.response.msg_version >= GDMA_MESSAGE_V2)
>>>> gc->adapter_mtu = resp.adapter_mtu;
>>>> else
>>>> gc->adapter_mtu = ETH_FRAME_LEN;
>>>>
>>>> + if (resp.hdr.response.msg_version >= GDMA_MESSAGE_V3)
>>>> + *bm_hostmode = resp.bm_hostmode;
>>>> + else
>>>> + *bm_hostmode = 0;
>>>
>>> Hi,
>>>
>>> Perhaps not strictly related to this patch, but I see
>>> that mana_verify_resp_hdr() is called a few lines above.
>>> And that verifies a minimum msg_version. But I do not see
>>> any verification of the maximum msg_version supported by the code.
>>>
>>> I am concerned about a hypothetical scenario where, say the as yet unknown
>>> version 5 is sent as the version, and the above behaviour is used, while
>>> not being correct.
>>>
>>> Could you shed some light on this?
>>>
>>
>> In driver, we specify the expected reply msg version is v3 here:
>> req.hdr.resp.msg_version = GDMA_MESSAGE_V3;
>>
>> If the HW side is upgraded, it won't send reply msg version higher
>> than expected, which may break the driver.
>
> Thanks,
>
> If I understand things correctly the HW side will honour the
> req.hdr.resp.msg_version and thus the SW won't receive anything
> it doesn't expect. Is that right?
@Haiyang, if Simon's interpretation is correct, please change the
version checking in the driver from:
if (resp.hdr.response.msg_version >= GDMA_MESSAGE_V3)
to
if (resp.hdr.response.msg_version == GDMA_MESSAGE_V3)
As the current code is misleading.
Thanks,
Paolo
^ permalink raw reply
* Re: [EXTERNAL] Re: [PATCH net-next,v2] net: mana: Add support for Multi Vports on Bare metal
From: Simon Horman @ 2025-05-22 12:02 UTC (permalink / raw)
To: Haiyang Zhang
Cc: linux-hyperv@vger.kernel.org, netdev@vger.kernel.org, Dexuan Cui,
stephen@networkplumber.org, KY Srinivasan, Paul Rosswurm,
olaf@aepfle.de, vkuznets@redhat.com, davem@davemloft.net,
wei.liu@kernel.org, edumazet@google.com, kuba@kernel.org,
pabeni@redhat.com, leon@kernel.org, Long Li,
ssengar@linux.microsoft.com, linux-rdma@vger.kernel.org,
daniel@iogearbox.net, john.fastabend@gmail.com,
bpf@vger.kernel.org, ast@kernel.org, hawk@kernel.org,
tglx@linutronix.de, shradhagupta@linux.microsoft.com,
andrew+netdev@lunn.ch, Konstantin Taranov,
linux-kernel@vger.kernel.org
In-Reply-To: <MN0PR21MB34373B1A0162D8452018ABAACA9EA@MN0PR21MB3437.namprd21.prod.outlook.com>
On Wed, May 21, 2025 at 05:28:33PM +0000, Haiyang Zhang wrote:
>
>
> > -----Original Message-----
> > From: Simon Horman <horms@kernel.org>
> > Sent: Wednesday, May 21, 2025 10:03 AM
> > To: Haiyang Zhang <haiyangz@microsoft.com>
> > Cc: linux-hyperv@vger.kernel.org; netdev@vger.kernel.org; Dexuan Cui
> > <decui@microsoft.com>; stephen@networkplumber.org; KY Srinivasan
> > <kys@microsoft.com>; Paul Rosswurm <paulros@microsoft.com>;
> > olaf@aepfle.de; vkuznets@redhat.com; davem@davemloft.net;
> > wei.liu@kernel.org; edumazet@google.com; kuba@kernel.org;
> > pabeni@redhat.com; leon@kernel.org; Long Li <longli@microsoft.com>;
> > ssengar@linux.microsoft.com; linux-rdma@vger.kernel.org;
> > daniel@iogearbox.net; john.fastabend@gmail.com; bpf@vger.kernel.org;
> > ast@kernel.org; hawk@kernel.org; tglx@linutronix.de;
> > shradhagupta@linux.microsoft.com; andrew+netdev@lunn.ch; Konstantin
> > Taranov <kotaranov@microsoft.com>; linux-kernel@vger.kernel.org
> > Subject: [EXTERNAL] Re: [PATCH net-next,v2] net: mana: Add support for
> > Multi Vports on Bare metal
> >
> > On Mon, May 19, 2025 at 09:20:36AM -0700, Haiyang Zhang wrote:
> > > To support Multi Vports on Bare metal, increase the device config
> > response
> > > version. And, skip the register HW vport, and register filter steps,
> > when
> > > the Bare metal hostmode is set.
> > >
> > > Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
> > > ---
> > > v2:
> > > Updated comments as suggested by ALOK TIWARI.
> > > Fixed the version check.
> > >
> > > ---
> > > drivers/net/ethernet/microsoft/mana/mana_en.c | 24 ++++++++++++-------
> > > include/net/mana/mana.h | 4 +++-
> > > 2 files changed, 19 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/drivers/net/ethernet/microsoft/mana/mana_en.c
> > b/drivers/net/ethernet/microsoft/mana/mana_en.c
> > > index 2bac6be8f6a0..9c58d9e0bbb5 100644
> > > --- a/drivers/net/ethernet/microsoft/mana/mana_en.c
> > > +++ b/drivers/net/ethernet/microsoft/mana/mana_en.c
> > > @@ -921,7 +921,7 @@ static void mana_pf_deregister_filter(struct
> > mana_port_context *apc)
> > >
> > > static int mana_query_device_cfg(struct mana_context *ac, u32
> > proto_major_ver,
> > > u32 proto_minor_ver, u32 proto_micro_ver,
> > > - u16 *max_num_vports)
> > > + u16 *max_num_vports, u8 *bm_hostmode)
> > > {
> > > struct gdma_context *gc = ac->gdma_dev->gdma_context;
> > > struct mana_query_device_cfg_resp resp = {};
> > > @@ -932,7 +932,7 @@ static int mana_query_device_cfg(struct mana_context
> > *ac, u32 proto_major_ver,
> > > mana_gd_init_req_hdr(&req.hdr, MANA_QUERY_DEV_CONFIG,
> > > sizeof(req), sizeof(resp));
> > >
> > > - req.hdr.resp.msg_version = GDMA_MESSAGE_V2;
> > > + req.hdr.resp.msg_version = GDMA_MESSAGE_V3;
> > >
> > > req.proto_major_ver = proto_major_ver;
> > > req.proto_minor_ver = proto_minor_ver;
> >
> > > @@ -956,11 +956,16 @@ static int mana_query_device_cfg(struct
> > mana_context *ac, u32 proto_major_ver,
> > >
> > > *max_num_vports = resp.max_num_vports;
> > >
> > > - if (resp.hdr.response.msg_version == GDMA_MESSAGE_V2)
> > > + if (resp.hdr.response.msg_version >= GDMA_MESSAGE_V2)
> > > gc->adapter_mtu = resp.adapter_mtu;
> > > else
> > > gc->adapter_mtu = ETH_FRAME_LEN;
> > >
> > > + if (resp.hdr.response.msg_version >= GDMA_MESSAGE_V3)
> > > + *bm_hostmode = resp.bm_hostmode;
> > > + else
> > > + *bm_hostmode = 0;
> >
> > Hi,
> >
> > Perhaps not strictly related to this patch, but I see
> > that mana_verify_resp_hdr() is called a few lines above.
> > And that verifies a minimum msg_version. But I do not see
> > any verification of the maximum msg_version supported by the code.
> >
> > I am concerned about a hypothetical scenario where, say the as yet unknown
> > version 5 is sent as the version, and the above behaviour is used, while
> > not being correct.
> >
> > Could you shed some light on this?
> >
>
> In driver, we specify the expected reply msg version is v3 here:
> req.hdr.resp.msg_version = GDMA_MESSAGE_V3;
>
> If the HW side is upgraded, it won't send reply msg version higher
> than expected, which may break the driver.
Thanks,
If I understand things correctly the HW side will honour the
req.hdr.resp.msg_version and thus the SW won't receive anything
it doesn't expect. Is that right?
^ permalink raw reply
* Re: [PATCH v3 4/4] net: mana: Allocate MSI-X vectors dynamically
From: Shradha Gupta @ 2025-05-22 9:17 UTC (permalink / raw)
To: Michael Kelley
Cc: Dexuan Cui, Wei Liu, Haiyang Zhang, K. Y. Srinivasan, Andrew Lunn,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Konstantin Taranov, Simon Horman, Leon Romanovsky, Maxim Levitsky,
Erni Sri Satya Vennela, Peter Zijlstra,
linux-hyperv@vger.kernel.org, linux-pci@vger.kernel.org,
linux-kernel@vger.kernel.org, Nipun Gupta, Yury Norov,
Jason Gunthorpe, Jonathan Cameron, Anna-Maria Behnsen, Kevin Tian,
Long Li, Thomas Gleixner, Bjorn Helgaas, Rob Herring,
Manivannan Sadhasivam, Krzysztof Wilczy???~Dski,
Lorenzo Pieralisi, netdev@vger.kernel.org,
linux-rdma@vger.kernel.org, Paul Rosswurm, Shradha Gupta
In-Reply-To: <BN7PR02MB414863E6F4E36CF14F71F80BD49EA@BN7PR02MB4148.namprd02.prod.outlook.com>
On Wed, May 21, 2025 at 04:27:04PM +0000, Michael Kelley wrote:
> From: Shradha Gupta <shradhagupta@linux.microsoft.com> Sent: Friday, May 9, 2025 3:14 AM
> >
> > Currently, the MANA driver allocates MSI-X vectors statically based on
> > MANA_MAX_NUM_QUEUES and num_online_cpus() values and in some cases ends
> > up allocating more vectors than it needs. This is because, by this time
> > we do not have a HW channel and do not know how many IRQs should be
> > allocated.
> >
> > To avoid this, we allocate 1 MSI-X vector during the creation of HWC and
> > after getting the value supported by hardware, dynamically add the
> > remaining MSI-X vectors.
> >
> > Signed-off-by: Shradha Gupta <shradhagupta@linux.microsoft.com>
> > Reviewed-by: Haiyang Zhang <haiyangz@microsoft.com>
> > ---
> > Changes in v3:
> > * implemented irq_contexts as xarrays rather than list
> > * split the patch to create a perparation patch around irq_setup()
> > * add log when IRQ allocation/setup for remaining IRQs fails
> > ---
> > Changes in v2:
> > * Use string 'MSI-X vectors' instead of 'pci vectors'
> > * make skip-cpu a bool instead of int
> > * rearrange the comment arout skip_cpu variable appropriately
> > * update the capability bit for driver indicating dynamic IRQ allocation
> > * enforced max line length to 80
> > * enforced RCT convention
> > * initialized gic to NULL, for when there is a possibility of gic
> > not being populated correctly
> > ---
> > .../net/ethernet/microsoft/mana/gdma_main.c | 248 +++++++++++++++---
> > include/net/mana/gdma.h | 8 +-
> > 2 files changed, 211 insertions(+), 45 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/microsoft/mana/gdma_main.c
> > b/drivers/net/ethernet/microsoft/mana/gdma_main.c
> > index 2de42ce43373..f07cebffc30d 100644
> > --- a/drivers/net/ethernet/microsoft/mana/gdma_main.c
> > +++ b/drivers/net/ethernet/microsoft/mana/gdma_main.c
> > @@ -6,6 +6,8 @@
> > #include <linux/pci.h>
> > #include <linux/utsname.h>
> > #include <linux/version.h>
> > +#include <linux/msi.h>
> > +#include <linux/irqdomain.h>
> >
> > #include <net/mana/mana.h>
> >
> > @@ -80,8 +82,15 @@ static int mana_gd_query_max_resources(struct pci_dev *pdev)
> > return err ? err : -EPROTO;
> > }
> >
> > - if (gc->num_msix_usable > resp.max_msix)
> > - gc->num_msix_usable = resp.max_msix;
> > + if (!pci_msix_can_alloc_dyn(pdev)) {
> > + if (gc->num_msix_usable > resp.max_msix)
> > + gc->num_msix_usable = resp.max_msix;
> > + } else {
> > + /* If dynamic allocation is enabled we have already allocated
> > + * hwc msi
> > + */
> > + gc->num_msix_usable = min(resp.max_msix, num_online_cpus() + 1);
> > + }
> >
> > if (gc->num_msix_usable <= 1)
> > return -ENOSPC;
> > @@ -482,7 +491,9 @@ static int mana_gd_register_irq(struct gdma_queue *queue,
> > }
> >
> > queue->eq.msix_index = msi_index;
> > - gic = &gc->irq_contexts[msi_index];
> > + gic = xa_load(&gc->irq_contexts, msi_index);
> > + if (!gic)
> > + return -EINVAL;
> >
> > spin_lock_irqsave(&gic->lock, flags);
> > list_add_rcu(&queue->entry, &gic->eq_list);
> > @@ -507,7 +518,10 @@ static void mana_gd_deregiser_irq(struct gdma_queue *queue)
> > if (WARN_ON(msix_index >= gc->num_msix_usable))
> > return;
> >
> > - gic = &gc->irq_contexts[msix_index];
> > + gic = xa_load(&gc->irq_contexts, msix_index);
> > + if (!gic)
> > + return;
>
> If xa_load() doesn't return a valid gic, it seems like that would warrant a
> WARN_ON(), like the above case where the msix_index is out of range.
>
That makes sense, I will add this change
> > +
> > spin_lock_irqsave(&gic->lock, flags);
> > list_for_each_entry_rcu(eq, &gic->eq_list, entry) {
> > if (queue == eq) {
> > @@ -1329,29 +1343,96 @@ static int irq_setup(unsigned int *irqs, unsigned int len, int node,
> > return 0;
> > }
> >
> > -static int mana_gd_setup_irqs(struct pci_dev *pdev)
> > +static int mana_gd_setup_dyn_irqs(struct pci_dev *pdev, int nvec)
> > {
> > struct gdma_context *gc = pci_get_drvdata(pdev);
> > - unsigned int max_queues_per_port;
> > struct gdma_irq_context *gic;
> > - unsigned int max_irqs, cpu;
> > - int start_irq_index = 1;
> > - int nvec, *irqs, irq;
> > + bool skip_first_cpu = false;
> > int err, i = 0, j;
>
> Initializing "i" to 0 is superfluous. The "for" loop below does it.
>
noted
> > + int *irqs, irq;
> >
> > cpus_read_lock();
> > - max_queues_per_port = num_online_cpus();
> > - if (max_queues_per_port > MANA_MAX_NUM_QUEUES)
> > - max_queues_per_port = MANA_MAX_NUM_QUEUES;
> >
> > - /* Need 1 interrupt for the Hardware communication Channel (HWC) */
> > - max_irqs = max_queues_per_port + 1;
> > + irqs = kmalloc_array(nvec, sizeof(int), GFP_KERNEL);
> > + if (!irqs) {
> > + err = -ENOMEM;
> > + goto free_irq_vector;
> > + }
> >
> > - nvec = pci_alloc_irq_vectors(pdev, 2, max_irqs, PCI_IRQ_MSIX);
> > - if (nvec < 0) {
> > - cpus_read_unlock();
> > - return nvec;
> > + for (i = 0; i < nvec; i++) {
> > + gic = kcalloc(1, sizeof(struct gdma_irq_context), GFP_KERNEL);
>
> kcalloc() with a constant 1 first argument is a bit unusual. Just use kzalloc() since
> there's no array here?
>
sure, will make this change
> > + if (!gic) {
> > + err = -ENOMEM;
> > + goto free_irq;
> > + }
> > + gic->handler = mana_gd_process_eq_events;
> > + INIT_LIST_HEAD(&gic->eq_list);
> > + spin_lock_init(&gic->lock);
> > +
> > + snprintf(gic->name, MANA_IRQ_NAME_SZ, "mana_q%d@pci:%s",
> > + i, pci_name(pdev));
> > +
> > + /* one pci vector is already allocated for HWC */
> > + irqs[i] = pci_irq_vector(pdev, i + 1);
> > + if (irqs[i] < 0) {
> > + err = irqs[i];
> > + goto free_current_gic;
> > + }
> > +
> > + err = request_irq(irqs[i], mana_gd_intr, 0, gic->name, gic);
> > + if (err)
> > + goto free_current_gic;
> > +
> > + xa_store(&gc->irq_contexts, i + 1, gic, GFP_KERNEL);
> > }
> > +
> > + /*
> > + * When calling irq_setup() for dynamically added IRQs, if number of
> > + * CPUs is more than or equal to allocated MSI-X, we need to skip the
> > + * first CPU sibling group since they are already affinitized to HWC IRQ
> > + */
> > + if (gc->num_msix_usable <= num_online_cpus())
> > + skip_first_cpu = true;
> > +
> > + err = irq_setup(irqs, nvec, gc->numa_node, skip_first_cpu);
> > + if (err)
> > + goto free_irq;
> > +
> > + cpus_read_unlock();
> > + kfree(irqs);
> > + return 0;
> > +
> > +free_current_gic:
> > + kfree(gic);
> > +free_irq:
>
> In the error case, this label is reached with "i" in two possible
> states. Case 1: It might be the index of the entry that failed due to
> the "goto free_current_gic" statements. Case 2: It might be the
> index of one entry past all the successfully requested irqs, when the
> failure occurs on irq_setup() and the code does "goto free_irq".
>
> > + for (j = i; j >= 0; j--) {
>
> So the "for" loop starts with "j" set to an index that doesn't
> exist (in Case 2 above), or an index that is only partially
> complete (Case 1 above).
>
> And actually, local variable "j" isn't needed for this loop.
> It could just count down using "i".
>
> > + irq = pci_irq_vector(pdev, j);
>
> This seems to be looking up the wrong irq vector. In the main
> loop earlier, the index to pci_irq_vector() is "i + 1" but there's
> no "+ 1" here.
>
> > + gic = xa_load(&gc->irq_contexts, j);
> > + if (!gic)
> > + continue;
>
> So evidently it is expected that this xa_load() will fail
> the first time through this "j" loop. In Case 1, the xa_store()
> was never done, and in Case 2, the index starts out one
> too big.
>
> > +
> > + irq_update_affinity_hint(irq, NULL);
> > + free_irq(irq, gic);
> > + xa_erase(&gc->irq_contexts, j);
> > + kfree(gic);
> > + }
>
> Except for the wrong index to pci_irq_vector(), I think this works,
> but it's a bit bizarre. More natural would be to initialize "j" to
> "i - 1" so that the first iteration through the loop isn't degenerate.
> In that case, all the calls to xa_load() should succeed, and you
> might put a WARN_ON() if there's a failure.
>
Okay, I think I get your point. Let me try to make this error handling
more intuitive and straight forward.
> > + kfree(irqs);
> > +free_irq_vector:
> > + cpus_read_unlock();
> > + return err;
> > +}
> > +
> > +static int mana_gd_setup_irqs(struct pci_dev *pdev, int nvec)
> > +{
> > + struct gdma_context *gc = pci_get_drvdata(pdev);
> > + struct gdma_irq_context *gic;
> > + int start_irq_index = 1;
> > + unsigned int cpu;
> > + int *irqs, irq;
> > + int err, i = 0, j;
>
> Initializing "i" to 0 is superfluous.
>
Noted
> > +
> > + cpus_read_lock();
> > +
> > if (nvec <= num_online_cpus())
> > start_irq_index = 0;
> >
> > @@ -1361,15 +1442,13 @@ static int mana_gd_setup_irqs(struct pci_dev *pdev)
> > goto free_irq_vector;
> > }
> >
> > - gc->irq_contexts = kcalloc(nvec, sizeof(struct gdma_irq_context),
> > - GFP_KERNEL);
> > - if (!gc->irq_contexts) {
> > - err = -ENOMEM;
> > - goto free_irq_array;
> > - }
> > -
> > for (i = 0; i < nvec; i++) {
> > - gic = &gc->irq_contexts[i];
> > + gic = kcalloc(1, sizeof(struct gdma_irq_context), GFP_KERNEL);
>
> kcalloc() with a constant 1 first argument is a bit unusual. Just use kzalloc() since
> there's no array here?
>
noted
> > + if (!gic) {
> > + err = -ENOMEM;
> > + goto free_irq;
> > + }
> > +
> > gic->handler = mana_gd_process_eq_events;
> > INIT_LIST_HEAD(&gic->eq_list);
> > spin_lock_init(&gic->lock);
> > @@ -1384,13 +1463,13 @@ static int mana_gd_setup_irqs(struct pci_dev *pdev)
> > irq = pci_irq_vector(pdev, i);
> > if (irq < 0) {
> > err = irq;
> > - goto free_irq;
> > + goto free_current_gic;
> > }
> >
> > if (!i) {
> > err = request_irq(irq, mana_gd_intr, 0, gic->name, gic);
> > if (err)
> > - goto free_irq;
> > + goto free_current_gic;
> >
> > /* If number of IRQ is one extra than number of online CPUs,
> > * then we need to assign IRQ0 (hwc irq) and IRQ1 to
> > @@ -1408,39 +1487,110 @@ static int mana_gd_setup_irqs(struct pci_dev *pdev)
> > }
> > } else {
> > irqs[i - start_irq_index] = irq;
> > - err = request_irq(irqs[i - start_irq_index], mana_gd_intr, 0,
> > - gic->name, gic);
> > + err = request_irq(irqs[i - start_irq_index],
> > + mana_gd_intr, 0, gic->name, gic);
> > if (err)
> > - goto free_irq;
> > + goto free_current_gic;
> > }
> > +
> > + xa_store(&gc->irq_contexts, i, gic, GFP_KERNEL);
> > }
>
> FWIW, I think all this logic around "start_irq_index" could be simplified,
> though I haven't worked through the details. If it would simplify the code,
> it would be fine to allocate the "irqs" array with one extra entry that is unused
> if IRQ0 and IRQ1 use the same CPUs.
>
Sure, let me try that
> >
> > err = irq_setup(irqs, (nvec - start_irq_index), gc->numa_node, false);
> > if (err)
> > goto free_irq;
> >
> > - gc->max_num_msix = nvec;
> > - gc->num_msix_usable = nvec;
> > cpus_read_unlock();
> > kfree(irqs);
> > return 0;
> >
> > +free_current_gic:
> > + kfree(gic);
> > free_irq:
> > for (j = i - 1; j >= 0; j--) {
>
> In this case j is initialized to i - 1, which is what I expected in
> the previous case.
>
> Again, this loop could just countdown using "i" instead of
> introducing "j".
>
> > irq = pci_irq_vector(pdev, j);
> > - gic = &gc->irq_contexts[j];
> > + gic = xa_load(&gc->irq_contexts, j);
> > + if (!gic)
> > + continue;
>
> Failure to get a valid gic should never happen, so perhaps a WARN_ON()
> is appropriate.
>
noted
> >
> > irq_update_affinity_hint(irq, NULL);
> > free_irq(irq, gic);
> > + xa_erase(&gc->irq_contexts, j);
> > + kfree(gic);
> > }
> >
> > - kfree(gc->irq_contexts);
> > - gc->irq_contexts = NULL;
> > -free_irq_array:
> > kfree(irqs);
> > free_irq_vector:
> > + xa_destroy(&gc->irq_contexts);
>
> This seems like the wrong place to be doing xa_destroy(). It leads
> to inconsistencies. For example, if mana_gd_setup_hwc_irqs()
> fails, it may have failed before calling mana_gd_setup_irqs(), in
> which case the xa_destroy() is not done. Or if the failure occurred here
> in mana_gd_setup_irqs(), then xa_destroy() will have been done.
> Ideally, the xa_destroy() for error cases could be done in
> mana_gd_probe() where the xa_init() is done so that the calls match
> up, and of course in mana_gd_remove() when the device goes away
> entirely.
>
I agree, I'll fix this
> > cpus_read_unlock();
> > - pci_free_irq_vectors(pdev);
> > + return err;
> > +}
> > +
> > +static int mana_gd_setup_hwc_irqs(struct pci_dev *pdev)
> > +{
> > + struct gdma_context *gc = pci_get_drvdata(pdev);
> > + unsigned int max_irqs, min_irqs;
> > + int max_queues_per_port;
> > + int nvec, err;
> > +
> > + if (pci_msix_can_alloc_dyn(pdev)) {
> > + max_irqs = 1;
> > + min_irqs = 1;
> > + } else {
> > + max_queues_per_port = num_online_cpus();
> > + if (max_queues_per_port > MANA_MAX_NUM_QUEUES)
> > + max_queues_per_port = MANA_MAX_NUM_QUEUES;
> > + /* Need 1 interrupt for HWC */
> > + max_irqs = max_queues_per_port + 1;
>
> This code is simply being copied from existing code, but it would be nicer to
> code it as:
>
> max_irqs = min(num_online_cpus(), MANA_MAX_NUM_QUEUES) + 1;
>
> Explicitly using the "min" function reduces the cognitive effort to parse
> the "if" statement and figure out that it is picking the minimum of the two
> values. And the local variable max_queues_per_port can be dropped, but
> keep the comment :-)
>
noted
> > + min_irqs = 2;
> > + }
> > +
> > + nvec = pci_alloc_irq_vectors(pdev, min_irqs, max_irqs, PCI_IRQ_MSIX);
> > + if (nvec < 0)
> > + return nvec;
> > +
> > + err = mana_gd_setup_irqs(pdev, nvec);
> > + if (err) {
> > + pci_free_irq_vectors(pdev);
> > + return err;
> > + }
> > +
> > + gc->num_msix_usable = nvec;
> > + gc->max_num_msix = nvec;
> > +
> > + return err;
>
> "err" should always be zero at this point, so could do "return 0"
>
noted
> > +}
> > +
> > +static int mana_gd_setup_remaining_irqs(struct pci_dev *pdev)
> > +{
> > + struct gdma_context *gc = pci_get_drvdata(pdev);
> > + int max_irqs, i, err = 0;
> > + struct msi_map irq_map;
> > +
> > + if (!pci_msix_can_alloc_dyn(pdev))
> > + /* remain irqs are already allocated with HWC IRQ */
> > + return 0;
> > +
> > + /* allocate only remaining IRQs*/
> > + max_irqs = gc->num_msix_usable - 1;
> > +
> > + for (i = 1; i <= max_irqs; i++) {
> > + irq_map = pci_msix_alloc_irq_at(pdev, i, NULL);
> > + if (!irq_map.virq) {
> > + err = irq_map.index;
> > + /* caller will handle cleaning up all allocated
> > + * irqs, after HWC is destroyed
> > + */
> > + return err;
> > + }
> > + }
> > +
> > + err = mana_gd_setup_dyn_irqs(pdev, max_irqs);
> > + if (err)
> > + return err;
> > +
> > + gc->max_num_msix = gc->max_num_msix + max_irqs;
> > +
> > return err;
>
> Again, err must always be zero here.
>
> > }
> >
> > @@ -1458,19 +1608,22 @@ static void mana_gd_remove_irqs(struct pci_dev *pdev)
> > if (irq < 0)
> > continue;
> >
> > - gic = &gc->irq_contexts[i];
> > + gic = xa_load(&gc->irq_contexts, i);
> > + if (!gic)
> > + continue;
> >
> > /* Need to clear the hint before free_irq */
> > irq_update_affinity_hint(irq, NULL);
> > free_irq(irq, gic);
> > + xa_erase(&gc->irq_contexts, i);
> > + kfree(gic);
> > }
> >
> > pci_free_irq_vectors(pdev);
> >
> > gc->max_num_msix = 0;
> > gc->num_msix_usable = 0;
> > - kfree(gc->irq_contexts);
> > - gc->irq_contexts = NULL;
> > + xa_destroy(&gc->irq_contexts);
>
> As noted above, I'm doubtful about this being the right place to do
> xa_destroy().
>
> > }
> >
> > static int mana_gd_setup(struct pci_dev *pdev)
> > @@ -1481,9 +1634,10 @@ static int mana_gd_setup(struct pci_dev *pdev)
> > mana_gd_init_registers(pdev);
> > mana_smc_init(&gc->shm_channel, gc->dev, gc->shm_base);
> >
> > - err = mana_gd_setup_irqs(pdev);
> > + err = mana_gd_setup_hwc_irqs(pdev);
> > if (err) {
> > - dev_err(gc->dev, "Failed to setup IRQs: %d\n", err);
> > + dev_err(gc->dev, "Failed to setup IRQs for HWC creation: %d\n",
> > + err);
> > return err;
> > }
> >
> > @@ -1499,6 +1653,12 @@ static int mana_gd_setup(struct pci_dev *pdev)
> > if (err)
> > goto destroy_hwc;
> >
> > + err = mana_gd_setup_remaining_irqs(pdev);
> > + if (err) {
> > + dev_err(gc->dev, "Failed to setup remaining IRQs: %d", err);
> > + goto destroy_hwc;
> > + }
> > +
> > err = mana_gd_detect_devices(pdev);
> > if (err)
> > goto destroy_hwc;
> > @@ -1575,6 +1735,7 @@ static int mana_gd_probe(struct pci_dev *pdev, const struct
> > pci_device_id *ent)
> > gc->is_pf = mana_is_pf(pdev->device);
> > gc->bar0_va = bar0_va;
> > gc->dev = &pdev->dev;
> > + xa_init(&gc->irq_contexts);
> >
> > if (gc->is_pf)
> > gc->mana_pci_debugfs = debugfs_create_dir("0", mana_debugfs_root);
> > diff --git a/include/net/mana/gdma.h b/include/net/mana/gdma.h
> > index 228603bf03f2..f20d1d1ea5e8 100644
> > --- a/include/net/mana/gdma.h
> > +++ b/include/net/mana/gdma.h
> > @@ -373,7 +373,7 @@ struct gdma_context {
> > unsigned int max_num_queues;
> > unsigned int max_num_msix;
> > unsigned int num_msix_usable;
> > - struct gdma_irq_context *irq_contexts;
> > + struct xarray irq_contexts;
> >
> > /* L2 MTU */
> > u16 adapter_mtu;
> > @@ -558,12 +558,16 @@ enum {
> > /* Driver can handle holes (zeros) in the device list */
> > #define GDMA_DRV_CAP_FLAG_1_DEV_LIST_HOLES_SUP BIT(11)
> >
> > +/* Driver supports dynamic MSI-X vector allocation */
> > +#define GDMA_DRV_CAP_FLAG_1_DYNAMIC_IRQ_ALLOC_SUPPORT BIT(13)
> > +
> > #define GDMA_DRV_CAP_FLAGS1 \
> > (GDMA_DRV_CAP_FLAG_1_EQ_SHARING_MULTI_VPORT | \
> > GDMA_DRV_CAP_FLAG_1_NAPI_WKDONE_FIX | \
> > GDMA_DRV_CAP_FLAG_1_HWC_TIMEOUT_RECONFIG | \
> > GDMA_DRV_CAP_FLAG_1_VARIABLE_INDIRECTION_TABLE_SUPPORT | \
> > - GDMA_DRV_CAP_FLAG_1_DEV_LIST_HOLES_SUP)
> > + GDMA_DRV_CAP_FLAG_1_DEV_LIST_HOLES_SUP | \
> > + GDMA_DRV_CAP_FLAG_1_DYNAMIC_IRQ_ALLOC_SUPPORT)
> >
> > #define GDMA_DRV_CAP_FLAGS2 0
> >
> > --
> > 2.34.1
> >
>
Thanks for all the comments Michael, I will have them all incorporated
in the next version.
^ permalink raw reply
* RE: [EXTERNAL] Re: [PATCH net-next,v4] net: mana: Add handler for hardware servicing events
From: Haiyang Zhang @ 2025-05-22 0:28 UTC (permalink / raw)
To: Jakub Kicinski
Cc: linux-hyperv@vger.kernel.org, netdev@vger.kernel.org, Dexuan Cui,
stephen@networkplumber.org, KY Srinivasan, Paul Rosswurm,
olaf@aepfle.de, vkuznets@redhat.com, davem@davemloft.net,
wei.liu@kernel.org, edumazet@google.com, pabeni@redhat.com,
leon@kernel.org, Long Li, ssengar@linux.microsoft.com,
linux-rdma@vger.kernel.org, daniel@iogearbox.net,
john.fastabend@gmail.com, bpf@vger.kernel.org, ast@kernel.org,
hawk@kernel.org, tglx@linutronix.de,
shradhagupta@linux.microsoft.com, andrew+netdev@lunn.ch,
Konstantin Taranov, horms@kernel.org,
linux-kernel@vger.kernel.org
In-Reply-To: <20250516180440.0db7b35a@kernel.org>
> -----Original Message-----
> From: Jakub Kicinski <kuba@kernel.org>
> Sent: Friday, May 16, 2025 9:05 PM
> To: Haiyang Zhang <haiyangz@microsoft.com>
> Cc: linux-hyperv@vger.kernel.org; netdev@vger.kernel.org; Dexuan Cui
> <decui@microsoft.com>; stephen@networkplumber.org; KY Srinivasan
> <kys@microsoft.com>; Paul Rosswurm <paulros@microsoft.com>;
> olaf@aepfle.de; vkuznets@redhat.com; davem@davemloft.net;
> wei.liu@kernel.org; edumazet@google.com; pabeni@redhat.com;
> leon@kernel.org; Long Li <longli@microsoft.com>;
> ssengar@linux.microsoft.com; linux-rdma@vger.kernel.org;
> daniel@iogearbox.net; john.fastabend@gmail.com; bpf@vger.kernel.org;
> ast@kernel.org; hawk@kernel.org; tglx@linutronix.de;
> shradhagupta@linux.microsoft.com; andrew+netdev@lunn.ch; Konstantin
> Taranov <kotaranov@microsoft.com>; horms@kernel.org; linux-
> kernel@vger.kernel.org
> Subject: [EXTERNAL] Re: [PATCH net-next,v4] net: mana: Add handler for
> hardware servicing events
>
> On Wed, 14 May 2025 13:30:37 -0700 Haiyang Zhang wrote:
> > + dev_info(gc->dev, "Start MANA service type:%d\n", type);
> > + gc->in_service = true;
> > + mns_wk->pdev = to_pci_dev(gc->dev);
> > + INIT_WORK(&mns_wk->serv_work, mana_serv_func);
> > + schedule_work(&mns_wk->serv_work);
>
> I don't see any refcounting in this patch, and the work is not
> canceled. What if the device is removed between work being scheduled
> and running?
Thanks for the review. I added refcnt handling, and submitted a new
patch.
- Haiyang
^ permalink raw reply
* [PATCH net-next,v5] net: mana: Add handler for hardware servicing events
From: Haiyang Zhang @ 2025-05-22 0:22 UTC (permalink / raw)
To: linux-hyperv, netdev
Cc: haiyangz, decui, stephen, kys, paulros, olaf, vkuznets, davem,
wei.liu, edumazet, kuba, pabeni, leon, longli, ssengar,
linux-rdma, daniel, john.fastabend, bpf, ast, hawk, tglx,
shradhagupta, andrew+netdev, kotaranov, horms, linux-kernel
To collaborate with hardware servicing events, upon receiving the special
EQE notification from the HW channel, remove the devices on this bus.
Then, after a waiting period based on the device specs, rescan the parent
bus to recover the devices.
Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
Reviewed-by: Shradha Gupta <shradhagupta@linux.microsoft.com>
Reviewed-by: Simon Horman <horms@kernel.org>
---
v5:
Get refcnt of the pdev struct to avoid removal before running the work
as suggested by Jakub Kicinski.
v4:
Renamed EQE type 135 to GDMA_EQE_HWC_RESET_REQUEST, since there can
be multiple cases of this reset request.
v3:
Updated for checkpatch warnings as suggested by Simon Horman.
v2:
Added dev_dbg for service type as suggested by Shradha Gupta.
Added driver cap bit.
---
.../net/ethernet/microsoft/mana/gdma_main.c | 75 +++++++++++++++++++
include/net/mana/gdma.h | 10 ++-
2 files changed, 83 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/microsoft/mana/gdma_main.c b/drivers/net/ethernet/microsoft/mana/gdma_main.c
index 4ffaf7588885..1091f233d8ad 100644
--- a/drivers/net/ethernet/microsoft/mana/gdma_main.c
+++ b/drivers/net/ethernet/microsoft/mana/gdma_main.c
@@ -352,11 +352,59 @@ void mana_gd_ring_cq(struct gdma_queue *cq, u8 arm_bit)
}
EXPORT_SYMBOL_NS(mana_gd_ring_cq, "NET_MANA");
+#define MANA_SERVICE_PERIOD 10
+
+struct mana_serv_work {
+ struct work_struct serv_work;
+ struct pci_dev *pdev;
+};
+
+static void mana_serv_func(struct work_struct *w)
+{
+ struct mana_serv_work *mns_wk;
+ struct pci_bus *bus, *parent;
+ struct pci_dev *pdev;
+
+ mns_wk = container_of(w, struct mana_serv_work, serv_work);
+ pdev = mns_wk->pdev;
+
+ pci_lock_rescan_remove();
+
+ if (!pdev)
+ goto out;
+
+ bus = pdev->bus;
+ if (!bus) {
+ dev_err(&pdev->dev, "MANA service: no bus\n");
+ goto out;
+ }
+
+ parent = bus->parent;
+ if (!parent) {
+ dev_err(&pdev->dev, "MANA service: no parent bus\n");
+ goto out;
+ }
+
+ pci_stop_and_remove_bus_device(bus->self);
+
+ msleep(MANA_SERVICE_PERIOD * 1000);
+
+ pci_rescan_bus(parent);
+
+out:
+ pci_unlock_rescan_remove();
+
+ pci_dev_put(pdev);
+ kfree(mns_wk);
+ module_put(THIS_MODULE);
+}
+
static void mana_gd_process_eqe(struct gdma_queue *eq)
{
u32 head = eq->head % (eq->queue_size / GDMA_EQE_SIZE);
struct gdma_context *gc = eq->gdma_dev->gdma_context;
struct gdma_eqe *eq_eqe_ptr = eq->queue_mem_ptr;
+ struct mana_serv_work *mns_wk;
union gdma_eqe_info eqe_info;
enum gdma_eqe_type type;
struct gdma_event event;
@@ -400,6 +448,33 @@ static void mana_gd_process_eqe(struct gdma_queue *eq)
eq->eq.callback(eq->eq.context, eq, &event);
break;
+ case GDMA_EQE_HWC_FPGA_RECONFIG:
+ dev_info(gc->dev, "Recv MANA service type:%d\n", type);
+
+ if (gc->in_service) {
+ dev_info(gc->dev, "Already in service\n");
+ break;
+ }
+
+ if (!try_module_get(THIS_MODULE)) {
+ dev_info(gc->dev, "Module is unloading\n");
+ break;
+ }
+
+ mns_wk = kzalloc(sizeof(*mns_wk), GFP_ATOMIC);
+ if (!mns_wk) {
+ module_put(THIS_MODULE);
+ break;
+ }
+
+ dev_info(gc->dev, "Start MANA service type:%d\n", type);
+ gc->in_service = true;
+ mns_wk->pdev = to_pci_dev(gc->dev);
+ pci_dev_get(mns_wk->pdev);
+ INIT_WORK(&mns_wk->serv_work, mana_serv_func);
+ schedule_work(&mns_wk->serv_work);
+ break;
+
default:
break;
}
diff --git a/include/net/mana/gdma.h b/include/net/mana/gdma.h
index 228603bf03f2..150ab3610869 100644
--- a/include/net/mana/gdma.h
+++ b/include/net/mana/gdma.h
@@ -58,7 +58,7 @@ enum gdma_eqe_type {
GDMA_EQE_HWC_INIT_EQ_ID_DB = 129,
GDMA_EQE_HWC_INIT_DATA = 130,
GDMA_EQE_HWC_INIT_DONE = 131,
- GDMA_EQE_HWC_SOC_RECONFIG = 132,
+ GDMA_EQE_HWC_FPGA_RECONFIG = 132,
GDMA_EQE_HWC_SOC_RECONFIG_DATA = 133,
GDMA_EQE_RNIC_QP_FATAL = 176,
};
@@ -388,6 +388,8 @@ struct gdma_context {
u32 test_event_eq_id;
bool is_pf;
+ bool in_service;
+
phys_addr_t bar0_pa;
void __iomem *bar0_va;
void __iomem *shm_base;
@@ -558,12 +560,16 @@ enum {
/* Driver can handle holes (zeros) in the device list */
#define GDMA_DRV_CAP_FLAG_1_DEV_LIST_HOLES_SUP BIT(11)
+/* Driver can self reset on FPGA Reconfig EQE notification */
+#define GDMA_DRV_CAP_FLAG_1_HANDLE_RECONFIG_EQE BIT(17)
+
#define GDMA_DRV_CAP_FLAGS1 \
(GDMA_DRV_CAP_FLAG_1_EQ_SHARING_MULTI_VPORT | \
GDMA_DRV_CAP_FLAG_1_NAPI_WKDONE_FIX | \
GDMA_DRV_CAP_FLAG_1_HWC_TIMEOUT_RECONFIG | \
GDMA_DRV_CAP_FLAG_1_VARIABLE_INDIRECTION_TABLE_SUPPORT | \
- GDMA_DRV_CAP_FLAG_1_DEV_LIST_HOLES_SUP)
+ GDMA_DRV_CAP_FLAG_1_DEV_LIST_HOLES_SUP | \
+ GDMA_DRV_CAP_FLAG_1_HANDLE_RECONFIG_EQE)
#define GDMA_DRV_CAP_FLAGS2 0
--
2.34.1
^ permalink raw reply related
* Re: [PATCH net-next v2 8/8] net: core: Convert dev_set_mac_address_user() to use struct sockaddr_storage
From: Gustavo A. R. Silva @ 2025-05-21 23:07 UTC (permalink / raw)
To: Kees Cook, Kuniyuki Iwashima
Cc: Willem de Bruijn, Jason Wang, Andrew Lunn, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, Simon Horman,
Stanislav Fomichev, Cosmin Ratiu, Vladimir Oltean,
Florian Fainelli, Kory Maincent, Maxim Georgiev, netdev,
Martin K. Petersen, Christoph Hellwig, Sagi Grimberg,
Chaitanya Kulkarni, Mike Christie, Max Gurtovoy,
Maurizio Lombardi, Dmitry Bogdanov, Mingzhe Zou, Christophe Leroy,
Dr. David Alan Gilbert, Gustavo A. R. Silva, 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, Aleksander Jan Bajkowski,
Philipp Hahn, Eric Biggers, 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: <20250521204619.2301870-8-kees@kernel.org>
On 21/05/25 14:46, Kees Cook wrote:
> Convert callers of dev_set_mac_address_user() to use struct
> sockaddr_storage. Add sanity checks on dev->addr_len usage.
>
> Signed-off-by: Kees Cook <kees@kernel.org>
Acked-by: Gustavo A. R. Silva <gustavoars@kernel.org>
Thanks!
-Gustavo
> ---
> Cc: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
> Cc: Jason Wang <jasowang@redhat.com>
> 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: Simon Horman <horms@kernel.org>
> Cc: Stanislav Fomichev <sdf@fomichev.me>
> Cc: Cosmin Ratiu <cratiu@nvidia.com>
> Cc: Vladimir Oltean <vladimir.oltean@nxp.com>
> Cc: Florian Fainelli <florian.fainelli@broadcom.com>
> Cc: Kory Maincent <kory.maincent@bootlin.com>
> Cc: Maxim Georgiev <glipus@gmail.com>
> Cc: Kuniyuki Iwashima <kuniyu@amazon.com>
> Cc: <netdev@vger.kernel.org>
> ---
> include/linux/netdevice.h | 2 +-
> drivers/net/tap.c | 14 +++++++++-----
> drivers/net/tun.c | 8 +++++++-
> net/core/dev_api.c | 5 +++--
> net/core/dev_ioctl.c | 6 ++++--
> 5 files changed, 24 insertions(+), 11 deletions(-)
>
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index b4242b997373..adb14db25798 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -4216,7 +4216,7 @@ 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_storage *ss,
> struct netlink_ext_ack *extack);
> -int dev_set_mac_address_user(struct net_device *dev, struct sockaddr *sa,
> +int dev_set_mac_address_user(struct net_device *dev, struct sockaddr_storage *ss,
> struct netlink_ext_ack *extack);
> int dev_get_mac_address(struct sockaddr *sa, struct net *net, char *dev_name);
> int dev_get_port_parent_id(struct net_device *dev,
> diff --git a/drivers/net/tap.c b/drivers/net/tap.c
> index d4ece538f1b2..bdf0788d8e66 100644
> --- a/drivers/net/tap.c
> +++ b/drivers/net/tap.c
> @@ -923,7 +923,7 @@ static long tap_ioctl(struct file *file, unsigned int cmd,
> unsigned int __user *up = argp;
> unsigned short u;
> int __user *sp = argp;
> - struct sockaddr sa;
> + struct sockaddr_storage ss;
> int s;
> int ret;
>
> @@ -1000,16 +1000,17 @@ static long tap_ioctl(struct file *file, unsigned int cmd,
> return -ENOLINK;
> }
> ret = 0;
> - dev_get_mac_address(&sa, dev_net(tap->dev), tap->dev->name);
> + dev_get_mac_address((struct sockaddr *)&ss, dev_net(tap->dev),
> + tap->dev->name);
> if (copy_to_user(&ifr->ifr_name, tap->dev->name, IFNAMSIZ) ||
> - copy_to_user(&ifr->ifr_hwaddr, &sa, sizeof(sa)))
> + copy_to_user(&ifr->ifr_hwaddr, &ss, sizeof(ifr->ifr_hwaddr)))
> ret = -EFAULT;
> tap_put_tap_dev(tap);
> rtnl_unlock();
> return ret;
>
> case SIOCSIFHWADDR:
> - if (copy_from_user(&sa, &ifr->ifr_hwaddr, sizeof(sa)))
> + if (copy_from_user(&ss, &ifr->ifr_hwaddr, sizeof(ifr->ifr_hwaddr)))
> return -EFAULT;
> rtnl_lock();
> tap = tap_get_tap_dev(q);
> @@ -1017,7 +1018,10 @@ static long tap_ioctl(struct file *file, unsigned int cmd,
> rtnl_unlock();
> return -ENOLINK;
> }
> - ret = dev_set_mac_address_user(tap->dev, &sa, NULL);
> + if (tap->dev->addr_len > sizeof(ifr->ifr_hwaddr))
> + ret = -EINVAL;
> + else
> + ret = dev_set_mac_address_user(tap->dev, &ss, NULL);
> tap_put_tap_dev(tap);
> rtnl_unlock();
> return ret;
> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> index 7babd1e9a378..1207196cbbed 100644
> --- a/drivers/net/tun.c
> +++ b/drivers/net/tun.c
> @@ -3193,7 +3193,13 @@ static long __tun_chr_ioctl(struct file *file, unsigned int cmd,
>
> case SIOCSIFHWADDR:
> /* Set hw address */
> - ret = dev_set_mac_address_user(tun->dev, &ifr.ifr_hwaddr, NULL);
> + if (tun->dev->addr_len > sizeof(ifr.ifr_hwaddr)) {
> + ret = -EINVAL;
> + break;
> + }
> + ret = dev_set_mac_address_user(tun->dev,
> + (struct sockaddr_storage *)&ifr.ifr_hwaddr,
> + NULL);
> break;
>
> case TUNGETSNDBUF:
> diff --git a/net/core/dev_api.c b/net/core/dev_api.c
> index 6011a5ef649d..1bf0153195f2 100644
> --- a/net/core/dev_api.c
> +++ b/net/core/dev_api.c
> @@ -84,14 +84,15 @@ void dev_set_group(struct net_device *dev, int new_group)
> netdev_unlock_ops(dev);
> }
>
> -int dev_set_mac_address_user(struct net_device *dev, struct sockaddr *sa,
> +int dev_set_mac_address_user(struct net_device *dev,
> + struct sockaddr_storage *ss,
> struct netlink_ext_ack *extack)
> {
> int ret;
>
> down_write(&dev_addr_sem);
> netdev_lock_ops(dev);
> - ret = netif_set_mac_address(dev, (struct sockaddr_storage *)sa, extack);
> + ret = netif_set_mac_address(dev, ss, extack);
> netdev_unlock_ops(dev);
> up_write(&dev_addr_sem);
>
> diff --git a/net/core/dev_ioctl.c b/net/core/dev_ioctl.c
> index fff13a8b48f1..616479e71466 100644
> --- a/net/core/dev_ioctl.c
> +++ b/net/core/dev_ioctl.c
> @@ -572,9 +572,11 @@ static int dev_ifsioc(struct net *net, struct ifreq *ifr, void __user *data,
> return dev_set_mtu(dev, ifr->ifr_mtu);
>
> case SIOCSIFHWADDR:
> - if (dev->addr_len > sizeof(struct sockaddr))
> + if (dev->addr_len > sizeof(ifr->ifr_hwaddr))
> return -EINVAL;
> - return dev_set_mac_address_user(dev, &ifr->ifr_hwaddr, NULL);
> + return dev_set_mac_address_user(dev,
> + (struct sockaddr_storage *)&ifr->ifr_hwaddr,
> + NULL);
>
> case SIOCSIFHWBROADCAST:
> if (ifr->ifr_hwaddr.sa_family != dev->type)
^ permalink raw reply
* [PATCH net-next v2 7/8] rtnetlink: do_setlink: Use struct sockaddr_storage
From: Kees Cook @ 2025-05-21 20:46 UTC (permalink / raw)
To: Kuniyuki Iwashima
Cc: Kees Cook, Gustavo A . R . Silva, Eric Dumazet, Jakub Kicinski,
David S. Miller, Paolo Abeni, Simon Horman, Ido Schimmel, netdev,
Willem de Bruijn, Martin K. Petersen, Christoph Hellwig,
Sagi Grimberg, Chaitanya Kulkarni, 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, Jason Wang,
Vladimir Oltean, Florian Fainelli, Kory Maincent, Maxim Georgiev,
Aleksander Jan Bajkowski, Philipp Hahn, Eric Biggers,
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: <20250521204310.it.500-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.
Acked-by: Gustavo A. R. Silva <gustavoars@kernel.org>
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 net-next v2 8/8] net: core: Convert dev_set_mac_address_user() to use struct sockaddr_storage
From: Kees Cook @ 2025-05-21 20:46 UTC (permalink / raw)
To: Kuniyuki Iwashima
Cc: Kees Cook, Willem de Bruijn, Jason Wang, Andrew Lunn,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Simon Horman, Stanislav Fomichev, Cosmin Ratiu, Vladimir Oltean,
Florian Fainelli, Kory Maincent, Maxim Georgiev, netdev,
Martin K. Petersen, Christoph Hellwig, Sagi Grimberg,
Chaitanya Kulkarni, Mike Christie, Max Gurtovoy,
Maurizio Lombardi, Dmitry Bogdanov, Mingzhe Zou, Christophe Leroy,
Dr. David Alan Gilbert, Gustavo A. R. Silva, 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, Aleksander Jan Bajkowski,
Philipp Hahn, Eric Biggers, 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: <20250521204310.it.500-kees@kernel.org>
Convert callers of dev_set_mac_address_user() to use struct
sockaddr_storage. Add sanity checks on dev->addr_len usage.
Signed-off-by: Kees Cook <kees@kernel.org>
---
Cc: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
Cc: Jason Wang <jasowang@redhat.com>
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: Simon Horman <horms@kernel.org>
Cc: Stanislav Fomichev <sdf@fomichev.me>
Cc: Cosmin Ratiu <cratiu@nvidia.com>
Cc: Vladimir Oltean <vladimir.oltean@nxp.com>
Cc: Florian Fainelli <florian.fainelli@broadcom.com>
Cc: Kory Maincent <kory.maincent@bootlin.com>
Cc: Maxim Georgiev <glipus@gmail.com>
Cc: Kuniyuki Iwashima <kuniyu@amazon.com>
Cc: <netdev@vger.kernel.org>
---
include/linux/netdevice.h | 2 +-
drivers/net/tap.c | 14 +++++++++-----
drivers/net/tun.c | 8 +++++++-
net/core/dev_api.c | 5 +++--
net/core/dev_ioctl.c | 6 ++++--
5 files changed, 24 insertions(+), 11 deletions(-)
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index b4242b997373..adb14db25798 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -4216,7 +4216,7 @@ 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_storage *ss,
struct netlink_ext_ack *extack);
-int dev_set_mac_address_user(struct net_device *dev, struct sockaddr *sa,
+int dev_set_mac_address_user(struct net_device *dev, struct sockaddr_storage *ss,
struct netlink_ext_ack *extack);
int dev_get_mac_address(struct sockaddr *sa, struct net *net, char *dev_name);
int dev_get_port_parent_id(struct net_device *dev,
diff --git a/drivers/net/tap.c b/drivers/net/tap.c
index d4ece538f1b2..bdf0788d8e66 100644
--- a/drivers/net/tap.c
+++ b/drivers/net/tap.c
@@ -923,7 +923,7 @@ static long tap_ioctl(struct file *file, unsigned int cmd,
unsigned int __user *up = argp;
unsigned short u;
int __user *sp = argp;
- struct sockaddr sa;
+ struct sockaddr_storage ss;
int s;
int ret;
@@ -1000,16 +1000,17 @@ static long tap_ioctl(struct file *file, unsigned int cmd,
return -ENOLINK;
}
ret = 0;
- dev_get_mac_address(&sa, dev_net(tap->dev), tap->dev->name);
+ dev_get_mac_address((struct sockaddr *)&ss, dev_net(tap->dev),
+ tap->dev->name);
if (copy_to_user(&ifr->ifr_name, tap->dev->name, IFNAMSIZ) ||
- copy_to_user(&ifr->ifr_hwaddr, &sa, sizeof(sa)))
+ copy_to_user(&ifr->ifr_hwaddr, &ss, sizeof(ifr->ifr_hwaddr)))
ret = -EFAULT;
tap_put_tap_dev(tap);
rtnl_unlock();
return ret;
case SIOCSIFHWADDR:
- if (copy_from_user(&sa, &ifr->ifr_hwaddr, sizeof(sa)))
+ if (copy_from_user(&ss, &ifr->ifr_hwaddr, sizeof(ifr->ifr_hwaddr)))
return -EFAULT;
rtnl_lock();
tap = tap_get_tap_dev(q);
@@ -1017,7 +1018,10 @@ static long tap_ioctl(struct file *file, unsigned int cmd,
rtnl_unlock();
return -ENOLINK;
}
- ret = dev_set_mac_address_user(tap->dev, &sa, NULL);
+ if (tap->dev->addr_len > sizeof(ifr->ifr_hwaddr))
+ ret = -EINVAL;
+ else
+ ret = dev_set_mac_address_user(tap->dev, &ss, NULL);
tap_put_tap_dev(tap);
rtnl_unlock();
return ret;
diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index 7babd1e9a378..1207196cbbed 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -3193,7 +3193,13 @@ static long __tun_chr_ioctl(struct file *file, unsigned int cmd,
case SIOCSIFHWADDR:
/* Set hw address */
- ret = dev_set_mac_address_user(tun->dev, &ifr.ifr_hwaddr, NULL);
+ if (tun->dev->addr_len > sizeof(ifr.ifr_hwaddr)) {
+ ret = -EINVAL;
+ break;
+ }
+ ret = dev_set_mac_address_user(tun->dev,
+ (struct sockaddr_storage *)&ifr.ifr_hwaddr,
+ NULL);
break;
case TUNGETSNDBUF:
diff --git a/net/core/dev_api.c b/net/core/dev_api.c
index 6011a5ef649d..1bf0153195f2 100644
--- a/net/core/dev_api.c
+++ b/net/core/dev_api.c
@@ -84,14 +84,15 @@ void dev_set_group(struct net_device *dev, int new_group)
netdev_unlock_ops(dev);
}
-int dev_set_mac_address_user(struct net_device *dev, struct sockaddr *sa,
+int dev_set_mac_address_user(struct net_device *dev,
+ struct sockaddr_storage *ss,
struct netlink_ext_ack *extack)
{
int ret;
down_write(&dev_addr_sem);
netdev_lock_ops(dev);
- ret = netif_set_mac_address(dev, (struct sockaddr_storage *)sa, extack);
+ ret = netif_set_mac_address(dev, ss, extack);
netdev_unlock_ops(dev);
up_write(&dev_addr_sem);
diff --git a/net/core/dev_ioctl.c b/net/core/dev_ioctl.c
index fff13a8b48f1..616479e71466 100644
--- a/net/core/dev_ioctl.c
+++ b/net/core/dev_ioctl.c
@@ -572,9 +572,11 @@ static int dev_ifsioc(struct net *net, struct ifreq *ifr, void __user *data,
return dev_set_mtu(dev, ifr->ifr_mtu);
case SIOCSIFHWADDR:
- if (dev->addr_len > sizeof(struct sockaddr))
+ if (dev->addr_len > sizeof(ifr->ifr_hwaddr))
return -EINVAL;
- return dev_set_mac_address_user(dev, &ifr->ifr_hwaddr, NULL);
+ return dev_set_mac_address_user(dev,
+ (struct sockaddr_storage *)&ifr->ifr_hwaddr,
+ NULL);
case SIOCSIFHWBROADCAST:
if (ifr->ifr_hwaddr.sa_family != dev->type)
--
2.34.1
^ permalink raw reply related
* [PATCH net-next v2 3/8] net/ncsi: Use struct sockaddr_storage for pending_mac
From: Kees Cook @ 2025-05-21 20:46 UTC (permalink / raw)
To: Kuniyuki Iwashima
Cc: Kees Cook, Gustavo A . R . Silva, Samuel Mendoza-Jonas,
Paul Fertser, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Simon Horman, netdev, Willem de Bruijn,
Martin K. Petersen, Christoph Hellwig, Sagi Grimberg,
Chaitanya Kulkarni, 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, Jason Wang, Vladimir Oltean,
Florian Fainelli, Kory Maincent, Maxim Georgiev,
Aleksander Jan Bajkowski, Philipp Hahn, Eric Biggers,
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: <20250521204310.it.500-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.
Acked-by: Gustavo A. R. Silva <gustavoars@kernel.org>
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 net-next v2 4/8] ieee802154: Use struct sockaddr_storage with dev_set_mac_address()
From: Kees Cook @ 2025-05-21 20:46 UTC (permalink / raw)
To: Kuniyuki Iwashima
Cc: Kees Cook, Gustavo A . R . Silva, Alexander Aring, Stefan Schmidt,
Miquel Raynal, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Simon Horman, linux-wpan, netdev, Willem de Bruijn,
Martin K. Petersen, Christoph Hellwig, Sagi Grimberg,
Chaitanya Kulkarni, 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, Jason Wang, Vladimir Oltean,
Florian Fainelli, Kory Maincent, Maxim Georgiev,
Aleksander Jan Bajkowski, Philipp Hahn, Eric Biggers,
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: <20250521204310.it.500-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.
Acked-by: Gustavo A. R. Silva <gustavoars@kernel.org>
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
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