lkml.org 
[lkml]   [2019]   [Dec]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH 1/3] iommu/vt-d: skip RMRR entries that fail the sanity check
    From
    Date


    On 12/16/2019 11:35 AM, Barret Rhoden wrote:
    > On 12/16/19 2:07 PM, Chen, Yian wrote:
    >>
    >>
    >> On 12/11/2019 11:46 AM, Barret Rhoden wrote:
    >>> RMRR entries describe memory regions that are DMA targets for devices
    >>> outside the kernel's control.
    >>>
    >>> RMRR entries that fail the sanity check are pointing to regions of
    >>> memory that the firmware did not tell the kernel are reserved or
    >>> otherwise should not be used.
    >>>
    >>> Instead of aborting DMAR processing, this commit skips these RMRR
    >>> entries.  They will not be mapped into the IOMMU, but the IOMMU can
    >>> still be utilized.  If anything, when the IOMMU is on, those devices
    >>> will not be able to clobber RAM that the kernel has allocated from
    >>> those
    >>> regions.
    >>>
    >>> Signed-off-by: Barret Rhoden <brho@google.com>
    >>> ---
    >>>   drivers/iommu/intel-iommu.c | 2 +-
    >>>   1 file changed, 1 insertion(+), 1 deletion(-)
    >>>
    >>> diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
    >>> index f168cd8ee570..f7e09244c9e4 100644
    >>> --- a/drivers/iommu/intel-iommu.c
    >>> +++ b/drivers/iommu/intel-iommu.c
    >>> @@ -4316,7 +4316,7 @@ int __init dmar_parse_one_rmrr(struct
    >>> acpi_dmar_header *header, void *arg)
    >>>       rmrr = (struct acpi_dmar_reserved_memory *)header;
    >>>       ret = arch_rmrr_sanity_check(rmrr);
    >>>       if (ret)
    >>> -        return ret;
    >>> +        return 0;
    >>>       rmrru = kzalloc(sizeof(*rmrru), GFP_KERNEL);
    >>>       if (!rmrru)
    >> Parsing rmrr function should report the error to caller. The behavior
    >> to response the error can be
    >> chose  by the caller in the calling stack, for example,
    >> dmar_walk_remapping_entries().
    >> A concern is that ignoring a detected firmware bug might have a
    >> potential side impact though
    >> it seemed safe for your case.
    >
    > That's a little difficult given the current code.  Once we are in
    > dmar_walk_remapping_entries(), the specific function (parse_one_rmrr)
    > is called via callback:
    >
    >     ret = cb->cb[iter->type](iter, cb->arg[iter->type]);
    >     if (ret)
    >         return ret;
    >
    > If there's an error of any sort, it aborts the walk.  Handling the
    > specific errors here is difficult, since we don't know what the errors
    > mean to the specific callback.  Is there some errno we can use that
    > means "there was a problem, but it's not so bad that you have to
    > abort, but I figured you ought to know"?  Not that I think that's a
    > good idea.
    >
    > The knowledge of whether or not a specific error is worth aborting all
    > DMAR functionality is best known inside the specific callback.  The
    > only handling to do is print a warning and either skip it or abort.
    >
    > I think skipping the entry for a bad RMRR is better than aborting
    > completely, though I understand if people don't like that.  It's
    > debatable.  By aborting, we lose the ability to use the IOMMU at all,
    > but we are still in a situation where the devices using the RMRR
    > regions might be clobbering kernel memory, right?  Using the IOMMU
    > (with no mappings for the bad RMRRs) would stop those devices from
    > clobbering memory.
    >
    > Regardless, I have two other patches in this series that could resolve
    > the problem for me and probably other people.  I'd just like at least
    > one of the three patches to get merged so that my machine boots when
    > the original commit f036c7fa0ab6 ("iommu/vt-d: Check VT-d RMRR region
    > in BIOS is reported as reserved") gets released.
    >
    when a firmware bug appears, the potential problem may beyond the scope
    of its visible impacts so that introducing a workaround in official
    implementation should be considered very carefully.

    If the workaround is really needed at this point, I would recommend
    adding a WARN_TAINT with TAINT_FIRMWARE_WORKAROUND, to tell the
    workaround is in the place.

    Thanks
    Yian

    > Thanks,
    >
    > Barret
    >

    \
     
     \ /
      Last update: 2019-12-17 20:20    [W:3.013 / U:0.312 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site