Page MenuHomeFreeBSD

Use atomic loads/stores when updating td->td_state
ClosedPublic

Authored by arichardson on Feb 10 2021, 11:14 AM.

Details

Summary

KCSAN complains about racy accesses in the locking code. Those races are
fine since they are inside a TD_SET_RUNNING() loop that expects the value
to be changed by another CPU.

Use relaxed atomic stores/loads to indicate that this variable can be
written/read by multiple CPUs at the same time. This will also prevent
the compiler from doing unexpected re-ordering.

Reported by: GENERIC-KCSAN

Test Plan

KCSAN no longer complains, kernel still runs fine.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 36854
Build 33743: arc lint + arc unit

Event Timeline

There are several other places which play with ->td_state, if rolling with atomic_* they all should be patched. Preferably there would be an atomic_int_t type or similar so that unpatched cases would fail to compile.

Maybe I should rephrase. The day is coming where someone will add the type and then this change will break if it does not take care of all uses.

sys/sys/proc.h
65

comments like this notoriously get stale

593

I think this should be TD_SET_STATE(td, state) and used by all of these and other places.

arichardson marked 2 inline comments as done.

Use macro accessors and wrap in a struct to prevent non-atomic accesses

arichardson added inline comments.
sys/sys/proc.h
593

Yes, much better. Thanks for the suggestion.

Assuming this runs and builds with tinderbox LGTM modulo the union comment.

sys/sys/proc.h
351

If this was added to induce compilation failures for plain td_state access it needs a comment.

I don't have an opinion one way or the other if this should be done here.

This revision is now accepted and ready to land.Feb 10 2021, 11:50 AM

Haven't done tinderbox yet, but grep shows there are no uses for other architecture:

sys/dev/twe/twe.c:    if (twe_get_param_1(sc, table, TWE_PARAM_UNITINFO_Status, &dr->td_state)) {
sys/dev/twe/twe_freebsd.c:      twe_describe_code(twe_table_unitstate, dr->td_state & TWE_PARAM_UNITSTATUS_MASK));
sys/dev/twe/twevar.h:    u_int8_t       td_state;
sys/sys/proc.h:     enum td_states {
sys/sys/proc.h: } td_state;         /* (t) thread state */
sys/sys/proc.h:#define  TD_GET_STATE(td)    atomic_load_int(&(td)->td_state.value)
sys/sys/proc.h:#define  TD_SET_STATE(td, state) atomic_store_int(&(td)->td_state.value, state)

Will add the comment before committing this.

sys/sys/proc.h
351

I would guess it'll also break some number of ports. Probably not many, but to me it's an argument against the name change.

546

I would define separate implementations for _KERNEL and !_KERNEL rather than pulling in atomic.h.

sys/sys/proc.h
351

Good point, I guess the wrapper struct could either be only for _KERNEL, or I just drop it entirely now that I've found all uses.

546

Sounds good, will do so later this week.

sys/sys/proc.h
351

It will break devel/mdb as mdb uses some weird hacks where it create mirror structures with a subset of members and depends on the CTF data for its own version and the CTF data in the kernel for the kernel's version to copy the right fields of the kernel structure into its mirrored structure. However, this depends on 1:1 correspondence of structures and is a PITA to deal with. (kgdb wouldn't care either way)

590–591
arichardson edited the summary of this revision. (Show Details)

Avoid atomics for !_KERNEL

This revision now requires review to proceed.Feb 17 2021, 11:14 AM
arichardson marked 2 inline comments as done.
arichardson marked 3 inline comments as done.
This revision is now accepted and ready to land.Feb 17 2021, 4:26 PM