Re: [Xen-devel] [PATCH v7 1/3] AMD/IOMMU: allocate one device table per PCI segment

[Jan Beulich <jbeulich@suse.com>]

On 04.10.2019 15:18, Andrew Cooper wrote:
> On 26/09/2019 15:28, Jan Beulich wrote:
>> @@ -1068,8 +1067,29 @@ static void * __init allocate_ppr_log(st
>>                                  IOMMU_PPR_LOG_DEFAULT_ENTRIES, "PPR Log");
>>  }
>>  
>> +/*
>> + * Within ivrs_mappings[] we allocate an extra array element to store
>> + * - segment number,
>> + * - device table.
>> + */
>> +#define IVRS_MAPPINGS_SEG(m) (m)[ivrs_bdf_entries].dte_requestor_id
>> +#define IVRS_MAPPINGS_DEVTAB(m) (m)[ivrs_bdf_entries].intremap_table
>> +
>> +static void __init free_ivrs_mapping(void *ptr)
>> +{
>> +    const struct ivrs_mappings *ivrs_mappings = ptr;
> 
> How absolutely certain are we that ptr will never be NULL?

As certain as we can be by never installing a NULL pointer into the
radix tree, and by observing that neither radix_tree_destroy() nor
radix_tree_node_destroy() would ever call the callback for a NULL
node.

> It might be better to rename this to radix_tree_free_ivrs_mappings() to
> make it clear who calls it, and also provide a hint as to why the
> parameter is void.

I'm not happy to add a radix_tree_ prefix; I'd be fine with adding
e.g. do_ instead, in case this provides enough of a hint for your
taste that this is actually a callback function.

>> @@ -1082,13 +1102,15 @@ static int __init amd_iommu_init_one(str
>>      if ( intr && !set_iommu_interrupt_handler(iommu) )
>>          goto error_out;
>>  
>> -    /* To make sure that device_table.buffer has been successfully allocated */
>> -    if ( device_table.buffer == NULL )
>> +    /* Make sure that the device table has been successfully allocated. */
>> +    ivrs_mappings = get_ivrs_mappings(iommu->seg);
>> +    if ( !IVRS_MAPPINGS_DEVTAB(ivrs_mappings) )
> 
> This is still going to crash with a NULL pointer deference in the case
> described by the comment.  (Then again, it may not crash, and hit
> userspace at the 64M mark.)
> 
> You absolutely need to check ivrs_mappings being non NULL before using
> IVRS_MAPPINGS_DEVTAB(), or perhaps roll the check into the macro.

I can only repeat what I've said in reply to your respective v6 remark:
We won't come here for an IOMMU which didn't have its ivrs_mappings
successfully allocated. You also seem to be mixing up this and the
device table allocation - the comment refers to the latter, while your
NULL deref concern is about the former. (If you go through the code
you'll find that we have numerous other places utilizing the fact that
get_ivrs_mappings() can't fail in cases like the one above.)

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel