Fix cross-shard allocator manipulation #7351

travisdowns · 2022-11-17T09:02:46Z

This patch series prevents cross-shard manipulation of the allocator state which may cause segfaults and assertion failures.

The main problem is that the destructor for allocation_units maintains a pointer to the state from which it was created, which when moved onto another shard will do a call back to the original shard on destruction: a race condition. Now we use foreign pointer to avoid this: the destructor will be called on the original shard.

Fixes #5558.

Backports Required

UX Changes

Release Notes

Bug Fixes

Fix cross-shard allocator manipulation

Const member prevents copy and move assignment, but copy and move assignment seem to have reasonable semantics (the destination shard id is replaced by the source) and we need it for allocation_units oncore tracking. Issue redpanda-data#5558.

Even though they ave movable, allocation_units objects must be destroyed on the same core the *original* allocation was made on, as they contain an embedded pointer back to the allocation state which lives on that core, since to do otherwise would be a cross-shard race-condition. Enforce this condition in debug mode using oncore checker.

Return the allocation_units wrapped in a foreign pointer, since they need to be freed on the same core on which they were allocated. Fixes redpanda-data#5558.

Checks at the top of most functions to ensure that the core of the caller is the same as the home core of the state, only in debug builds.

graphcareful · 2022-11-17T14:00:03Z

Nice work I added myself as a reviewer just to be kept in the loop here

emaxerrno · 2022-11-17T16:03:33Z

This is relatively clean. Any reason is draft.

travisdowns · 2022-11-17T22:54:50Z

ci-failure is #6870

travisdowns · 2022-11-17T22:56:40Z

@emaxerrno wrote:

This is relatively clean. Any reason is draft.

I just didn't have time to fill out the cover letter yet but I wanted to get a CI run in in the meantime and maybe get an eye on it.

I've marked it as ready for review now.

travisdowns · 2022-11-17T22:59:07Z

src/v/bytes/oncore.h

@@ -29,7 +29,7 @@ class oncore final {
    void verify_shard_source_location(const char* file, int linenum) const;

 private:
-    const shard_id_type _owner_shard;


@dotnwat you added this const in a recent compile-time related change, but is it necessary? It prevents assignment but I think assignment basically DTRT at least in the case of allocation_units the destination will be assigned the internals which still point to the shard associated with the source so the oncore should also be assigned with the shard of the source.

it isn't necessary, but i'm unaware of any case in which the owner shard is not the same as the shard that created the oncore object (and could make that write-once assignment). it almost seems like if it is being manually assigned or changed, then it implies that some code is doing manual tracking of core owner, which defeats the sort of checks we wanted in the first place.

i'll take a closer look at this PR to understand what is happening.

@dotnwat - right, it may be true that "the owner shard is not the same as the shard that created the oncore object" but making the member const is implies something even stricter: "this object can never be assigned". Now I think you can argue that assignment of an oncore object itself doesn't make much sense since the only state it has should not change, but this also means any object that contains an oncore member can't be assigned (at least not without overriding the operator= and manually assigning all the fields except that one, ugh), and that may be an entirely valid operation.

So one way to keep similar semantics without preventing assignment operator generation could be to assert in the oncore assignment operator that the LHS and RHS shard ID are the same.

jcsp · 2022-11-21T10:28:30Z

nit: please can you use the component: foo bar style for commit message first lines, makes it much easier to browse history later

jcsp · 2022-11-21T10:44:49Z

Do we like this as a cause for #7343 ? It seems odd that the issue would manifest on upgrade if this is a long-standing bug in Redpanda.

piyushredpanda · 2022-11-21T17:56:06Z

/backport v22.3.x

piyushredpanda · 2022-11-21T17:56:20Z

/backport v22.2.x

vbotbuildovich · 2022-11-21T17:56:43Z

Oops! Something went wrong.

Workflow run logs.

vbotbuildovich · 2022-11-21T17:57:11Z

Failed to run cherry-pick command. I executed the below command:

git cherry-pick -x c64fdbbc8d7423050d2783c0b698af17c1f03b00 0565bf67dc24a0f30fd28da770891ec64e1aab09 e5dad45b37b752ba240af4c7f06af099d56e21aa 413cbe30dd8a2c94c4025a43bc43a820789de697 44c0dc71305676eed3e66fc51f59330968bf3e2f

Workflow run logs.

andrewhsu · 2022-11-21T21:46:44Z

The attempt to backport to v22.2.x branch failed because this PR modifies src/v/bytes/oncore.h which is a file that does not currently exist on v22.2.x branch. May require backport of PR #6986 which first introduces that file. I'm not entirely sure that's the right thing to do. Plus attempt to backport failed with simple cherry pick: #6986 (comment)

@travisdowns @dotnwat or anybody else who knows the innards of this codebase...need advice.

graphcareful · 2022-11-21T22:25:33Z

/backport v22.3.x

andrewhsu · 2022-11-21T22:34:17Z

notes from conversation with @dlex regarding backport to v22.2.x branch:

to backport 7351, the first commit in it should be dropped / or the other four should be cherry-picked. That is (apparently) an unrelated change that is causing the backport failure.

dotnwat · 2022-11-23T05:48:02Z

@andrewhsu i don't think you need to backport 6986. this PR makes a one line change to that oncore file, but in 22.2.x that file exists in a different location. you could even drop the change to that file entirely, just document it in the PR as a conflict.

andrewhsu · 2022-11-23T14:59:27Z

fyi i added release notes section to the pr body; it was empty

travisdowns · 2022-11-23T21:06:57Z

Thanks all for the help here.

Just to clarify, the Remove const from shard_id in oncore change was related and required for this series of changes in dev, because there the shard ID in oncore object is const. It is not needed at all in 22.2.x because that member is not const there (the same series that added this header added the constness).

github-actions bot added the area/redpanda label Nov 17, 2022

travisdowns and others added 3 commits November 17, 2022 01:05

Remove const from shard_id in oncore

c64fdbb

Const member prevents copy and move assignment, but copy and move assignment seem to have reasonable semantics (the destination shard id is replaced by the source) and we need it for allocation_units oncore tracking. Issue redpanda-data#5558.

Better output in allocation_node

0565bf6

travisdowns force-pushed the td-5558-allocator-assert branch from 4844d09 to c61631c Compare November 17, 2022 09:06

travisdowns added 2 commits November 17, 2022 01:46

Update partition_allocator to return foreign ptr

413cbe3

Return the allocation_units wrapped in a foreign pointer, since they need to be freed on the same core on which they were allocated. Fixes redpanda-data#5558.

Add oncore verification to allocation_state

44c0dc7

Checks at the top of most functions to ensure that the core of the caller is the same as the home core of the state, only in debug builds.

travisdowns force-pushed the td-5558-allocator-assert branch from c61631c to 44c0dc7 Compare November 17, 2022 09:46

graphcareful self-requested a review November 17, 2022 13:59

travisdowns marked this pull request as ready for review November 17, 2022 22:55

travisdowns requested a review from dotnwat November 17, 2022 22:57

travisdowns commented Nov 17, 2022

View reviewed changes

piyushredpanda added this to the v22.3.4 milestone Nov 17, 2022

kargh mentioned this pull request Nov 18, 2022

Cannot Start Redpanda After Upgrading from 22.2.7 to 22.3.2 (or 22.3.1) #7343

Closed

travisdowns requested a review from dlex November 18, 2022 23:51

mmaslankaprv self-requested a review November 21, 2022 07:04

mmaslankaprv approved these changes Nov 21, 2022

View reviewed changes

jcsp approved these changes Nov 21, 2022

View reviewed changes

piyushredpanda merged commit 74cfa67 into redpanda-data:dev Nov 21, 2022

This was referenced Nov 21, 2022

[v22.3.x] Asserts and segmentation faults in topic creation under load #7414

Closed

[v22.3.x] Fix cross-shard allocator manipulation #7415

Merged

travisdowns mentioned this pull request Nov 22, 2022

Intermittent failure in kafka_server_fixture read_from_ntp_max_bytes #7438

Closed

dotnwat mentioned this pull request Nov 23, 2022

Reduce iobuf.h impact on intermediate object sizes #6986

Merged

6 tasks

jcsp mentioned this pull request Nov 28, 2022

Redpanda pods are failed after trying to remove all topics #7531

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix cross-shard allocator manipulation #7351

Fix cross-shard allocator manipulation #7351

travisdowns commented Nov 17, 2022 •

edited by andrewhsu

Loading

graphcareful commented Nov 17, 2022

emaxerrno commented Nov 17, 2022

travisdowns commented Nov 17, 2022

travisdowns commented Nov 17, 2022

travisdowns Nov 17, 2022

dotnwat Nov 23, 2022 •

edited

Loading

travisdowns Nov 23, 2022

jcsp commented Nov 21, 2022

jcsp commented Nov 21, 2022

piyushredpanda commented Nov 21, 2022

piyushredpanda commented Nov 21, 2022

vbotbuildovich commented Nov 21, 2022

vbotbuildovich commented Nov 21, 2022

andrewhsu commented Nov 21, 2022

graphcareful commented Nov 21, 2022

andrewhsu commented Nov 21, 2022

dotnwat commented Nov 23, 2022

andrewhsu commented Nov 23, 2022

travisdowns commented Nov 23, 2022

Fix cross-shard allocator manipulation #7351

Fix cross-shard allocator manipulation #7351

Conversation

travisdowns commented Nov 17, 2022 • edited by andrewhsu Loading

Backports Required

UX Changes

Release Notes

Bug Fixes

graphcareful commented Nov 17, 2022

emaxerrno commented Nov 17, 2022

travisdowns commented Nov 17, 2022

travisdowns commented Nov 17, 2022

travisdowns Nov 17, 2022

Choose a reason for hiding this comment

dotnwat Nov 23, 2022 • edited Loading

Choose a reason for hiding this comment

travisdowns Nov 23, 2022

Choose a reason for hiding this comment

jcsp commented Nov 21, 2022

jcsp commented Nov 21, 2022

piyushredpanda commented Nov 21, 2022

piyushredpanda commented Nov 21, 2022

vbotbuildovich commented Nov 21, 2022

vbotbuildovich commented Nov 21, 2022

andrewhsu commented Nov 21, 2022

graphcareful commented Nov 21, 2022

andrewhsu commented Nov 21, 2022

dotnwat commented Nov 23, 2022

andrewhsu commented Nov 23, 2022

travisdowns commented Nov 23, 2022

travisdowns commented Nov 17, 2022 •

edited by andrewhsu

Loading

dotnwat Nov 23, 2022 •

edited

Loading