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

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.

> > +				  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>
};

> > +		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);

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

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.

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.

Thanks,
Slava.

> > +	}
> > +
> > +	list_for_each_entry_safe(lreq, nlreq, need_resend_linger,
> > scan_item) {
> > +		list_del_init(&lreq->scan_item);
> > +	}
> > +}
> > +
> >  /*
> >   * Process updated osd map.
> >   *
> > @@ -4243,6 +4263,7 @@ void ceph_osdc_handle_map(struct
> > ceph_osd_client *osdc, struct ceph_msg *msg)
> >  bad:
> >  	pr_err("osdc handle_map corrupt msg\n");
> >  	ceph_msg_dump(msg);
> > +	clear_resend_requests(osdc, &need_resend,
> > &need_resend_linger);
> >  	up_write(&osdc->lock);
> >  }
> >