Ceph » Linux kernel client

how is this going?

It's on the list, I'll get to it sometime after I'm done with #9779.
Is that really something you care about? What's your use case for an image with so many parents?

Hello, I'm bo cai, the bug submitter's colleague, I have been focusing on this problem for two weeks and just reached a conclusion:

In function rbd_add(), there's a recursive-like invoking logic below:
rbd_dev_image_probe()->rbd_dev_probe_parent()->rbd_dev_image_probe()->rbd_dev_probe_parent()->......

When a image has too many parents, this logic will result in stack overflow (however, a process in kernel mode only has a little 8K stack in X64 arch), I changed the kernel stack size from 8K to 64K and recompiled kernel, and then the problem disappeared.

so, shoud I do something in rbd_add()? for example, change the recursion to a loop? add a clone-chain-length limit? or something else? @llya Dryomov

Yeah, the stack overflow part was clear from the beginning. Changing the structure of the code so that it doesn't generate a recursive process is the way to do it, but all of that is tied with the submit/callback code which I'd rather not change just to fix this particular problem - there are other issues with it, so it will be reworked in the future. You can't really fix this in a backportable way, so there is no upside.

We could limit the clone chain length, but then again, what's your use case for an image with >50 parents? It's going to perform pretty horribly...

@llya Dryomov

Could you indicate more specifically about the submit/callback code? Because from my side I can only see that the changes have very limited influence to other codes.

Also I would appreciate it if you could share some details about the related rework plan.

As for this issue, I'm afraid it could be a potential but serious security problem, as someone may use it to take the whole system down. Therefore, IMO it is necessary to limit the clone chain length in the user-mode code (or in kernel?) to serve as a temporary solution, what do you think?

rbd_dev_image_probe() -> rbd_dev_probe_parent() -> rbd_dev_image_probe() is not the problem - at least not when we are talking about a 50-100 images chain. Kernel stack is blown in the reply path, where we end up with the following: rbd_obj_request_complete() -> rbd_img_obj_callback() -> rbd_img_parent_read_callback() -> rbd_obj_request_complete() for no good reason.

The rework (or, rather, part of the rework) would be to switch it from a callback-based scheme to a more state machine like scheme, with no recursion. Callbacks don't buy us anything since there is no module boundary, the code as written is just trying to be way more generic then it needs to be.

I'll post a patch for limiting the chain length.

OK, thanks so much for your answer! Could you please give me the link after you posting the patch? my email is cao.xudong@h3c.com, thank you!

Ilya Dryomov wrote:

[PATCH] rbd: don't leak parent_spec in rbd_dev_probe_parent()
[PATCH] rbd: prevent kernel stack blow up on rbd map

This just puts a cap on the parent chain length, but I think it would be a good idea to keep it around even after the problematic recursion is eliminated.

I see status changed to "Need Review", but where is the code about how to fix this?
could you give us some link?

thanks.

I sent a link to a github commit to Xudong Cao a couple of days ago. And, like all other kernel client patches, it's been posted to the mailing list:

https://www.mail-archive.com/ceph-devel@vger.kernel.org/msg25996.html
https://www.mail-archive.com/ceph-devel@vger.kernel.org/msg25997.html