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

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)

> > +		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?

Thanks,
Slava.

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