Re: [PATCH] KVM: arm64: vgic: drop WARN from vgic_get_irq

[Oliver Upton <oupton@google.com>]

On Thu, Aug 19, 2021 at 1:04 AM Marc Zyngier <maz@kernel.org> wrote:
>
> On Thu, 19 Aug 2021 08:41:19 +0100,
> Oliver Upton <oupton@google.com> wrote:
> >
> > On Wed, Aug 18, 2021 at 2:45 PM Ricardo Koller <ricarkol@google.com> wrote:
> > >
> > > On Wed, Aug 18, 2021 at 02:34:03PM -0700, Oliver Upton wrote:
> > > > Hi Ricardo,
> > > >
> > > > On Wed, Aug 18, 2021 at 2:32 PM Ricardo Koller <ricarkol@google.com> wrote:
> > > > >
> > > > > vgic_get_irq(intid) is used all over the vgic code in order to get a
> > > > > reference to a struct irq. It warns whenever intid is not a valid number
> > > > > (like when it's a reserved IRQ number). The issue is that this warning
> > > > > can be triggered from userspace (e.g., KVM_IRQ_LINE for intid 1020).
> > > > >
> > > > > Drop the WARN call from vgic_get_irq.
> > > > >
> > > > > Signed-off-by: Ricardo Koller <ricarkol@google.com>
> > > > > ---
> > > > >  arch/arm64/kvm/vgic/vgic.c | 1 -
> > > > >  1 file changed, 1 deletion(-)
> > > > >
> > > > > diff --git a/arch/arm64/kvm/vgic/vgic.c b/arch/arm64/kvm/vgic/vgic.c
> > > > > index 111bff47e471..81cec508d413 100644
> > > > > --- a/arch/arm64/kvm/vgic/vgic.c
> > > > > +++ b/arch/arm64/kvm/vgic/vgic.c
> > > > > @@ -106,7 +106,6 @@ struct vgic_irq *vgic_get_irq(struct kvm *kvm, struct kvm_vcpu *vcpu,
> > > > >         if (intid >= VGIC_MIN_LPI)
> > > > >                 return vgic_get_lpi(kvm, intid);
> > > > >
> > > > > -       WARN(1, "Looking up struct vgic_irq for reserved INTID");
> > > >
> > > > Could we maybe downgrade the message to WARN_ONCE() (to get a stack)
> > > > or pr_warn_ratelimited()? I agree it is problematic that userspace can
> > > > cause this WARN to fire, but it'd be helpful for debugging too.
> > > >
> > >
> > > Was thinking about that, until I found this in bug.h:
> > >
> > >         /*
> > >          * WARN(), WARN_ON(), WARN_ON_ONCE, and so on can be used to report
> > >          * significant kernel issues that need prompt attention if they should ever
> > >          * appear at runtime.
> > >          *
> > >          * Do not use these macros when checking for invalid external inputs
> > >          * (e.g. invalid system call arguments, or invalid data coming from
> > >          * network/devices),
> > >
> > > Just in case, KVM_IRQ_LINE returns -EINVAL for an invalid intid (like
> > > 1020). I think it's more appropriate for the vmm to log it. What do you
> > > think?
> >
> > vgic_get_irq() is called in a bunch of other places though, right?
> > IOW, intid doesn't necessarily come from userspace. In fact, I believe
> > KVM_IRQ_LINE is the only place we pass a value from userspace to
> > vgic_get_irq() (don't quote me on that).
> >
> > Perhaps instead the fix could be to explicitly check that the intid
> > from userspace is valid and exit early rather than count on
> > vgic_get_irq() to do the right thing.
>
> vgic_get_irq() is designed to do the right thing. Returning NULL is
> the way it reports an error, and this NULL value is already checked at
> when directly provided either by the VMM or the guest. If we missed
> any of those, that would be what needs addressing.  Obtaining a NULL
> pointer is a good way to catch those.
>
> In general, the kernel log is not how we report userspace errors (we
> have been there before...). It is slow, noisy, unclear and ultimately
> leaks information.

Absolutely. My comments were aimed at calls to vgic_get_irq() where
intid is coming from the kernel, not userspace. That being said,
probably no good reason to buy a full fat WARN() in a function such as
this one.  I'm done waffling on this one liner now :)

Reviewed-by: Oliver Upton <oupton@google.com>

> If you really want something, then a pr_debug is a
> potential tool as it can be dynamically enabled with the right
> configuration.
>
> Thanks,
>
>         M.
>
> --
> Without deviation from the norm, progress is not possible.