Re: [PATCH] libceph: Assign requests to homeless osd if calculated osd exceeds max_osd

On 29.06.26 7:30 PM, Viacheslav Dubeyko wrote:
> On Mon, 2026-06-29 at 09:46 -0700, Viacheslav Dubeyko wrote:
>> CC: [email protected]
>>
>> On Mon, 2026-06-29 at 10:42 +0200, Raphael Zimmer wrote:
>>> A corrupted osdmap received from a Ceph monitor or OSD may contain
>>> placement groups with osd indices greater than max_osd.
>>> Subsequently,
>>> this may lead to calc_target() returning such an index as target
>>> osd
>>> for
>>> a (linger) request. Because the osd_state, osd_weight, and osd_addr
>>> arrays only contain max_osd entries, this leads to out-of-bounds
>>> accesses when trying to read values from these arrays.
>>>
>>> This patch fixes the issue by adding a check to calc_target()
>>> assigning
>>> the request to the homeless osd if the target osd index read from
>>> the
>>> osdmap exceeds the maximum index of an osd.
>>>
>>> Fixes: 63244fa123a7 ("libceph: introduce ceph_osd_request_target,
>>> calc_target()")
>>> Signed-off-by: Raphael Zimmer <[email protected]>
>>> ---
>>>  net/ceph/osd_client.c | 6 ++++++
>>>  1 file changed, 6 insertions(+)
>>>
>>> diff --git a/net/ceph/osd_client.c b/net/ceph/osd_client.c
>>> index a4d3cbfd27a3..3ae7c51e2edc 100644
>>> --- a/net/ceph/osd_client.c
>>> +++ b/net/ceph/osd_client.c
>>> @@ -1707,6 +1707,12 @@ static enum calc_target_result
>>> calc_target(struct ceph_osd_client *osdc,
>>>  		}
>>>  	}
>>>  
>>> +	if (t->osd >= osdc->osdmap->max_osd) {
> 
> The t->osd is int, osdc->osdmap->max_osd is u32. In C, the signed
> operand is promoted to unsigned in a mixed comparison. If t->osd ==
> CEPH_HOMELESS_OSD == -1, the comparison becomes (u32)-1 >= max_osd. The
> check fires on every homeless request. I think we need to be more
> careful in this comparison:
> 
> if (t->osd >= 0 && (u32)t->osd >= osdc->osdmap->max_osd)
> 

You're right, I missed this point. I will fix it.

>>> +		t->osd = CEPH_HOMELESS_OSD;
>>> +		ct_res = CALC_TARGET_NO_ACTION;
> 
> I am not completely sure that CALC_TARGET_NO_ACTION is correct setting.
> We set req->r_t.osd == CEPH_HOMELESS_OSD while req->r_osd still points
> to the old OSD. I assume that request will be sent to the wrong OSD and
> it will never retried via the homeless path. The calc_target() has
> multiple callers. Should we set ct_res to CALC_TARGET_NEED_RESEND?

I also wasn't sure which option would be the best for ct_res in this
specific case of a corrupted osdmap. It sounds reasonable to me to
switch to CALC_TARGET_NEED_RESEND.

I will send a new version of the patch with these modifications.

Best regards,
Raphael

> 
> Thanks,
> Slava.
> 
>>> +		goto out;
>>> +	}
>>> +
>>>  	if (unpaused || legacy_change || force_resend || split)
>>>  		ct_res = CALC_TARGET_NEED_RESEND;
>>>  	else