Re: [PATCH] libceph: Always remove requests from ceph_osdc_handle_map() structures

On 30.06.26 6:06 PM, Raphael Zimmer wrote:
> On 29.06.26 6:43 PM, Viacheslav Dubeyko wrote:
>> On Mon, 2026-06-29 at 09:12 -0700, Viacheslav Dubeyko wrote:
>>> CC: [email protected]
>>>
>>> On Wed, 2026-06-17 at 15:13 +0200, Raphael Zimmer wrote:
>>>> ceph_osdc_handle_map() creates two local stack variables that track
>>>> (linger) requests that need to be resent. For requests, this is an
>>>> rb_root, and for linger requests, a list_head. Subsequently,
>>>> handle_one_map() is called, which calls scan_requests() for each
>>>> osd.
>>>> In scan_requests, the new target is calculated for each (linger)
>>>> request, and, in case of CALC_TARGET_NEED_RESEND, they are inserted
>>>> into
>>>> the previously created list/rbtree. All requests in these
>>>> structures
>>>> are
>>>> kicked off to their new target osd after processing all osdmaps. If
>>>> an
>>>> error occurs during processing, e.g., because an osdmap is
>>>> corrupted,
>>>> ceph_osdc_handle_map() jumps to the bad label and doesn't call
>>>> kick_requests(). Therefore, the (linger) requests are still
>>>> inserted
>>>> in
>>>> the list/rbtree, which now doesn't have a head/root anymore. This
>>>> results in requests containing invalid pointers to stack memory.
>>>> Subsequently, this can have multiple effects, ranging from not
>>>> being
>>>> able to insert linger requests again because they still have prev
>>>> and
>>>> next pointers or triggering a BUG_ON() assertion when trying to
>>>> insert a
>>>> non-empty rb_node again, to overwriting stack memory.
>>>>
>>>> This patch fixes the issue by making sure all (linger) requests are
>>>> removed from the list/rbtree in case the osdmap processing returns
>>>> an
>>>> error. This is accomplished by adding logic that is executed after
>>>> a
>>>> jump to the bad label performing the removal similar to
>>>> kick_requests.
>>>>
>>>> Fixes: 5aea3dcd5021 ("libceph: a major OSD client update")
>>>> Fixes: 922dab613417 ("libceph, rbd: ceph_osd_linger_request,
>>>> watch/notify v2")
>>>> Signed-off-by: Raphael Zimmer <[email protected]>
>>>> ---
>>>>  net/ceph/osd_client.c | 21 +++++++++++++++++++++
>>>>  1 file changed, 21 insertions(+)
>>>>
>>>> diff --git a/net/ceph/osd_client.c b/net/ceph/osd_client.c
>>>> index 5fc79c29aab0..a4d3cbfd27a3 100644
>>>> --- a/net/ceph/osd_client.c
>>>> +++ b/net/ceph/osd_client.c
>>>> @@ -4129,6 +4129,26 @@ static void kick_requests(struct
>>>> ceph_osd_client *osdc,
>>>>  	}
>>>>  }
>>>>  
>>>> +static void clear_resend_requests(struct ceph_osd_client *osdc,
>>
>> This function is small enough. So, we can declare the function as
>> static inline.
>>
>> As far as I can see, osdc parameter hasn't been used in function at
>> all. I assume that it is not necessary.
>>
> 
> Yes, you’re right, I will fix this.
> 
>>>> +				  struct rb_root *need_resend,
>>>> +				  struct list_head
>>>> *need_resend_linger)
>>>> +{
>>>> +	struct ceph_osd_linger_request *lreq, *nlreq;
>>>> +	struct rb_node *n;
>>>> +
>>>> +	for (n = rb_first(need_resend); n; ) {
>>
>> This for() declaration looks really weird. I think you need to
>> implement something like this:
>>
>> n = rb_first(need_resend);
>>
>> while (n) {
>>     <execute logic>
>> };
>>
> 
> I have to admit that I took this from kick_requests(). For me, the
> question is rather whether we should remove this duplicate code.
> However, I didn’t want to modify kick_requests(), and it does much more
> than just this removal in an intertwined way. If we stay with this
> solution, I wouldn’t really want to introduce yet another kind of loop.
> 
>>>> +		struct ceph_osd_request *req =
>>>> +		    rb_entry(n, struct ceph_osd_request, r_node);
>>
>> I think you need to move the struct ceph_osd_request *req into
>> declaration section, then you will be able to have assignment on one
>> line:
>>
>> req = rb_entry(n, struct ceph_osd_request, r_node);
>>
> 
> This would be possible. Again, kick_requests() also uses this pattern.
> Moreover, it seems to be consistently used in the surrounding functions,
> e.g. scan_requests() or for a ceph_pg_pool_info in set_pool_was_full().
> To preserve consistency, I would prefer to either modify it everywhere
> or not at all.
> 
>>>> +
>>>> +		n = rb_next(n);
>>>> +		erase_request(need_resend, req);
>>
>> The erase_request() has BUG_ON() for empty node:
>>
>> BUG_ON(RB_EMPTY_NODE(&t->r_node));
>>
>> Are we sure that we will have not empty node always?
>>
> 
> We got this node from rb_first()/rb_entry() directly beforehand.
> Therefore, it should never be empty.
> 
> 
>> I think that we need to call the erase_request() at first and, then, to
>> call the rb_next(). It sounds more reasonable for my taste.
>>
> 
> If we call erase_request() first, the request will be erased from the
> rbtree, and we can’t call rb_next() on it anymore.
> 
>> I am guessing... Should we re-link these requests to osdc->homeless_osd
>> so they can be retried? These requests are orphaned and it will be
>> never resent. It sounds like any waiters will hang until timeout.
>>
> 
> I’m not sure. The problem occurs if the osdmap is corrupted in a way
> that a processing error occurs after at least once successfully calling
> scan_requests() from handle_one_map(). That means we will have an
> updated osdmap afterwards, but the requests will still be linked to the
> osds they were previously linked to. If the processing fails because the
> osdmap was corrupted, the other parts of it may also have been
> corrupted, and the old mapping would still be correct for some or all
> requests. Therefore, it could be a reasonable choice to leave the
> mapping as it is, until another osdmap arrives that is valid. However,
> if it was not completely corrupted, remapping to the homeless_osd could
> also be the better choice. If the changed mapping has not been corrupted
> at all, it could even be best to simply call kick_requests() and resend
> these requests to their new osds. In this case, we wouldn't need to
> introduce this new function here at all.
> 
> What do you think about it? Would you still argue for linking it to the
> homeless_osd or prefer one of the other solutions?
> 

Hi,
do you have any opinion on the open points by now so that I can create a
new version of this patch incorporating them?

Best regards,
Raphael