Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

hammer: librados: bad flags can crash the osd #11936

Merged
merged 3 commits into from Jan 5, 2017
Merged

hammer: librados: bad flags can crash the osd #11936

merged 3 commits into from Jan 5, 2017

Conversation

smithfarm
Copy link
Contributor

@smithfarm smithfarm self-assigned this Nov 12, 2016
@smithfarm smithfarm added this to the hammer milestone Nov 12, 2016
@smithfarm smithfarm changed the title hammer: librados,osd: bad flags can crash the osd hammer: librados: bad flags can crash the osd Nov 16, 2016
smithfarm added a commit that referenced this pull request Nov 18, 2016
@smithfarm
Merge pull request #11936: hammer: librados: bad flags can crash the osd
ff81206 
Reviewed-by: Nathan Cutler <ncutler@suse.com>
smithfarm added a commit that referenced this pull request Nov 20, 2016
@smithfarm
Merge pull request #11936: hammer: librados: bad flags can crash the osd
6609bbb 
Reviewed-by: Nathan Cutler <ncutler@suse.com>
@smithfarm
Copy link
Contributor Author

@liewegas This backport passed a rados suite at http://tracker.ceph.com/issues/17151#note-7 with failures that I believe have been addressed (except for http://tracker.ceph.com/issues/15139 which is caused by the build system no longer providing dumpling-era packages).

I have rebuilt the integration branch to include the two fixes and scheduled a new run at http://tracker.ceph.com/issues/17151#note-14

Do you think it's OK to merge provided the second run succeeds?

smithfarm added a commit that referenced this pull request Nov 21, 2016
@smithfarm
Merge pull request #11936: hammer: librados: bad flags can crash the osd
fa0b4d4 
Reviewed-by: Nathan Cutler <ncutler@suse.com>
smithfarm added a commit that referenced this pull request Nov 21, 2016
@smithfarm
Merge pull request #11936: hammer: librados: bad flags can crash the osd
fafcf78 
Reviewed-by: Nathan Cutler <ncutler@suse.com>
@smithfarm
Copy link
Contributor Author

smithfarm commented Nov 21, 2016 •
edited

Second run succeeded; third run here: http://pulpito.front.sepia.ceph.com/smithfarm-2016-11-21_11:52:41-rados-hammer-backports---basic-smithi/ (also succeeded)

smithfarm added a commit that referenced this pull request Nov 23, 2016
@smithfarm
Merge pull request #11936: hammer: librados: bad flags can crash the osd
22b31b9 
Reviewed-by: Nathan Cutler <ncutler@suse.com>
smithfarm added a commit that referenced this pull request Nov 24, 2016
@smithfarm
Merge pull request #11936: hammer: librados: bad flags can crash the osd
7fc9555 
Reviewed-by: Nathan Cutler <ncutler@suse.com>
@smithfarm
Copy link
Contributor Author

@liewegas @athanatos This passed another rados run, see http://tracker.ceph.com/issues/17151#note-23

Do you think it can be merged?

@athanatos
Copy link
Contributor

@jdurgin @liewegas Wait a minute. This changes the librados header. That seems bad.

I think the only part we want to backport is the ReplicatedPG bit that avoid crashing the osd.

@liewegas
Copy link
Member

Yeah I think i brought this up on the jewel backprot, but we can't backport the remove() fucntion addition because it goes from not overloaded to overloaded, which breaks the ABI.

@athanatos athanatos changed the title hammer: librados: bad flags can crash the osd DNM: hammer: librados: bad flags can crash the osd Nov 28, 2016
@athanatos
Copy link
Contributor

Changed to DNM

smithfarm
smithfarm commented Nov 29, 2016
View reviewed changes
src/test/librados/misc.cc
}
{
ASSERT_EQ(-EINVAL, ioctx.remove("badfoo", badflags));
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@liewegas This is the reason I backported remove. Can this test be dropped in hammer?

@smithfarm
Copy link
Contributor Author

I think i brought this up on the jewel backprot, but we can't backport the remove() fucntion addition because it goes from not overloaded to overloaded, which breaks the ABI.

The jewel backport of 1aa807f uses remove() without needing any additional commits, so apparently librados supports remove() in jewel.

@liewegas
Copy link
Member

liewegas commented Nov 29, 2016 via email

@liewegas @smithfarm
ceph_test_rados_api_misc: test rados op with bad flas
8d35235 
Pass the bad PARALLELEXEC flag to remove(), which takes a flags arg.

Signed-off-by: Sage Weil <sage@redhat.com>
(cherry picked from commit 1aa807f)

Conflicts:
    src/test/librados/misc.cc (dropped ioctx.remove() test because there is no
                              remove() in hammer librados)
@liewegas @smithfarm
osd: reject PARALLELEXEC ops with EINVAL
be71845 
Errors are better than crashing.

Signed-off-by: Sage Weil <sage@redhat.com>
(cherry picked from commit 4074951)

Conflicts:
	src/osd/ReplicatedPG.cc (trivial resolution)
@smithfarm smithfarm changed the title DNM: hammer: librados: bad flags can crash the osd hammer: librados: bad flags can crash the osd Nov 29, 2016
@majianpeng @smithfarm
osd: Add func has_flag in MOSDOp.
5f00e1d 
Signed-off-by: Jianpeng Ma <jianpeng.ma@intel.com>
(manually cherry picked just one line from d9a2ca5)
@smithfarm
Copy link
Contributor Author

@liewegas So, in the first commit, I dropped just the test that uses remove() - is that OK or did you mean to drop the entire commit?

smithfarm added a commit to SUSE/ceph that referenced this pull request Dec 13, 2016
@smithfarm
Merge pull request ceph#11936: hammer: librados: bad flags can crash …
1baa6d2 
…the osd

Reviewed-by: Nathan Cutler <ncutler@suse.com>
smithfarm added a commit that referenced this pull request Dec 14, 2016
@smithfarm
Merge pull request #11936: hammer: librados: bad flags can crash the osd
67e5096 
Reviewed-by: Nathan Cutler <ncutler@suse.com>
smithfarm added a commit that referenced this pull request Dec 14, 2016
@smithfarm
Merge pull request #11936: hammer: librados: bad flags can crash the osd
b359c88 
Reviewed-by: Nathan Cutler <ncutler@suse.com>
smithfarm added a commit that referenced this pull request Jan 2, 2017
@smithfarm
Merge pull request #11936: hammer: librados: bad flags can crash the osd
25b1f2a 
Reviewed-by: Nathan Cutler <ncutler@suse.com>
@smithfarm
Copy link
Contributor Author

smithfarm commented Jan 5, 2017 •
edited

@athanatos This just passed another rados run at http://tracker.ceph.com/issues/17151#note-38 and I dropped the test that was using remove().

Do you think it's OK to merge now?

@athanatos athanatos merged commit 7b1f8fe into ceph:hammer Jan 5, 2017
@smithfarm smithfarm deleted the wip-16432-hammer branch January 5, 2017 18:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@smithfarm smithfarm

Labels
bug-fix core
Projects
None yet
Milestone
hammer
4 participants
@smithfarm @athanatos @liewegas @majianpeng