client: fix memory leak in Client::CRF_iofinish::complete by synarete · Pull Request #60507 · ceph/ceph

synarete · 2024-10-27T09:57:21Z

Commit 1210ddf ("Client: Add non-blocking helper classes") introduced Client::C_Read_Finisher Context object for async READ operations, but it has a read-after-free bug which may cause memory leak when calling libcephf's non-blocking ceph_ll_nonblocking_readv_writev API with async READ:

ceph_ll_nonblocking_readv_writev (READ)
Client::ll_preadv_pwritev
...
Client::_read_async
Context::complete
Client::CRF_iofinish::complete
Client::CRF_iofinish::finish
CRF->finish_io()
Client::C_Read_Finisher::finish_io
...
delete this; // frees CRF_iofinish->CRF
if (CRF->iofinished) // use-after-free of CRF
delete this; // may not get here

A possible memory leak depends on timing and race with other thread allocation which alters the memory address of CRF->iofinished to false, thus skipping the last delete operation.

The check of if (CRF->iofinished) is unnecessary: it is always set to true upon calling CRF->finish_io(). Removed.

Contribution Guidelines

To sign and title your commits, please refer to Submitting Patches to Ceph.
If you are submitting a fix for a stable branch (e.g. "quincy"), please refer to Submitting Patches to Ceph - Backports for the proper workflow.
When filling out the below checklist, you may click boxes directly in the GitHub web UI. When entering or editing the entire PR message in the GitHub web UI editor, you may also select a checklist item by adding an x between the brackets: [x]. Spaces and capitalization matter when checking off items this way.

Checklist

Tracker (select at least one)
- References tracker ticket
- Very recent bug; references commit where it was introduced
- New feature (ticket optional)
- Doc update (no ticket needed)
- Code cleanup (no ticket needed)
Component impact
- Affects Dashboard, opened tracker ticket
- Affects Orchestrator, opened tracker ticket
- No impact that needs to be tracked
Documentation (select at least one)
- Updates relevant documentation
- No doc update is appropriate
Tests (select at least one)
- Includes unit test(s)
- Includes integration test(s)
- Includes bug reproducer
- No tests

Show available Jenkins commands

jenkins retest this please
jenkins test classic perf
jenkins test crimson perf
jenkins test signed
jenkins test make check
jenkins test make check arm64
jenkins test submodules
jenkins test dashboard
jenkins test dashboard cephadm
jenkins test api
jenkins test docs
jenkins render docs
jenkins test ceph-volume all
jenkins test ceph-volume tox
jenkins test windows
jenkins test rook e2e

src/client/Client.h

dparmar18 · 2024-10-30T11:45:11Z

from what I see is we might also need to audit C_Read_Sync_NonBlocking which has a similar override of complete() in the same commit mentioned in description 1210ddf#diff-7a209b0f3a8f5e5b20525fb85acc702a4b1ec7d568dcf3e88ebc826fa2104e8aR1346-R1352

synarete · 2024-10-30T12:19:54Z

from what I see is we might also need to audit C_Read_Sync_NonBlocking which has a similar override of complete() in the same commit mentioned in description 1210ddf#diff-7a209b0f3a8f5e5b20525fb85acc702a4b1ec7d568dcf3e88ebc826fa2104e8aR1346-R1352

@dparmar18 As far as I can tell from reading the code of Client::C_Read_Sync_NonBlocking::finish() and Client::C_Read_Sync_NonBlocking::complete() it does not have the same use-after-free case as fixed by this PR (the statement if (fini) delete this; uses private member which is still valid at this point). Please correct me if I am wrong here.

dparmar18 · 2024-10-30T12:35:19Z

from what I see is we might also need to audit C_Read_Sync_NonBlocking which has a similar override of complete() in the same commit mentioned in description 1210ddf#diff-7a209b0f3a8f5e5b20525fb85acc702a4b1ec7d568dcf3e88ebc826fa2104e8aR1346-R1352

@dparmar18 As far as I can tell from reading the code of Client::C_Read_Sync_NonBlocking::finish() and Client::C_Read_Sync_NonBlocking::complete() it does not have the same use-after-free case as fixed by this PR (the statement if (fini) delete this; uses private member which is still valid at this point). Please correct me if I am wrong here.

Yeah but since @batrick mentioned, im wondering if that part is really required since the base class should take care of it, no?

synarete · 2024-10-30T12:53:00Z

from what I see is we might also need to audit C_Read_Sync_NonBlocking which has a similar override of complete() in the same commit mentioned in description 1210ddf#diff-7a209b0f3a8f5e5b20525fb85acc702a4b1ec7d568dcf3e88ebc826fa2104e8aR1346-R1352

@dparmar18 As far as I can tell from reading the code of Client::C_Read_Sync_NonBlocking::finish() and Client::C_Read_Sync_NonBlocking::complete() it does not have the same use-after-free case as fixed by this PR (the statement if (fini) delete this; uses private member which is still valid at this point). Please correct me if I am wrong here.

Yeah but since @batrick mentioned, im wondering if that part is really required since the base class should take care of it, no?

The code of Client::C_Read_Sync_NonBlocking::finish() has a case in which it may return without setting fini to false, thus the if (fini) in Client::C_Read_Sync_NonBlocking::complete() will not delete this. However, I do not know if it OK or (another) bug.

dparmar18 · 2024-10-30T14:15:58Z

from what I see is we might also need to audit C_Read_Sync_NonBlocking which has a similar override of complete() in the same commit mentioned in description 1210ddf#diff-7a209b0f3a8f5e5b20525fb85acc702a4b1ec7d568dcf3e88ebc826fa2104e8aR1346-R1352

@dparmar18 As far as I can tell from reading the code of Client::C_Read_Sync_NonBlocking::finish() and Client::C_Read_Sync_NonBlocking::complete() it does not have the same use-after-free case as fixed by this PR (the statement if (fini) delete this; uses private member which is still valid at this point). Please correct me if I am wrong here.

Yeah but since @batrick mentioned, im wondering if that part is really required since the base class should take care of it, no?

The code of Client::C_Read_Sync_NonBlocking::finish() has a case in which it may return without setting fini to false, thus the if (fini) in Client::C_Read_Sync_NonBlocking::complete() will not delete this. However, I do not know if it OK or (another) bug.

Okay, I guess it might be needed for Client::C_Read_Sync_NonBlocking::retry() to make sure the retried calls don't segfault.

ffilz · 2024-10-30T14:23:47Z

from what I see is we might also need to audit C_Read_Sync_NonBlocking which has a similar override of complete() in the same commit mentioned in description 1210ddf#diff-7a209b0f3a8f5e5b20525fb85acc702a4b1ec7d568dcf3e88ebc826fa2104e8aR1346-R1352

@dparmar18 As far as I can tell from reading the code of Client::C_Read_Sync_NonBlocking::finish() and Client::C_Read_Sync_NonBlocking::complete() it does not have the same use-after-free case as fixed by this PR (the statement if (fini) delete this; uses private member which is still valid at this point). Please correct me if I am wrong here.

Yeah but since @batrick mentioned, im wondering if that part is really required since the base class should take care of it, no?

The code of Client::C_Read_Sync_NonBlocking::finish() has a case in which it may return without setting fini to false, thus the if (fini) in Client::C_Read_Sync_NonBlocking::complete() will not delete this. However, I do not know if it OK or (another) bug.

Okay, I guess it might be needed for Client::C_Read_Sync_NonBlocking::retry() to make sure the retried calls don't segfault.

Yea, I think that one really is needed, or if we can find a different way to resolve the Context lifetime.

dparmar18 · 2024-10-30T15:21:07Z

from what I see is we might also need to audit C_Read_Sync_NonBlocking which has a similar override of complete() in the same commit mentioned in description 1210ddf#diff-7a209b0f3a8f5e5b20525fb85acc702a4b1ec7d568dcf3e88ebc826fa2104e8aR1346-R1352

@dparmar18 As far as I can tell from reading the code of Client::C_Read_Sync_NonBlocking::finish() and Client::C_Read_Sync_NonBlocking::complete() it does not have the same use-after-free case as fixed by this PR (the statement if (fini) delete this; uses private member which is still valid at this point). Please correct me if I am wrong here.

Yeah but since @batrick mentioned, im wondering if that part is really required since the base class should take care of it, no?

The code of Client::C_Read_Sync_NonBlocking::finish() has a case in which it may return without setting fini to false, thus the if (fini) in Client::C_Read_Sync_NonBlocking::complete() will not delete this. However, I do not know if it OK or (another) bug.

Okay, I guess it might be needed for Client::C_Read_Sync_NonBlocking::retry() to make sure the retried calls don't segfault.

Yea, I think that one really is needed, or if we can find a different way to resolve the Context lifetime.

since this bug was caused due to some race where the onfinished's memory addr was altered to the inverse value, im curious if the same can happen for the retries, what if the fini gets altered to true, the context gets deleted and we have a UAF. I think we should refactor the case to some robust solution. Maybe manage the retry context in a thread like finisher thread (ceph/src/common/Finisher.h)?

vshankar · 2024-11-29T05:57:02Z

I'm suspecting this change to be causing failures reported in #60411 (comment)

Will update when I verify that.

github-actions · 2025-01-28T07:01:37Z

This pull request has been automatically marked as stale because it has not had any activity for 60 days. It will be closed if no further activity occurs for another 30 days.
If you are a maintainer or core committer, please follow-up on this pull request to identify what steps should be taken by the author to move this proposed change forward.
If you are the author of this pull request, thank you for your proposed contribution. If you believe this change is still appropriate, please ensure that any feedback has been addressed and ask for a code review.

github-actions · 2025-02-27T08:02:31Z

This pull request has been automatically closed because there has been no activity for 90 days. Please feel free to reopen this pull request (or open a new one) if the proposed change is still appropriate. Thank you for your contribution!

vshankar · 2025-02-27T08:18:26Z

@dparmar18 This change need to be reviewed and testes. @dparmar18 Mind having a look?

I will get to it later today.

dparmar18 · 2025-02-27T08:55:30Z

@dparmar18 This change need to be reviewed and testes. @dparmar18 Mind having a look?

I will get to it later today.

ACK

dparmar18 · 2025-02-28T11:45:40Z

This patch looks good to me but as @ffilz mentioned in #60507 (comment) that if in case of short reads finish_io might need to return without setting iofinished to true. If that is implemented, the need to have this overridden complete() might exist.

dparmar18 · 2025-02-28T11:50:43Z

I feel we should have a better mechanism of managing context lifetime i.e. independent of fini (in C_Read_Sync_NonBlocking) or iofinished

ffilz · 2025-03-05T17:40:00Z

I feel we should have a better mechanism of managing context lifetime i.e. independent of fini (in C_Read_Sync_NonBlocking) or iofinished

Yea, lifetime seems mucky... Not sure best way to fix it.

github-actions · 2025-05-04T18:02:10Z

This pull request has been automatically marked as stale because it has not had any activity for 60 days. It will be closed if no further activity occurs for another 30 days.
If you are a maintainer or core committer, please follow-up on this pull request to identify what steps should be taken by the author to move this proposed change forward.
If you are the author of this pull request, thank you for your proposed contribution. If you believe this change is still appropriate, please ensure that any feedback has been addressed and ask for a code review.

dparmar18 · 2025-05-07T10:19:11Z

jenkins test make check arm64

anoopcs9

@synarete Sorry to mention at this stage but can you please fix the commit message by removing the unwanted text and duplicate Signed-off-by tag?

dparmar18 · 2025-06-09T06:32:23Z

jenkins test make check arm64

Commit 1210ddf ("Client: Add non-blocking helper classes") introduced Client::C_Read_Finisher Context object for async READ operations, but it has a read-after-free bug which may cause memory leak when calling libcephf's non-blocking ceph_ll_nonblocking_readv_writev API with async READ: ceph_ll_nonblocking_readv_writev (READ) Client::ll_preadv_pwritev ... Client::_read_async Context::complete Client::CRF_iofinish::complete Client::CRF_iofinish::finish CRF->finish_io() Client::C_Read_Finisher::finish_io ... delete this; // frees CRF_iofinish->CRF if (CRF->iofinished) // use-after-free of CRF delete this; // may not get here A possible memory leak depends on timing and race with other thread allocation which alters the memory address of CRF->iofinished to false, thus skipping the last delete operation. The check of `if (CRF->iofinished)` is unnecessary: it is always set to true upon calling CRF->finish_io(). Thus, there is no need to have the override function Client::CRF_iofinish::complete() as it now has the same logic as Context::complete(). Removed. Signed-off-by: Shachar Sharon <[email protected]>

vshankar · 2025-06-17T08:55:43Z

Testing update: https://tracker.ceph.com/issues/71514#note-12

The jobs have picked up crimson flavour for OSD and is causing lots of failures. Not sure how did that happen. This is under resolution and we are talking to crimson team. Standby!

vshankar · 2025-06-18T04:53:42Z

https://tracker.ceph.com/projects/cephfs/wiki/QA_main_2025#wip-vshankar-testing-20250613134551-debug

vshankar · 2025-06-18T05:06:30Z

@synarete I will backport this fix to squid/tentacle branch with other client side fixes.

github-actions bot added the cephfs Ceph File System label Oct 27, 2024

synarete added wip-ssharon-test2 synarete testing and removed cephfs Ceph File System labels Oct 27, 2024

synarete marked this pull request as draft October 27, 2024 10:57

synarete force-pushed the ss-cephfs-asyncio-use-after-free-bugfix branch from d98aa32 to b479023 Compare October 28, 2024 08:19

github-actions bot added the cephfs Ceph File System label Oct 28, 2024

synarete marked this pull request as ready for review October 28, 2024 08:19

synarete requested review from ffilz and vshankar October 28, 2024 08:24

vshankar requested a review from kotreshhr October 28, 2024 09:25

kotreshhr self-assigned this Oct 28, 2024

batrick requested changes Oct 28, 2024

View reviewed changes

src/client/Client.h Outdated Show resolved Hide resolved

src/client/Client.h Outdated Show resolved Hide resolved

ffilz requested a review from dparmar18 October 28, 2024 14:28

synarete force-pushed the ss-cephfs-asyncio-use-after-free-bugfix branch from b479023 to a655d80 Compare October 29, 2024 10:13

synarete requested a review from batrick October 29, 2024 10:15

dparmar18 self-assigned this Oct 29, 2024

batrick approved these changes Oct 29, 2024

View reviewed changes

src/client/Client.h Show resolved Hide resolved

batrick self-assigned this Oct 30, 2024

synarete force-pushed the ss-cephfs-asyncio-use-after-free-bugfix branch 4 times, most recently from e5d2a37 to d26c19d Compare November 5, 2024 11:55

synarete force-pushed the ss-cephfs-asyncio-use-after-free-bugfix branch from d26c19d to 0c27fe3 Compare November 10, 2024 09:28

kotreshhr mentioned this pull request Nov 28, 2024

client: Fix a deadlock when osd is full #60411

Merged

14 tasks

vshankar removed the wip-vshankar-testing label Nov 29, 2024

github-actions bot added the stale label Jan 28, 2025

github-actions bot closed this Feb 27, 2025

dparmar18 reopened this Feb 27, 2025

dparmar18 removed the stale label Feb 27, 2025

github-actions bot added the stale label May 4, 2025

dparmar18 removed the stale label May 7, 2025

vshankar added the wip-vshankar-testing4 label Jun 9, 2025

vshankar approved these changes Jun 9, 2025

View reviewed changes

anoopcs9 requested changes Jun 9, 2025

View reviewed changes

synarete force-pushed the ss-cephfs-asyncio-use-after-free-bugfix branch from 5b89503 to 6dc7756 Compare June 13, 2025 11:34

vshankar merged commit a75c560 into ceph:main Jun 18, 2025
13 checks passed

vshankar removed the wip-ssharon-test2 synarete testing label Jun 18, 2025

vshankar removed the wip-vshankar-testing4 label Jun 18, 2025

Conversation

synarete commented Oct 27, 2024 • edited by rishabh-d-dave Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution Guidelines

Checklist

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dparmar18 commented Oct 30, 2024

Uh oh!

synarete commented Oct 30, 2024

Uh oh!

dparmar18 commented Oct 30, 2024

Uh oh!

synarete commented Oct 30, 2024

Uh oh!

dparmar18 commented Oct 30, 2024

Uh oh!

ffilz commented Oct 30, 2024

Uh oh!

dparmar18 commented Oct 30, 2024

Uh oh!

vshankar commented Nov 29, 2024

Uh oh!

github-actions bot commented Jan 28, 2025

Uh oh!

github-actions bot commented Feb 27, 2025

Uh oh!

vshankar commented Feb 27, 2025

Uh oh!

dparmar18 commented Feb 27, 2025

Uh oh!

dparmar18 commented Feb 28, 2025

Uh oh!

dparmar18 commented Feb 28, 2025

Uh oh!

ffilz commented Mar 5, 2025

Uh oh!

github-actions bot commented May 4, 2025

Uh oh!

dparmar18 commented May 7, 2025

Uh oh!

anoopcs9 left a comment

Choose a reason for hiding this comment

Uh oh!

dparmar18 commented Jun 9, 2025

Uh oh!

vshankar commented Jun 17, 2025

Uh oh!

vshankar commented Jun 18, 2025

Uh oh!

Uh oh!

vshankar commented Jun 18, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

synarete commented Oct 27, 2024 •

edited by rishabh-d-dave

Loading