Re: [PATCH 2/2] ceph: fix UAF in check_new_map() on session freed during unlock

[Viacheslav Dubeyko]

On Tue, 2026-07-14 at 16:13 +0800, Xiubo Li via B4 Relay wrote:
> From: Xiubo Li <xiubo.li@xxxxxxxxx>
> 
> check_new_map() iterates mdsc->sessions[] and for each active session
> drops mdsc->mutex to perform per-session operations.  The forced-
> close
> path (rank removed from map) correctly takes a reference on s via
> ceph_get_mds_session() before releasing mdsc->mutex, but three other
> paths do not:
> 
>   Path A (address changed):  mutex_unlock → mutex_lock(&s->s_mutex)
>   Path B (reconnect):        mutex_unlock → send_mds_reconnect(mdsc,
> s)
>   Path C (active transition): mutex_unlock → mutex_lock(&s->s_mutex)
> 
> Without the extra reference, another thread can acquire mdsc->mutex
> during the unlock window, call __unregister_session() which drops the
> last reference on s, and free it.  The original thread then accesses
> freed memory via s->s_mutex.
> 
> Fix by adding ceph_get_mds_session(s) before each mutex_unlock and
> ceph_put_mds_session(s) after the corresponding mutex_lock, matching
> the pattern already used in the forced-close path.
> 
> Race timeline (Path A):
> 
>   Thread A (check_new_map)             Thread B (another map update
>     holds mdsc->mutex                      or session teardown)
>   --------------------------           --------------------------
>   s = mdsc->sessions[i]
>   (refcount == 1, held only by
>    sessions[] array)
> 
>   mutex_unlock(&mdsc->mutex)
>                                --->    acquires mdsc->mutex
>                                        __unregister_session(mdsc, s)
>                                          sessions[i] = NULL
>                                          ceph_put_mds_session(s)
>                                            refcount: 1 -> 0
>                                            kfree(s)  <--- freed!
> 
>   mutex_lock(&s->s_mutex)
>   UAF on freed s->s_mutex
> 
> Signed-off-by: Xiubo Li <xiubo.li@xxxxxxxxx>
> ---
>  fs/ceph/mds_client.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
> index 18be36c2e9e8..00a401f9f73b 100644
> --- a/fs/ceph/mds_client.c
> +++ b/fs/ceph/mds_client.c
> @@ -5827,9 +5827,11 @@ static void check_new_map(struct
> ceph_mds_client *mdsc,
>  			   ceph_mdsmap_get_addr(newmap, i),
>  			   sizeof(struct ceph_entity_addr))) {
>  			/* just close it */
> +			ceph_get_mds_session(s);
>  			mutex_unlock(&mdsc->mutex);
>  			mutex_lock(&s->s_mutex);
>  			mutex_lock(&mdsc->mutex);
> +			ceph_put_mds_session(s);
>  			ceph_con_close(&s->s_con);
>  			mutex_unlock(&s->s_mutex);
>  			s->s_state = CEPH_MDS_SESSION_RESTARTING;
> @@ -5844,6 +5846,7 @@ static void check_new_map(struct
> ceph_mds_client *mdsc,
>  		    newstate >= CEPH_MDS_STATE_RECONNECT) {
>  			int rc;
>  
> +			ceph_get_mds_session(s);
>  			mutex_unlock(&mdsc->mutex);
>  			clear_bit(i, targets);
>  			rc = send_mds_reconnect(mdsc, s);
> @@ -5852,6 +5855,7 @@ static void check_new_map(struct
> ceph_mds_client *mdsc,
>  					       "mds%d reconnect
> failed: %d\n",
>  					       i, rc);
>  			mutex_lock(&mdsc->mutex);
> +			ceph_put_mds_session(s);
>  		}
>  
>  		/*
> @@ -5864,9 +5868,11 @@ static void check_new_map(struct
> ceph_mds_client *mdsc,
>  				pr_info_client(cl, "mds%d recovery
> completed\n",
>  					       s->s_mds);
>  			kick_requests(mdsc, i);
> +			ceph_get_mds_session(s);
>  			mutex_unlock(&mdsc->mutex);
>  			mutex_lock(&s->s_mutex);
>  			mutex_lock(&mdsc->mutex);
> +			ceph_put_mds_session(s);
>  			ceph_kick_flushing_caps(mdsc, s);
>  			mutex_unlock(&s->s_mutex);
>  			wake_up_session_caps(s, RECONNECT);

Looks good.

Reviewed-by: Viacheslav Dubeyko <slava@xxxxxxxxxxx>

Thanks,
Slava.