Page MenuHomeFreeBSD

iommu static analysis cleanup
ClosedPublic

Authored by rlibby on May 15 2019, 5:59 AM.

Details

Summary

A static analyzer complained about a couple instances of checking a
variable against NULL after already having dereferenced it.

  • dmar_gas_alloc_region: remove the tautological NULL checks
  • dmar_release_resources / dmar_fini_fault_log: don't deref unit->regs unless initialized.

And while here, fix an inverted initialization check in dmar_fini_qi.

Test Plan

So far just make buildkernel, but I can put this through a quick
internal sanity check. The dmar_release_resources path only executes on
dmar_attach error, which is to say, rarely.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

rlibby created this revision.May 15 2019, 5:59 AM
kib added inline comments.May 15 2019, 11:33 AM
sys/x86/iommu/intel_fault.c
294 ↗(On Diff #57407)

Why this move is needed ? Disabling interrupts regardless of the init status seems to be innocent and might be even useful.

sys/x86/iommu/intel_gas.c
563 ↗(On Diff #57407)

Both these lines are fine.

sys/x86/iommu/intel_qi.c
428 ↗(On Diff #57407)

This is fine.

rlibby added inline comments.May 15 2019, 3:14 PM
sys/x86/iommu/intel_fault.c
294 ↗(On Diff #57407)

What the static analyzer noticed is that this depends on unit->regs
being initialized, but there's a check for unit->regs == NULL after this
point in dmar_release_resources().

That part is actually okay though, since in the current code,
dmar_release_resources() is only called after unit->regs is initialized.
(But perhaps it was contemplated that unit->regs could be NULL, else why
have the check?)

However another issue is that this also uses the unit->lock (DMAR_LOCK()),
but there are call sites where the mutex is not yet initialized. We
could move up the mutex initialization.

I'm happy to take your suggestion here, or to leave it to you.

tychon added inline comments.May 15 2019, 4:58 PM
sys/x86/iommu/intel_fault.c
294 ↗(On Diff #57407)

If you wanted to you ... you could move the if (unit->regs != NULL) { earlier in dmar_release_resources(). I believe that will address the analyzer's concern and make it clear that regs is assumed to be initialized very early on.

rlibby added inline comments.May 15 2019, 5:06 PM
sys/x86/iommu/intel_fault.c
294 ↗(On Diff #57407)

If you wanted to you ... you could move the if (unit->regs != NULL) { earlier in dmar_release_resources(). I believe that will address the analyzer's concern and make it clear that regs is assumed to be initialized very early on.

Yes, that would quiet the analyzer, but it wouldn't address the mutex initialization issue.

kib accepted this revision.May 15 2019, 6:31 PM
This revision is now accepted and ready to land.May 15 2019, 6:31 PM
This revision was automatically updated to reflect the committed changes.