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

On Mon, Jul 27, 2026 at 5:32=E2=80=AFPM Raphael Zimmer
<[email protected]> wrote:
>
> On 27.07.26 12:35 PM, Ilya Dryomov wrote:
> > On Wed, Jul 1, 2026 at 5:56=E2=80=AFPM Raphael Zimmer
> > <[email protected]> wrote:
> >>
> >> A corrupted osdmap received from a Ceph monitor or OSD may contain
> >> placement groups with osd indices that don't exist, i.e., that are
> >> greater than max_osd or smaller than CEPH_HOMELESS_OSD (-1).
> >
> > Hi Raphael,
> >
> > The osdmap doesn't contain all placement group mappings -- they are
> > produced on demand by CRUSH based off of the crushmap that is embedded
> > in the osdmap but doesn't contain any OSD indices.  There are a few
> > (usually small or empty) exception tables in the form of pg_temp,
> > primary_temp, pg_upmap and pg_upmap_items where the "override" OSD
> > indices are stored.  Are those what you have in mind wrt. osdmap
> > corruption?
> >
>
> Hi Ilya,
>
> Yes, it's about these. I looked into it again after submitting this
> patch and concluded that the problematic part is specifically
> primary_temp. In the functions called from ceph_pg_to_up_acting_osds(),
> non-existent osds are removed from the osd sets. Therefore, the up set
> will not contain any invalid osds afterwards and will also have a valid
> primary, as it is always one of the osds in the set. Similarly,
> non-existent osds are removed from the acting set. However, the primary
> osd is directly assigned from pg->primary_temp.osd in get_temp_osds(),
> without checking for its existence. This way, an invalid osd index can
> be returned as target osd from calc_target().
>
> Therefore, it would suffice to add the check at this position (in
> get_temp_osds()) and set the primary osd to the homeless osd in this
> case (or perhaps to another osd from the acting set, but this doesn't
> seem like a good solution to me, as the osdmap may be corrupted even
> more and other osd mappings are also incorrect).

Setting the primary to the homeless OSD in get_temp_osds() would make
the OSD set invalid as far as osds_valid() is concerned.  I don't see
a problem with "another OSD" solution because corruptions of this kind
shouldn't ever happen in real deployments.

>
> > If so, I'd suggest adding map->max_osd checks to the corresponding
> > decode routines.  Munging ct_res to CALC_TARGET_NEED_RESEND as done in
> > this patch doesn't make much sense -- you are asking the OSD client to
> > arrange for the OSD request to be resent when there is no valid OSD to
> > send it to (and no expectation whatsoever that the resend's invocation
> > of calc_target() would produce a different result).
> >
>
> I don't completely agree on this matter. I specifically reassign the
> request to the homeless osd here and don't ask for resending it to an
> invalid one. This will result in not sending it out. __submit_request()
> only sends it if !osd_homeless(osd). The same holds true for
> kick_requests(). For linger requests, send_linger() also ultimately
> calls __submit_request(), leading to the same behavior. Finally,
> recalc_linger_target() only re-links the request to the homeless osd if
> calc_target() returns CALC_TARGET_NEED_RESEND.
> If applying the solution described above (checking in get_temp_osds()),
> this would furthermore only happen once.
>
> Adding map->max_osd checks to the corresponding decode routines would
> also be possible. I initially decided not to do this because the
> proposed solution needs fewer code changes and still resolves the issue
> of a potential out-of-bounds access. If we wanted to add checks to the
> decode functions, this should be done in all of them for consistency.
> That would mean we need to change multiple functions, including their
> signatures, to pass the map->max_osd into them.
>
> Do you still think that checks should be added to the decode functions,
> or would a check for either ceph_osd_is_down(pg->primary_temp.osd) or
> !ceph_osd_exists(pg->primary_temp.osd) in get_temp_osds() be an
> acceptable option too?

If there is confidence that the potential out-of-bounds read issue is
limited to primary_temp, I'd go with ceph_osd_is_down() check in
get_temp_osds() and ignore the primary_temp entry if the check fires on
the OSD in question -- the primary derived from pg_temp or the up set
primary (if no pg_temp) might very well be actionable in case of legit
mishap on the monitors.  I'd also warn though since this isn't supposed
to happen normally.  Something along the lines of

    /* primary_temp? (shouldn't ever be a nonexistent or down OSD) */
    pg =3D lookup_pg_mapping(&osdmap->primary_temp, pgid);
    if (pg && !WARN_ON_ONCE(ceph_osd_is_down(osdmap,
                                             pg->primary_temp.osd)))
            temp->primary =3D pg->primary_temp.osd;

What do you think?

Thanks,

                Ilya