Re: [PATCH 3/6] VT-d: don't leak domid mapping on error path

[Jan Beulich <jbeulich@xxxxxxxx>]

On 12.11.2021 15:35, Roger Pau Monné wrote:
> On Fri, Nov 12, 2021 at 02:45:14PM +0100, Jan Beulich wrote:
>> On 12.11.2021 14:42, Roger Pau Monné wrote:
>>> On Fri, Nov 12, 2021 at 10:48:43AM +0100, Jan Beulich wrote:
>>>> While domain_context_mapping() invokes domain_context_unmap() in a sub-
>>>> case of handling DEV_TYPE_PCI when encountering an error, thus avoiding
>>>> a leak, individual calls to domain_context_mapping_one() aren't
>>>> similarly covered. Such a leak might persist until domain destruction.
>>>> Leverage that these cases can be recognized by pdev being non-NULL.
>>>
>>> Would it help to place the domid cleanup in domain_context_unmap_one,
>>> as that would then cover calls from domain_context_unmap and the
>>> failure path in domain_context_mapping_one.
>>
>> I don't think that would work (without further convolution), because of
>> the up to 3 successive calls in DEV_TYPE_PCI handling. Cleanup may happen
>> only on the first map's error path or after the last unmap.
> 
> Hm, I see. And AFAICT that's because some devices that get assigned to
> a guest iommu context are not actually assigned to the guest (ie:
> pdev->domain doesn't get set, neither the device is added to the
> per-domain list), which makes them invisible to
> any_pdev_behind_iommu.
> 
> I dislike that the domid is added in domain_context_mapping_one, while
> the cleanup is not done in domain_context_unmap_one, and that some
> devices context could be using the domain id without being assigned to
> the domain.

This all isn't really pretty, is it? As Andrew has been saying (I think
more than once), ideally we'd rewrite IOMMU code from scratch. But I
don't see anyone having enough spare time to actually do so ...

Jan