linux-kernel - Re: [RFC v3 14/21] iommu: introduce device fault data

Open Source and information security mailing list archives

Date:   Wed, 16 Jan 2019 19:33:38 +0100
From:   Auger Eric <eric.auger@...hat.com>
To:     Jean-Philippe Brucker <jean-philippe.brucker@....com>,
        Jacob Pan <jacob.jun.pan@...ux.intel.com>
Cc:     "yi.l.liu@...ux.intel.com" <yi.l.liu@...ux.intel.com>,
        "kevin.tian@...el.com" <kevin.tian@...el.com>,
        "alex.williamson@...hat.com" <alex.williamson@...hat.com>,
        "ashok.raj@...el.com" <ashok.raj@...el.com>,
        "kvm@...r.kernel.org" <kvm@...r.kernel.org>,
        Marc Zyngier <Marc.Zyngier@....com>,
        Will Deacon <Will.Deacon@....com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "iommu@...ts.linux-foundation.org" <iommu@...ts.linux-foundation.org>,
        Robin Murphy <Robin.Murphy@....com>,
        "kvmarm@...ts.cs.columbia.edu" <kvmarm@...ts.cs.columbia.edu>,
        "eric.auger.pro@...il.com" <eric.auger.pro@...il.com>
Subject: Re: [RFC v3 14/21] iommu: introduce device fault data

Hi Jean,

On 1/16/19 4:52 PM, Jean-Philippe Brucker wrote:
> On 14/01/2019 22:32, Jacob Pan wrote:
>>> [...]
>>>>> +/**
>>>>> + * struct iommu_fault - Generic fault data
>>>>> + *
>>>>> + * @type contains fault type
>>>>> + * @reason fault reasons if relevant outside IOMMU driver.
>>>>> + * IOMMU driver internal faults are not reported.
>>>>> + * @addr: tells the offending page address
>>>>> + * @fetch_addr: tells the address that caused an abort, if any
>>>>> + * @pasid: contains process address space ID, used in shared
>>>>> virtual memory
>>>>> + * @page_req_group_id: page request group index
>>>>> + * @last_req: last request in a page request group
>>>>> + * @pasid_valid: indicates if the PRQ has a valid PASID
>>>>> + * @prot: page access protection flag:
>>>>> + *	IOMMU_FAULT_READ, IOMMU_FAULT_WRITE
>>>>> + */
>>>>> +
>>>>> +struct iommu_fault {
>>>>> +	__u32	type;   /* enum iommu_fault_type */
>>>>> +	__u32	reason; /* enum iommu_fault_reason */
>>>>> +	__u64	addr;
>>>>> +	__u64	fetch_addr;
>>>>> +	__u32	pasid;
>>>>> +	__u32	page_req_group_id;
>>>>> +	__u32	last_req;
>>>>> +	__u32	pasid_valid;
>>>>> +	__u32	prot;
>>>>> +	__u32	access;  
>>>
>>> What does @access contain? Can it be squashed into @prot?
>>>
>> I agreed.
>>
>> how about this?
>> #define IOMMU_FAULT_VERSION_V1 0x1
>> struct iommu_fault {
>> 	__u16 version;
> 
> Right, but the version field becomes redundant when we present a batch
> of these to userspace, in patch 18 (assuming we don't want to mix fault
> structure versions within a batch... I certainly don't).>
> When introducing IOMMU_FAULT_VERSION_V2, in a distant future, I think we
> still need to support a userspace that uses IOMMU_FAULT_VERSION_V1. One
> strategy for this:
> 
> * We define structs iommu_fault_v1 (the current iommu_fault) and
>   iommu_fault_v2.
> * Userspace selects IOMMU_FAULT_VERSION_V1 when registering the fault
>   queue
> * The IOMMU driver fills iommu_fault_v2 and passes it to VFIO
> * VFIO does its best to translate this into a iommu_fault_v1 struct
> 
> So what we need now, is a way for userspace to tell the kernel which
> structure version it expects. I'm not sure we even need to pass the
> actual version number we're using back to userspace. Agreeing on one
> version at registration should be sufficient.

As we expose a VFIO region we can report its size, entry size, max
supported entry version, actual entry version in the region capabilities.

Conveying the version along with the eventfd at registration time will
require to introduce a new flag at vfio_irq_set level (if we still plan
to use this VFIO_DEVICE_SET_IRQS API) but that should be feasible.
> 
>> 	__u16 type;
>> 	__u32 reason;
>> 	__u64 addr;
> 
> I'm in favor of keeping @fetch_addr as well, it can contain useful
> information. For example, while attempting to translate an IOVA
> 0xfffff000, the IOMMU can't find the PASID table that we installed with
> address 0xdead - the guest passed an invalid address to
> bind_pasid_table(). We can then report 0xfffff000 in @addr, and 0xdead
> in @fetch_addr.
agreed
> 
>> 	__u32 pasid;
>> 	__u32 page_req_group_id;
>> 	__u32 last_req : 1;
>> 	__u32 pasid_valid : 1;
> 
> Agreed, with some explicit padding or combined as a @flag field. In fact
> if we do add the @fetch_addr field, I think we need a bit that indicates
> its validity as well.
Can't we simply state fetch_addr is valid for
IOMMU_FAULT_REASON_PASID_FETCH, IOMMU_FAULT_REASON_DEVICE_CONTEXT_FETCH,
IOMMU_FAULT_REASON_WALK_EABT which maps to its usage in SMMU spec.

Thanks

Eric

> 
> Thanks,
> Jean
> 
>> 	__u32 prot;
>> 	__u64 device_private[2];
>> 	__u8 padding[48];
>> };
>>
>>
>>> Thanks,
>>> Jean
>>>
>>>> relocated to uapi, Yi can you confirm?
>>>> 	__u64 device_private[2];
>>>>   
>>>>> +};
>>>>>  #endif /* _UAPI_IOMMU_H */  
>>>>
>>>> _______________________________________________
>>>> iommu mailing list
>>>> iommu@...ts.linux-foundation.org
>>>> https://lists.linuxfoundation.org/mailman/listinfo/iommu
>>>>   
>>>
>>
>> [Jacob Pan]
>> _______________________________________________
>> iommu mailing list
>> iommu@...ts.linux-foundation.org
>> https://lists.linuxfoundation.org/mailman/listinfo/iommu
>>
> 
> _______________________________________________
> kvmarm mailing list
> kvmarm@...ts.cs.columbia.edu
> https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
> 

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