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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 04.10.2019 19:28, Andrew Cooper wrote:
> On 04/10/2019 14:30, Jan Beulich wrote:
>> 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.
> 
> How about a _callback() suffix?  I'm looking to make it obvious that you
> code shouldn't simply call it directly.

Well, okay, done.

> A "do_" prefix, in particular, provides no useful information to the reader.

Depends, I guess: There are a couple of places where we already use
such naming. People aware of this may make this implication.

>>>> @@ -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.
> 
> Right, but to a first approximation, I don't care.  I can picture
> exactly what Coverity will say about this, in that radix_tree_lookup()
> may return NULL, and it is used here unconditionally where in most other
> contexts, the pointer gets checked before use.

Except that, as per your stats below, it's not anywhere near "most".

>> 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.)
> 
> The existing code being terrible isn't a reasonable justification for
> adding to the mess.
> 
> It appears we have:
> 
> 1x assert not null
> 14x blind use
> 3x check
> 
> which isn't a great statement about the quality of the code.

If any of the "blind" uses were indeed on a path where this could
in theory be NULL, I'd agree. The patch we're discussing here
definitely doesn't fall into this category.

> Seeing as we are pushed to the deadline for 4.13, begrudgingly A-by
> (preferably with the _callback() suffix), but I'm still not happy with
> the overall quality of the code.  At least it isn't getting
> substantially worse as a consequence here.

I appreciate the ack, but I think I'd prefer to not make use of it
if at all possible under these conditions. Instead I'd like us to
reach some common ground here. Seeing that we're past the deadline
already, Jürgen's release ack will now be needed anyway. Jürgen -
would you be fine with settling on this taking a few more days,
and then still allow in this series? Or is trying to soon find a
resolution here pointless as you'd rather not see this go in
anymore?

As to what (if anything) to change - I'd be fine with adding an
assertion, but I don't think that would buy us much (considering
non-debug builds). What I'm not happy about is adding checks just
for the sake of doing so. Applying the underlying thinking of
"don't trust ourselves" to the entire code base would imo result
in severe crippling of the sources (nevertheless I agree that
there are cases, when connections are less obvious, where adding
extra checks is actually useful).

As to the stats you provide and your implication on code
quality: What's wrong with code e.g. utilizing the knowledge
that once it holds a struct amd_iommu in its hands, it can rely
on there being a respective IVRS mappings entry? The cases where
the return value of get_ivrs_mappings() gets checked are
- to determine whether the mapping needs allocating
  (alloc_ivrs_mappings()),
- to determine whether there's an IOMMU for a device in the
  first place (find_iommu_for_device()),
- redundant verification after an IOMMU has already been
  determined for a device (amd_iommu_add_device()).
I.e. the first two are justified, and to arrange for a consistent
code base the 3rd one should be considered to drop again (I think
this is an instance I added recently, not having realized (yet)
that the implication is being utilized everywhere else.

Jan

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