common/buffer: fix stack corruption in rebuild_aligned_size_and_memory() #42112

CongMinYin · 2021-06-30T09:02:16Z

There is such a bl, which needs to satisfy two conditions:

all ptr's length sum except last ptr is aligned with 4K;
the length of last ptr is 0.

This bl will cause stack corruption when calling
bufferlist::rebuild_aligned_size_and_memory().

Deal with this special scenario in rebuild_aligned_size_and_memory() to
solve the bug. And added a specialtest-case to reproduce this scenario.

Fixes: https://tracker.ceph.com/issues/51419

Signed-off-by: Yin Congmin congmin.yin@intel.com

Checklist

References tracker ticket
Updates documentation if necessary
Includes tests for new functionality or reproducer for bug

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 api
jenkins test docs
jenkins render docs
jenkins test ceph-volume all
jenkins test ceph-volume tox

CongMinYin · 2021-06-30T09:04:50Z

Hi @rzarzynski , in the process of fixing the RBD bug, I found there was a problem with the common buffer. I have written a test case to reproduce this problem. Can you have a look at it.

CongMinYin · 2021-06-30T09:13:30Z

CC @idryomov , new issue about pwl/ssd, this to fix https://tracker.ceph.com/issues/51419.

CongMinYin · 2021-06-30T09:14:26Z

CC @majianpeng

tchaikov · 2021-06-30T09:21:57Z

@CongMinYin could you summarize the change you made in the commit in the title. see https://github.com/ceph/ceph/blob/master/SubmittingPatches.rst#commit-title

"fix SIGABRT" is the effect of this change, but it does not describe the change.

idryomov · 2021-06-30T09:34:36Z

Yes, please. This needs a detailed explanation of what is wrong with the bufferlist code.

Separately, I would appreciate if you describe the RBD bug that led you to discovering this.

src/test/bufferlist.cc

majianpeng · 2021-06-30T09:50:47Z

use our test case w/o this pr;

before splice blbuffer::list(len=20480,
        buffer::ptr(0~124 0x563a79612d00 in raw 0x563a79612d00 len 124 nref 1),
        buffer::ptr(0~3972 0x563a7966c010 in raw 0x563a7966c010 len 8096 nref 1),
        buffer::ptr(0~16384 0x563a79668000 in raw 0x563a79668000 len 16384 nref 2)
)
after splice blbuffer::list(len=12288,
        buffer::ptr(4096~12288 0x563a79669000 in raw 0x563a79668000 len 16384 nref 3),
        buffer::ptr(3972~0 0x563a7966cf94 in raw 0x563a7966c010 len 8096 nref 2)

in func rebuild_aligned_size_and_memory, it will corrupt stack. This because
a)bl has a ptr which length is zero.
b)the first ptr's size and memroy-address aligned w/ 4K.
So https://github.com/ceph/ceph/blob/master/src/common/buffer.cc#L1261 will corrupt stack and cause SIGABRT.

rzarzynski · 2021-06-30T09:52:16Z

src/common/buffer.cc

@@ -1645,7 +1645,6 @@ static ceph::spinlock debug_lock;
        curbuf = _buffers.erase_after(curbuf_prev);
 	_carriage->set_offset(_carriage->offset() + _carriage->length());
 	_carriage->set_length(0);
-	_buffers.push_back(*_carriage);


Well, if we really need to drop this line, the comment above should be reworked as well. Also, I'm worried the patch introduces a memory leak as erase_after() doesn't dispose the node.

In the meantime, I'm joining the folks asking for more descriptive commit message.

Well, if we really need to drop this line, the comment above should be reworked as well. Also, I'm worried the patch introduces a memory leak as erase_after() doesn't dispose the node.

In the meantime, I joining the folks asking for more descriptive commit message.

I'm not sure about the solution, but the problem does exist. see the issue describe by Jianpeng

@CongMinYin, @majianpeng: I'm not questioning that at all. :-) Just arguing about the way how we fix it.

a)bl has a ptr which length is zero.

This is still valid and can (rarely) happen because of how _carriage and the non-pointer append() variants of contiguous_appender interact:

class contiguous_appender { // ... void append(const bufferlist& l) { if (deep) { // ... } else { flush_and_continue(); bl.append(l); space = bl.obtain_contiguous_space(0); out_of_band_offset += l.length(); } }

My impression is we need to take a look on rebuild_aligned_size_and_memory(). I can help with that.

Also, take a look on the BufferListIterator,iterate_with_empties testcase.

We also look forward to discussing with you and seeking the most appropriate solution.

OK, I think the fix could look like this one:

diff --git a/src/common/buffer.cc b/src/common/buffer.cc index 548abc458fd..9c76cde6eed 100644 --- a/src/common/buffer.cc +++ b/src/common/buffer.cc @@ -1258,8 +1258,12 @@ static ceph::spinlock debug_lock; buffer::create_aligned(unaligned._len, align_memory))); had_to_rebuild = true; } - _buffers.insert_after(p_prev, *ptr_node::create(unaligned._buffers.front()).release()); - _num += 1; + if (unaligned.get_num_buffers()) { + _buffers.insert_after(p_prev, *ptr_node::create(unaligned._buffers.front()).release()); + _num += 1; + } else { + // a bufferlist containing only 0-length bptrs is rebuild as empty + } ++p_prev; } return had_to_rebuild;

@rzarzynski Thank you for your solution. I have updated the code and commit information of PR. Please check.

src/test/bufferlist.cc

MahatiC · 2021-07-01T03:12:09Z

Separately, I would appreciate if you describe the RBD bug that led you to discovering this.

@idryomov an example where this can be reproduced is when the below fio config is run

[global]

ioengine=rbd
clientname=admin
rw=randwrite
#bs=1m
bs=16k
time_based=1
runtime=20s
iodepth=16
pool=rbd
group_reporting

[volumes]
rbdname=fio-test

src/test/bufferlist.cc

idryomov · 2021-07-02T13:04:25Z

jenkins test make check

tchaikov · 2021-07-02T15:11:28Z

failures are not related to this changeset.

tchaikov · 2021-07-02T15:11:44Z

@rzarzynski could you take another look?

src/test/bufferlist.cc

CongMinYin · 2021-07-06T02:56:41Z

@idryomov made changes as you suggested, please check.

idryomov

Looks good to me, just a couple of really minor nits. I assume you have verified that the new test case still leads to a crash if the patch isn't applied?

src/common/buffer.cc

src/test/bufferlist.cc

There is such a bl, which needs to satisfy two conditions: 1)all ptrs' length sum except last ptr is aligned with 4K; 2)the length of last ptr is 0. This bl will cause stack corruption when calling bufferlist::rebuild_aligned_size_and_memory(). Deal with this special scenario in rebuild_aligned_size_and_memory() to solve the bug. And added a specialtest-case to reproduce this scenario. Fixes: https://tracker.ceph.com/issues/51419 Signed-off-by: Yin Congmin <congmin.yin@intel.com>

CongMinYin · 2021-07-09T10:04:39Z

Looks good to me, just a couple of really minor nits. I assume you have verified that the new test case still leads to a crash if the patch isn't applied?

Yes, I test the new test case. Without this change in rebuild_aligned_size_and_memory, new unit_test failed., after appling this change, new uint_test passed.

had modified comments, please check.

idryomov · 2021-07-09T11:31:24Z

Merging based on Kefu's earlier runs since the fix itself hasn't changed.

github-actions bot added common tests labels Jun 30, 2021

tchaikov requested a review from rzarzynski June 30, 2021 09:22

tchaikov added the bug-fix label Jun 30, 2021

idryomov reviewed Jun 30, 2021

View reviewed changes

src/test/bufferlist.cc Outdated Show resolved Hide resolved

rzarzynski requested changes Jun 30, 2021

View reviewed changes

CongMinYin force-pushed the fix-common-buffer-SIGABRT branch 2 times, most recently from 002bf0c to 203143f Compare July 1, 2021 03:46

tchaikov added the wip-kefu-testing label Jul 1, 2021

rzarzynski reviewed Jul 1, 2021

View reviewed changes

src/test/bufferlist.cc Outdated Show resolved Hide resolved

CongMinYin force-pushed the fix-common-buffer-SIGABRT branch from 203143f to bc15029 Compare July 2, 2021 03:31

tchaikov added needs-review and removed wip-kefu-testing labels Jul 2, 2021

rzarzynski reviewed Jul 5, 2021

View reviewed changes

src/test/bufferlist.cc Outdated Show resolved Hide resolved

CongMinYin force-pushed the fix-common-buffer-SIGABRT branch from bc15029 to e043b5e Compare July 5, 2021 08:35

rzarzynski approved these changes Jul 5, 2021

View reviewed changes

idryomov reviewed Jul 5, 2021

View reviewed changes

src/test/bufferlist.cc Outdated Show resolved Hide resolved

CongMinYin force-pushed the fix-common-buffer-SIGABRT branch 2 times, most recently from 238b825 to 48c3bd2 Compare July 6, 2021 02:55

idryomov reviewed Jul 8, 2021

View reviewed changes

src/common/buffer.cc Outdated Show resolved Hide resolved

src/test/bufferlist.cc Outdated Show resolved Hide resolved

src/test/bufferlist.cc Outdated Show resolved Hide resolved

CongMinYin force-pushed the fix-common-buffer-SIGABRT branch from 48c3bd2 to 05a49ae Compare July 9, 2021 10:01

idryomov changed the title ~~common/buffer: fix SIGABRT in rebuild_aligned_size_and_memory.~~ common/buffer: fix stack corruption in rebuild_aligned_size_and_memory() Jul 9, 2021

idryomov approved these changes Jul 9, 2021

View reviewed changes

idryomov merged commit 174b611 into ceph:master Jul 9, 2021

This was referenced Aug 30, 2021

octopus: common/buffer: fix SIGABRT in rebuild_aligned_size_and_memory #42975

Merged

pacific: common/buffer: fix SIGABRT in rebuild_aligned_size_and_memory #42976

Merged

ideepika mentioned this pull request Nov 5, 2021

pacific: librbd/cache/pwl: persistant cache backports #43772

Merged

ronen-fr mentioned this pull request Jan 19, 2022

osd/scrub: extracting scrub "backend" functionality #42993

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

common/buffer: fix stack corruption in rebuild_aligned_size_and_memory() #42112

common/buffer: fix stack corruption in rebuild_aligned_size_and_memory() #42112

CongMinYin commented Jun 30, 2021 •

edited

CongMinYin commented Jun 30, 2021 •

edited

CongMinYin commented Jun 30, 2021

CongMinYin commented Jun 30, 2021

tchaikov commented Jun 30, 2021 •

edited

idryomov commented Jun 30, 2021

majianpeng commented Jun 30, 2021

rzarzynski Jun 30, 2021 •

edited

CongMinYin Jun 30, 2021

rzarzynski Jun 30, 2021

rzarzynski Jun 30, 2021

CongMinYin Jun 30, 2021

rzarzynski Jun 30, 2021

CongMinYin Jul 1, 2021 •

edited

MahatiC commented Jul 1, 2021

idryomov commented Jul 2, 2021

tchaikov commented Jul 2, 2021

tchaikov commented Jul 2, 2021

CongMinYin commented Jul 6, 2021

idryomov left a comment

CongMinYin commented Jul 9, 2021

idryomov commented Jul 9, 2021

common/buffer: fix stack corruption in rebuild_aligned_size_and_memory() #42112

common/buffer: fix stack corruption in rebuild_aligned_size_and_memory() #42112

Conversation

CongMinYin commented Jun 30, 2021 • edited

Checklist

CongMinYin commented Jun 30, 2021 • edited

CongMinYin commented Jun 30, 2021

CongMinYin commented Jun 30, 2021

tchaikov commented Jun 30, 2021 • edited

idryomov commented Jun 30, 2021

majianpeng commented Jun 30, 2021

rzarzynski Jun 30, 2021 • edited

Choose a reason for hiding this comment

CongMinYin Jun 30, 2021

Choose a reason for hiding this comment

rzarzynski Jun 30, 2021

Choose a reason for hiding this comment

rzarzynski Jun 30, 2021

Choose a reason for hiding this comment

CongMinYin Jun 30, 2021

Choose a reason for hiding this comment

rzarzynski Jun 30, 2021

Choose a reason for hiding this comment

CongMinYin Jul 1, 2021 • edited

Choose a reason for hiding this comment

MahatiC commented Jul 1, 2021

idryomov commented Jul 2, 2021

tchaikov commented Jul 2, 2021

tchaikov commented Jul 2, 2021

CongMinYin commented Jul 6, 2021

idryomov left a comment

Choose a reason for hiding this comment

CongMinYin commented Jul 9, 2021

idryomov commented Jul 9, 2021

CongMinYin commented Jun 30, 2021 •

edited

CongMinYin commented Jun 30, 2021 •

edited

tchaikov commented Jun 30, 2021 •

edited

rzarzynski Jun 30, 2021 •

edited

CongMinYin Jul 1, 2021 •

edited