iommu static analysis cleanup
ClosedPublic
Actions

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

Details

Reviewers

cem
kib
tychon

Commits

rS347645: iommu static analysis cleanup

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

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 24242
Build 23078: arc lint + arc unit

Event Timeline

rlibby created this revision.May 15 2019, 5:59 AM

Harbormaster completed remote builds in B24242: Diff 57407.May 15 2019, 5:59 AM

kib added inline comments.May 15 2019, 11:33 AM

sys/x86/iommu/intel_fault.c
294	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	Both these lines are fine.
sys/x86/iommu/intel_qi.c
428	This is fine.

rlibby added inline comments.May 15 2019, 3:14 PM

sys/x86/iommu/intel_fault.c
294	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	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	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.