linux-kernel - Re: [PATCH] vfio: Drop vfio_file_iommu_group() stub to fudge around a KVM wart

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] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 29 Nov 2023 20:12:16 -0400
From:   Jason Gunthorpe <jgg@...dia.com>
To:     Sean Christopherson <seanjc@...gle.com>
Cc:     Alex Williamson <alex.williamson@...hat.com>, kvm@...r.kernel.org,
        linux-kernel@...r.kernel.org,
        Nick Desaulniers <ndesaulniers@...gle.com>,
        Michael Ellerman <mpe@...erman.id.au>
Subject: Re: [PATCH] vfio: Drop vfio_file_iommu_group() stub to fudge around
 a KVM wart

On Wed, Nov 29, 2023 at 04:10:00PM -0800, Sean Christopherson wrote:
> Drop the vfio_file_iommu_group() stub and instead unconditionally declare
> the function to fudge around a KVM wart where KVM tries to do symbol_get()
> on vfio_file_iommu_group() (and other VFIO symbols) even if CONFIG_VFIO=n.
> 
> Ensuring the symbol is always declared fixes a PPC build error when
> modules are also disabled, in which case symbol_get() simply points at the
> address of the symbol (with some attributes shenanigans).  Because KVM
> does symbol_get() instead of directly depending on VFIO, the lack of a
> fully defined symbol is not problematic (ugly, but "fine").
> 
>    arch/powerpc/kvm/../../../virt/kvm/vfio.c:89:7:
>    error: attribute declaration must precede definition [-Werror,-Wignored-attributes]
>            fn = symbol_get(vfio_file_iommu_group);
>                 ^
>    include/linux/module.h:805:60: note: expanded from macro 'symbol_get'
>    #define symbol_get(x) ({ extern typeof(x) x __attribute__((weak,visibility("hidden"))); &(x); })
>                                                               ^
>    include/linux/vfio.h:294:35: note: previous definition is here
>    static inline struct iommu_group *vfio_file_iommu_group(struct file *file)
>                                      ^
>    arch/powerpc/kvm/../../../virt/kvm/vfio.c:89:7:
>    error: attribute declaration must precede definition [-Werror,-Wignored-attributes]
>            fn = symbol_get(vfio_file_iommu_group);
>                 ^
>    include/linux/module.h:805:65: note: expanded from macro 'symbol_get'
>    #define symbol_get(x) ({ extern typeof(x) x __attribute__((weak,visibility("hidden"))); &(x); })
>                                                                    ^
>    include/linux/vfio.h:294:35: note: previous definition is here
>    static inline struct iommu_group *vfio_file_iommu_group(struct file *file)
>                                      ^
>    2 errors generated.
> 
> Although KVM is firmly in the wrong (there is zero reason for KVM to build
> virt/kvm/vfio.c when VFIO is disabled), fudge around the error in VFIO as
> the stub is unnecessary and doesn't serve its intended purpose (KVM is the
> only external user of vfio_file_iommu_group()), and there is an in-flight
> series to clean up the entire KVM<->VFIO interaction, i.e. fixing this in
> KVM would result in more churn in the long run, and the stub needs to go
> away regardless.
> 
> Reported-by: kernel test robot <lkp@...el.com>
> Closes: https://lore.kernel.org/oe-kbuild-all/202308251949.5IiaV0sz-lkp@intel.com
> Closes: https://lore.kernel.org/oe-kbuild-all/202309030741.82aLACDG-lkp@intel.com
> Closes: https://lore.kernel.org/oe-kbuild-all/202309110914.QLH0LU6L-lkp@intel.com
> Link: https://lore.kernel.org/all/0-v1-08396538817d+13c5-vfio_kvm_kconfig_jgg@nvidia.com
> Link: https://lore.kernel.org/all/20230916003118.2540661-1-seanjc@google.com
> Cc: Nick Desaulniers <ndesaulniers@...gle.com>
> Cc: Jason Gunthorpe <jgg@...dia.com>
> Tested-by: Michael Ellerman <mpe@...erman.id.au>
> Fixes: c1cce6d079b8 ("vfio: Compile vfio_group infrastructure optionally")
> Signed-off-by: Sean Christopherson <seanjc@...gle.com>
> ---
>  include/linux/vfio.h | 8 ++------
>  1 file changed, 2 insertions(+), 6 deletions(-)

Reviewed-by: Jason Gunthorpe <jgg@...dia.com>

Jason