Page MenuHomeFreeBSD

iommu static analysis cleanup
ClosedPublic

Authored by rlibby on May 15 2019, 5:59 AM.
Tags
None
Referenced Files
F80154421: D20263.diff
Thu, Mar 28, 3:31 PM
Unknown Object (File)
Wed, Mar 20, 10:58 PM
Unknown Object (File)
Feb 12 2024, 8:37 PM
Unknown Object (File)
Feb 2 2024, 12:33 PM
Unknown Object (File)
Dec 20 2023, 12:39 AM
Unknown Object (File)
Dec 12 2023, 10:04 PM
Unknown Object (File)
Nov 24 2023, 1:16 AM
Unknown Object (File)
Nov 14 2023, 2:11 AM
Subscribers

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 - subversion
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

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.

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.

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.

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.

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.