linux-kernel - Re: [PATCH v4 0/2] PCI: hv: Refactor hv_irq_unmask() to use hv_vpset and cpumask_to

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <87d0nckwq8.fsf@vitty.brq.redhat.com>
Date:   Thu, 28 Feb 2019 11:56:15 +0100
From:   Vitaly Kuznetsov <vkuznets@...hat.com>
To:     Maya Nakamura <m.maya.nakamura@...il.com>
Cc:     linux-kernel@...r.kernel.org, haiyangz@...rosoft.com,
        marcelo.cerri@...onical.com, linux-hyperv@...r.kernel.org,
        lorenzo.pieralisi@....com, bhelgaas@...gle.com,
        linux-pci@...r.kernel.org, kys@...rosoft.com,
        sthemmin@...rosoft.com, olaf@...fle.de, apw@...onical.com,
        jasowang@...hat.com, mikelley@...rosoft.com,
        Alexander.Levin@...rosoft.com
Subject: Re: [PATCH v4 0/2] PCI: hv: Refactor hv_irq_unmask() to use hv_vpset and cpumask_to_vpset()

Maya Nakamura <m.maya.nakamura@...il.com> writes:

> This patchset removes a duplicate definition of VP set (hv_vp_set) and
> uses the common definition (hv_vpset) that is used in other places. It
> changes the order of the members in struct hv_pcibus_device due to
> flexible array in hv_vpset.
>
> It also removes the duplicate implementation of cpumask_to_vpset(), uses
> the shared implementation, and exports hv_max_vp_index, which is
> required by cpumask_to_vpset().
>
> Based on Vitaly's findings, two changes were applied: replace GFP_KERNEL
> with GFP_ATOMIC for alloc_cpumask_var() because hv_irq_unmask() runs
> while a spinlock is held, and add __aligned(8) to struct
> retarget_msi_interrupt because Hyper-V requires that hypercall arguments
> be aligned on an 8 byte boundary.
>
> Vitaly, thank you for finding the issues, and Lorenzo and Michael, thank
> you for your guidance and support!
>

Reviewed-by: Vitaly Kuznetsov <vkuznets@...hat.com>
Tested-by: Vitaly Kuznetsov <vkuznets@...hat.com>

I, however, agree with Lorenzo that '__aligned(8)' change deserves its
own patch as it fixes an already present issue (which may actually be
masked but still present).

Going forward I'd like to get rid of the newly added cpumask allocation:
while it is very unlikely to fail and it only happens when we reassign
IRQs to some different CPU (thus it's not a hotpath or anything)
failures are still possible. This work can definitely be postponed, I
don't think it should block your patches.

-- 
Vitaly