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.
As indicated I've done this.
@@ -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.
Just one more word on top of the prior discussion: Would you also
insist on an explicit check here (when ...
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
... none exists on basically all similar paths elsewhere) if the
IVRS mappings array hung off of struct amd_iommu as a plain pointer,
rather than being taken from a guaranteed populated (by this point
in time) radix tree slot?
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.
Juergen, since I didn't hear back from Andrew, would you be willing
to give a release ack on this series, as at this point I don't see
any good alternative to using the "begrudgingly A-by" give above?
