Re: [PATCH] iommu/iova: Don't BUG on invalid PFNs

[Robin Murphy]

On 2020-06-10 10:27, guptap@xxxxxxxxxxxxxx wrote:
> On 2020-06-02 18:38, Robin Murphy wrote:
> > Unlike the other instances which represent a complete loss of
> > consistency within the rcache mechanism itself, or a fundamental
> > and obvious misconfiguration by an IOMMU driver, the BUG_ON() in
> > iova_magazine_free_pfns() can be provoked at more or less any time
> > in a "spooky action-at-a-distance" manner by any old device driver
> > passing nonsense to dma_unmap_*() which then propagates through to
> > queue_iova().
> >
> > Not only is this well outside the IOVA layer's control, it's also
> > nowhere near fatal enough to justify panicking anyway - all that
> > really achieves is to make debugging the offending driver more
> > difficult. Let's simply WARN and otherwise ignore bogus PFNs.
> >
> > Reported-by: Prakash Gupta <guptap@xxxxxxxxxxxxxx>
> > Signed-off-by: Robin Murphy <robin.murphy@xxxxxxx>
> > ---
> > Âdrivers/iommu/iova.c | 4 +++-
> > Â1 file changed, 3 insertions(+), 1 deletion(-)
>
> Copying stable@xxxxxxxxxxxxxxx

Per Documentation/process/stable-kernel-rules.rst, I'm not convinced 
this meets the criteria for stable, which is why I deliberately left 
that out. This change isn't fixing any bug in itself, it is merely 
relaxing a behaviour that only comes into play if a serious and 
effectively unrecoverable bug has already occurred elsewhere.

If Joerg feels like sending it to stable anyway then fair enough, but 
FYI there is no relevant "Fixes" tag.

> You can add
> Reviewed-by: Prakash Gupta <guptap@xxxxxxxxxxxxxx>

Thanks,
Robin.

> > diff --git a/drivers/iommu/iova.c b/drivers/iommu/iova.c
> > index 0e6a9536eca6..612cbf668adf 100644
> > --- a/drivers/iommu/iova.c
> > +++ b/drivers/iommu/iova.c
> > @@ -811,7 +811,9 @@ iova_magazine_free_pfns(struct iova_magazine *mag,
> > struct iova_domain *iovad)
> > ÂÂÂÂ for (i = 0 ; i < mag->size; ++i) {
> > ÂÂÂÂÂÂÂÂ struct iova *iova = private_find_iova(iovad, mag->pfns[i]);
> >
> > -ÂÂÂÂÂÂÂ BUG_ON(!iova);
> > +ÂÂÂÂÂÂÂ if (WARN_ON(!iova))
> > +ÂÂÂÂÂÂÂÂÂÂÂ continue;
> > +
> > ÂÂÂÂÂÂÂÂ private_free_iova(iovad, iova);
> > ÂÂÂÂ }