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

On 27.07.26 7:41 PM, Ilya Dryomov wrote:
> On Mon, Jul 27, 2026 at 5:32 PM Raphael Zimmer
> <[email protected]> wrote:
>>
>> On 27.07.26 12:35 PM, Ilya Dryomov wrote:
>>> On Wed, Jul 1, 2026 at 5:56 PM 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.
> 

That sounds plausible to me.

>>
>>> 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 = lookup_pg_mapping(&osdmap->primary_temp, pgid);
>     if (pg && !WARN_ON_ONCE(ceph_osd_is_down(osdmap,
>                                              pg->primary_temp.osd)))
>             temp->primary = pg->primary_temp.osd;
> 
> What do you think?
> 

raw_to_up_osds() checks for ceph_osd_is_down(osdmap, set->osds[i]) and
therefore removes all invalid osd indices from the up set. The primary
is only ever taken from this set afterwards. So there should be no
chance for an invalid osd index to remain in the up set. The acting set
gets filled in get_temp_osds(), and a check for ceph_osd_is_down(osdmap,
pg->pg_temp.osds[i]) is also already present there. The only unchecked
part is the primary of the acting set taken from primary_temp. So the
potential out-of-bounds read can only happen because of an invalid index
in primary_temp (at least in the trigger path I observed).

Your proposed solution looks good, I will resend a new patch with it.

Best regards,
Raphael