btf: fix race due to concurrent read access #1357
Conversation
|
@eiffel-fl the locking ended up being more subtle than what you had in your PR. Can you give this a whirl? |
|
|
||
| if ok { | ||
| // Fast path: the type has been copied before. | ||
| return cpy |
There was a problem hiding this comment.
How come we didn't have this before? 🤔
There was a problem hiding this comment.
Because the same thing happens when entering the closure passed to modifyGraphPreorder.
lmb
force-pushed
the
btf-spec-concurrency
branch
2 times, most recently
from
accdd88
to
4456db1
Compare
Up until the introduction of lazy copying, reading from a Spec concurrently was safe. Now a read may trigger a copy and a write into the Spec, therefore causing a race on mutableTypes. Fix this by introducing a mutex which protects access to the mutable state. We need to be a bit careful here: copying in mutableTypes.add happens piecemeal, so we need to take a lock for the whole duration of modifyGraph. Signed-off-by: Lorenz Bauer <lmb@isovalent.com>
lmb
force-pushed
the
btf-spec-concurrency
branch
from
4456db1
to
61be69d
Compare
|
Hi!
We are not on the same timezone, hence I can only test it now but everything seems OK locally: $ sudo go test ./...
go: downloading github.com/hashicorp/go-multierror v1.1.1
go: downloading github.com/hashicorp/errwrap v1.1.0
ok github.com/inspektor-gadget/inspektor-gadget/pkg/gadgets/trace/capabilities/tracer 1.309sI also updated our PR to bump directly to So, thank you for quickly fixing this :D! Best regards. |
Up until the introduction of lazy copying, reading from a Spec concurrently was safe. Now a read may trigger a copy and a write into the Spec, therefore causing a race on mutableTypes.
Fix this by introducing a mutex which protects access to the mutable state. We need to be a bit careful here: copying in mutableTypes.add happens piecemeal, so we need to take a lock for the whole duration of modifyGraph.