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.

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

librbd: prevent overflow of discard API result code #18923

Merged
merged 1 commit into from Nov 16, 2017

Conversation

Projects
None yet
3 participants
@dillaman
Copy link
Contributor

commented Nov 14, 2017

Fixes: http://tracker.ceph.com/issues/21966
Signed-off-by: Jason Dillaman dillaman@redhat.com

@vshankar

This comment has been minimized.

Copy link
Contributor

commented Nov 15, 2017

LGTM

@@ -134,7 +136,7 @@ ssize_t ImageRequestWQ<I>::discard(uint64_t off, uint64_t len,
if (r < 0) {
return r;
}
return len;
return std::min<uint64_t>(std::numeric_limits<int>::max(), len);

This comment has been minimized.

Copy link
@trociny

trociny Nov 15, 2017

Contributor

Interesting, I see some inconsistency. In librbd::Image::discard() we return -EINVAL if len is greater than max int, while in rbd_discard() we don't have this check.

Also, don't you think it is better to truncate the return value explicitly in API functions? For me the reason of truncating would be more clear then when reading the code. And it will make adding rbd_discard2 (if we eventually want) simpler. And the return value of io_work_queue->discard may be used as a return value of ssize_t rbd_writesame(), which might give unexpected result.

This comment has been minimized.

Copy link
@dillaman

dillaman Nov 15, 2017

Author Contributor

Good point -- I didn't realize that the C++ API already threw an error.

@trociny

This comment has been minimized.

Copy link
Contributor

commented Nov 15, 2017

And may be it is time to add rbd_discard2 and use it instead of rbd_discard in python binding? This looks like a nicer fix for http://tracker.ceph.com/issues/21966

@dillaman

This comment has been minimized.

Copy link
Contributor Author

commented Nov 15, 2017

@trociny Adding a rbd_discard2(..., uint64_t off, size_t len) seems like it would have the same effect of just checking the incoming length and returning -EINVAL if too large. I wouldn't want to expose an API method that allowed uint64_t len since, for example, a 1TB discard could turn into hundreds of thousands of concurrent IO requests to the OSDs (although we really should have an internal max IO queue depth to prevent such situations).

I did open a ticket against OpenStack for this issue since they should be issuing the discard in reasonable size chunks (or just resize the image down since that's what they are using the discard for anyway).

librbd: prevent overflow of discard API result code
Prevent discard/writesame lengths larger than 2GB.

Fixes: http://tracker.ceph.com/issues/21966
Signed-off-by: Jason Dillaman <dillaman@redhat.com>
@trociny

This comment has been minimized.

Copy link
Contributor

commented Nov 15, 2017

@dillaman I consider this rather a problem of librbd that it can't deal with large truncates nicely. Though I agree that returning -EINVAL if too large is probably good enough until someone improves librbd.

@dillaman dillaman force-pushed the dillaman:wip-21966 branch from f5f592a to 3effd32 Nov 15, 2017

@trociny
Copy link
Contributor

left a comment

LGTM

@trociny trociny merged commit 956f3c8 into ceph:master Nov 16, 2017

5 checks passed

Docs: build check OK - docs built
Details
Signed-off-by all commits in this PR are signed
Details
Unmodified Submodules submodules for project are unmodified
Details
make check make check succeeded
Details
make check (arm64) make check succeeded
Details

@dillaman dillaman deleted the dillaman:wip-21966 branch Nov 16, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.