ACK/Cmnt: [PATCH] xen/events: avoid removing an event channel while handling it
Stefan Bader
stefan.bader at canonical.com
Tue Feb 23 15:05:12 UTC 2021
On 23.02.21 15:58, Tim Gardner wrote:
>
>
> On 2/23/21 7:55 AM, Stefan Bader wrote:
>> On 23.02.21 14:23, Tim Gardner wrote:
>>> From: Juergen Gross <jgross at suse.com>
>>>
>>> Today it can happen that an event channel is being removed from the
>>> system while the event handling loop is active. This can lead to a
>>> race resulting in crashes or WARN() splats when trying to access the
>>> irq_info structure related to the event channel.
>>>
>>> Fix this problem by using a rwlock taken as reader in the event
>>> handling loop and as writer when deallocating the irq_info structure.
>>>
>>> As the observed problem was a NULL dereference in evtchn_from_irq()
>>> make this function more robust against races by testing the irq_info
>>> pointer to be not NULL before dereferencing it.
>>>
>>> And finally make all accesses to evtchn_to_irq[row][col] atomic ones
>>> in order to avoid seeing partial updates of an array element in irq
>>> handling. Note that irq handling can be entered only for event channels
>>> which have been valid before, so any not populated row isn't a problem
>>> in this regard, as rows are only ever added and never removed.
>>>
>>> This is XSA-331.
>>>
>>> Cc: stable at vger.kernel.org
>>> Reported-by: Marek Marczykowski-Górecki <marmarek at invisiblethingslab.com>
>>> Reported-by: Jinoh Kang <luke1337 at theori.io>
>>> Signed-off-by: Juergen Gross <jgross at suse.com>
>>> Reviewed-by: Stefano Stabellini <sstabellini at kernel.org>
>>> Reviewed-by: Wei Liu <wl at xen.org>
>>> (backported from commit 073d0552ead5bfc7a3a9c01de590e924f11b5dd2)
>> [rtg: Minor context adjustment in xen_free_irq()]
^ Format, just like upstream does it...
>>> Signed-off-by: Tim Gardner <tim.gardner at canonical.com>
>> Acked-by: Stefan Bader <stefan.bader at canonical.com>
>>>
>>> Conflicts:
>>> drivers/xen/events/events_base.c
>>> ---
>>
>> I give up. Why above is not possible, I cannot understand.
>>
>
> I have no idea what you want here. As I pointed out above, the conflict was a
> minor context adjustment in xen_free_irq(). How can I better describe what I did ?
>
>> -Stefan
>>
>>> drivers/xen/events/events_base.c | 41 ++++++++++++++++++++++++++++----
>>> 1 file changed, 36 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/xen/events/events_base.c b/drivers/xen/events/events_base.c
>>> index 499eff7d3f65..c56fc81a409f 100644
>>> --- a/drivers/xen/events/events_base.c
>>> +++ b/drivers/xen/events/events_base.c
>>> @@ -33,6 +33,7 @@
>>> #include <linux/slab.h>
>>> #include <linux/irqnr.h>
>>> #include <linux/pci.h>
>>> +#include <linux/spinlock.h>
>>> #ifdef CONFIG_X86
>>> #include <asm/desc.h>
>>> @@ -70,6 +71,23 @@ const struct evtchn_ops *evtchn_ops;
>>> */
>>> static DEFINE_MUTEX(irq_mapping_update_lock);
>>> +/*
>>> + * Lock protecting event handling loop against removing event channels.
>>> + * Adding of event channels is no issue as the associated IRQ becomes active
>>> + * only after everything is setup (before request_[threaded_]irq() the handler
>>> + * can't be entered for an event, as the event channel will be unmasked only
>>> + * then).
>>> + */
>>> +static DEFINE_RWLOCK(evtchn_rwlock);
>>> +
>>> +/*
>>> + * Lock hierarchy:
>>> + *
>>> + * irq_mapping_update_lock
>>> + * evtchn_rwlock
>>> + * IRQ-desc lock
>>> + */
>>> +
>>> static LIST_HEAD(xen_irq_list_head);
>>> /* IRQ <-> VIRQ mapping. */
>>> @@ -102,7 +120,7 @@ static void clear_evtchn_to_irq_row(unsigned row)
>>> unsigned col;
>>> for (col = 0; col < EVTCHN_PER_ROW; col++)
>>> - evtchn_to_irq[row][col] = -1;
>>> + WRITE_ONCE(evtchn_to_irq[row][col], -1);
>>> }
>>> static void clear_evtchn_to_irq_all(void)
>>> @@ -139,7 +157,7 @@ static int set_evtchn_to_irq(unsigned evtchn, unsigned irq)
>>> clear_evtchn_to_irq_row(row);
>>> }
>>> - evtchn_to_irq[row][col] = irq;
>>> + WRITE_ONCE(evtchn_to_irq[row][col], irq);
>>> return 0;
>>> }
>>> @@ -149,7 +167,7 @@ int get_evtchn_to_irq(unsigned evtchn)
>>> return -1;
>>> if (evtchn_to_irq[EVTCHN_ROW(evtchn)] == NULL)
>>> return -1;
>>> - return evtchn_to_irq[EVTCHN_ROW(evtchn)][EVTCHN_COL(evtchn)];
>>> + return READ_ONCE(evtchn_to_irq[EVTCHN_ROW(evtchn)][EVTCHN_COL(evtchn)]);
>>> }
>>> /* Get info for IRQ */
>>> @@ -247,10 +265,14 @@ static void xen_irq_info_cleanup(struct irq_info *info)
>>> */
>>> unsigned int evtchn_from_irq(unsigned irq)
>>> {
>>> - if (WARN(irq >= nr_irqs, "Invalid irq %d!\n", irq))
>>> + const struct irq_info *info = NULL;
>>> +
>>> + if (likely(irq < nr_irqs))
>>> + info = info_for_irq(irq);
>>> + if (!info)
>>> return 0;
>>> - return info_for_irq(irq)->evtchn;
>>> + return info->evtchn;
>>> }
>>> unsigned irq_from_evtchn(unsigned int evtchn)
>>> @@ -426,16 +448,21 @@ static int __must_check xen_allocate_irq_gsi(unsigned gsi)
>>> static void xen_free_irq(unsigned irq)
>>> {
>>> struct irq_info *info = irq_get_handler_data(irq);
>>> + unsigned long flags;
>>> if (WARN_ON(!info))
>>> return;
>>> + write_lock_irqsave(&evtchn_rwlock, flags);
>>> +
>>> list_del(&info->list);
>>> irq_set_handler_data(irq, NULL);
>>> WARN_ON(info->refcnt > 0);
>>> + write_unlock_irqrestore(&evtchn_rwlock, flags);
>>> +
>>> kfree(info);
>>> /* Legacy IRQ descriptors are managed by the arch. */
>>> @@ -1218,6 +1245,8 @@ static void __xen_evtchn_do_upcall(void)
>>> struct vcpu_info *vcpu_info = __this_cpu_read(xen_vcpu);
>>> int cpu = smp_processor_id();
>>> + read_lock(&evtchn_rwlock);
>>> +
>>> do {
>>> vcpu_info->evtchn_upcall_pending = 0;
>>> @@ -1228,6 +1257,8 @@ static void __xen_evtchn_do_upcall(void)
>>> virt_rmb(); /* Hypervisor can set upcall pending. */
>>> } while (vcpu_info->evtchn_upcall_pending);
>>> +
>>> + read_unlock(&evtchn_rwlock);
>>> }
>>> void xen_evtchn_do_upcall(struct pt_regs *regs)
>>>
>>
>>
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <https://lists.ubuntu.com/archives/kernel-team/attachments/20210223/b17b4897/attachment-0001.sig>
More information about the kernel-team
mailing list