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

rbd-nbd: check /sys/block/nbdX/size to ensure kernel mapped correctly #13229

Merged
merged 1 commit into from Feb 11, 2017

Conversation

Projects
None yet
4 participants
@trociny
Copy link
Contributor

trociny commented Feb 2, 2017

Fixes: http://tracker.ceph.com/issues/18335
Signed-off-by: Mykola Golub mgolub@mirantis.com

r = -EFBIG;
cerr << "rbd-nbd: image is too large (" << prettybyte_t(size) << ", max is "
<< prettybyte_t((1UL << 32) * 512) << ")" << std::endl;
r = ioctl(nbd, NBD_SET_SIZE, size);

This comment has been minimized.

Copy link
@liupan1111

liupan1111 Feb 2, 2017

Contributor

Could you explain why you remove size constraint check? what happens if run in 32 bit system?

This comment has been minimized.

Copy link
@trociny

trociny Feb 2, 2017

Author Contributor

I expect check_device_size to fail in this case.

@@ -468,6 +469,28 @@ static int open_device(const char* path, bool try_load_moudle = false)
return nbd;
}

static int check_device_size(int nbd_index, unsigned long expected_size)
{
unsigned long size = 0;

This comment has been minimized.

Copy link
@liupan1111

liupan1111 Feb 2, 2017

Contributor

I suggest use uint64_t instead of unsigned long here. size *= 512 may overflow...

@trociny trociny force-pushed the trociny:wip-18335 branch from 769629a to 327413d Feb 2, 2017

@trociny

This comment has been minimized.

Copy link
Contributor Author

trociny commented Feb 2, 2017

updated: check for info.size <= ULONG_MAX is added.

@liupan1111

This comment has been minimized.

Copy link
Contributor

liupan1111 commented Feb 3, 2017

updated: check for info.size <= ULONG_MAX is added.

lgtm

@liupan1111

This comment has been minimized.

Copy link
Contributor

liupan1111 commented Feb 3, 2017

I expect check_device_size to fail in this case.

I don't like this very much... depends on a check based on a overflowed value is not very suitable.

@trociny

This comment has been minimized.

Copy link
Contributor Author

trociny commented Feb 3, 2017

@liupan1111 After the chek for info.size <= ULONG_MAX is added, overflow in check_device_size is possible only if the kernel returned really strange result.

@liupan1111

This comment has been minimized.

Copy link
Contributor

liupan1111 commented Feb 3, 2017

oh... I see. Agree with it!

return -EINVAL;
}
ifs >> size;
size *= 512;

This comment has been minimized.

Copy link
@dillaman

dillaman Feb 3, 2017

Contributor

Nit: use a constant for block size here and below

Mykola Golub

@trociny trociny force-pushed the trociny:wip-18335 branch from 327413d to 596e5ea Feb 4, 2017

@trociny

This comment has been minimized.

Copy link
Contributor Author

trociny commented Feb 4, 2017

@dillaman updated:

  • a constant for block size
  • qa test (forgot to update it in the previous version)
@dillaman
Copy link
Contributor

dillaman left a comment

lgtm

@dillaman dillaman merged commit e95fe0d into ceph:master Feb 11, 2017

3 checks passed

Signed-off-by all commits in this PR are signed
Details
Unmodifed Submodules submodules for project are unmodified
Details
default Build finished.
Details

@trociny trociny deleted the trociny:wip-18335 branch Feb 11, 2017

@zhsj

This comment has been minimized.

Copy link
Contributor

zhsj commented May 5, 2017

Hi, could you please take look at http://tracker.ceph.com/issues/19871 which seems caused by this commit.

@trociny

This comment has been minimized.

Copy link
Contributor Author

trociny commented May 5, 2017

@zhsj Thank you for the report. I will take care of it.

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.