Re: [PATCH 1/1] libceph: use RCU to protect monmap in ceph_compare_options()

On Wed, 2026-07-08 at 11:03 +0800, Ren Wei wrote:
> From: Yong Wang <[email protected]>
> 
> ceph_compare_options() checks whether a new mount shares any monitor
> address with an existing client by walking client->monc.monmap via
> ceph_monmap_contains().  That comparison can run under sb_lock or
> rbd_client_list_lock, so it cannot take monc->mutex.
> 
> Meanwhile, monmap update handling replaces monc->monmap under
> monc->mutex and frees the old map immediately.  A concurrent shared-
> mount comparison can therefore dereference a freed monmap and walk
> stale mon_inst[] entries, triggering a use-after-free.
> 
> Protect the compare path with RCU and publish/free monitor maps with
> rcu_assign_pointer() and kfree_rcu().  Annotate monc->monmap as an
> RCU pointer and use rcu_dereference_protected() in mutex-protected
> paths to keep the accesses consistent with the new pointer contract.
> 
> This keeps the existing non-blocking comparison semantics while
> ensuring that replaced monmaps remain alive until readers are done.

The approach makes sense to me. However, I have some concern. If the replaced
monmap(s) could be in use with newly allocated one(s), then are we safe here?

Have you tried to run xfstests for the patch?

Thanks,
Slava.

> 
> Fixes: 4e7a5dcd1bbab ("ceph: negotiate authentication protocol; implement AUTH_NONE protocol")
> Cc: [email protected]
> Cc: [email protected]
> Reported-by: Yuan Tan <[email protected]>
> Reported-by: Xin Liu <[email protected]>
> Assisted-by: Codex:gpt-5.4
> Signed-off-by: Yong Wang <[email protected]>
> Reviewed-by: Ren Wei <[email protected]>
> ---
>  fs/ceph/super.c                 |  7 +++--
>  include/linux/ceph/mon_client.h |  4 ++-
>  net/ceph/ceph_common.c          | 21 ++++++++++----
>  net/ceph/debugfs.c              | 11 ++++---
>  net/ceph/mon_client.c           | 51 ++++++++++++++++++++++-----------
>  5 files changed, 65 insertions(+), 29 deletions(-)
> 
> diff --git a/fs/ceph/super.c b/fs/ceph/super.c
> index c05fbd4237f8..4145594ba710 100644
> --- a/fs/ceph/super.c
> +++ b/fs/ceph/super.c
> @@ -60,6 +60,7 @@ static int ceph_statfs(struct dentry *dentry, struct kstatfs *buf)
>  {
>  	struct ceph_fs_client *fsc = ceph_inode_to_fs_client(d_inode(dentry));
>  	struct ceph_mon_client *monc = &fsc->client->monc;
> +	struct ceph_monmap *monmap;
>  	struct ceph_statfs st;
>  	int i, err;
>  	u64 data_pool;
> @@ -111,8 +112,10 @@ static int ceph_statfs(struct dentry *dentry, struct kstatfs *buf)
>  	/* Must convert the fsid, for consistent values across arches */
>  	buf->f_fsid.val[0] = 0;
>  	mutex_lock(&monc->mutex);
> -	for (i = 0 ; i < sizeof(monc->monmap->fsid) / sizeof(__le32) ; ++i)
> -		buf->f_fsid.val[0] ^= le32_to_cpu(((__le32 *)&monc->monmap->fsid)[i]);
> +	monmap = rcu_dereference_protected(monc->monmap,
> +					   lockdep_is_held(&monc->mutex));
> +	for (i = 0 ; i < sizeof(monmap->fsid) / sizeof(__le32) ; ++i)
> +		buf->f_fsid.val[0] ^= le32_to_cpu(((__le32 *)&monmap->fsid)[i]);
>  	mutex_unlock(&monc->mutex);
>  
>  	/* fold the fs_cluster_id into the upper bits */
> diff --git a/include/linux/ceph/mon_client.h b/include/linux/ceph/mon_client.h
> index 7a9a40163c0f..98a745f6d9ab 100644
> --- a/include/linux/ceph/mon_client.h
> +++ b/include/linux/ceph/mon_client.h
> @@ -4,6 +4,7 @@
>  
>  #include <linux/completion.h>
>  #include <linux/kref.h>
> +#include <linux/rcupdate.h>
>  #include <linux/rbtree.h>
>  
>  #include <linux/ceph/messenger.h>
> @@ -19,6 +20,7 @@ struct ceph_monmap {
>  	struct ceph_fsid fsid;
>  	u32 epoch;
>  	u32 num_mon;
> +	struct rcu_head rcu;
>  	struct ceph_entity_inst mon_inst[] __counted_by(num_mon);
>  };
>  
> @@ -69,7 +71,7 @@ struct ceph_mon_generic_request {
>  
>  struct ceph_mon_client {
>  	struct ceph_client *client;
> -	struct ceph_monmap *monmap;
> +	struct ceph_monmap __rcu *monmap;
>  
>  	struct mutex mutex;
>  	struct delayed_work delayed_work;
> diff --git a/net/ceph/ceph_common.c b/net/ceph/ceph_common.c
> index 952121849180..929df54cf3bb 100644
> --- a/net/ceph/ceph_common.c
> +++ b/net/ceph/ceph_common.c
> @@ -133,6 +133,7 @@ int ceph_compare_options(struct ceph_options *new_opt,
>  {
>  	struct ceph_options *opt1 = new_opt;
>  	struct ceph_options *opt2 = client->options;
> +	struct ceph_monmap *monmap;
>  	int ofs = offsetof(struct ceph_options, mon_addr);
>  	int i;
>  	int ret;
> @@ -180,13 +181,20 @@ int ceph_compare_options(struct ceph_options *new_opt,
>  	if (ret)
>  		return ret;
>  
> +	rcu_read_lock();
> +	monmap = rcu_dereference(client->monc.monmap);
> +	ret = -1;
> +
>  	/* any matching mon ip implies a match */
>  	for (i = 0; i < opt1->num_mon; i++) {
> -		if (ceph_monmap_contains(client->monc.monmap,
> -				 &opt1->mon_addr[i]))
> -			return 0;
> +		if (ceph_monmap_contains(monmap, &opt1->mon_addr[i])) {
> +			ret = 0;
> +			break;
> +		}
>  	}
> -	return -1;
> +
> +	rcu_read_unlock();
> +	return ret;
>  }
>  EXPORT_SYMBOL(ceph_compare_options);
>  
> @@ -791,6 +799,7 @@ int __ceph_open_session(struct ceph_client *client)
>  {
>  	DEFINE_WAIT_FUNC(wait, woken_wake_function);
>  	long timeout = ceph_timeout_jiffies(client->options->mount_timeout);
> +	struct ceph_monmap *monmap;
>  	bool have_monmap, have_osdmap;
>  	int err;
>  
> @@ -803,7 +812,9 @@ int __ceph_open_session(struct ceph_client *client)
>  	for (;;) {
>  		mutex_lock(&client->monc.mutex);
>  		err = client->auth_err;
> -		have_monmap = client->monc.monmap && client->monc.monmap->epoch;
> +		monmap = rcu_dereference_protected(client->monc.monmap,
> +						   lockdep_is_held(&client->monc.mutex));
> +		have_monmap = monmap && monmap->epoch;
>  		mutex_unlock(&client->monc.mutex);
>  
>  		down_read(&client->osdc.lock);
> diff --git a/net/ceph/debugfs.c b/net/ceph/debugfs.c
> index 83c270bce63c..f0aa4631cf6e 100644
> --- a/net/ceph/debugfs.c
> +++ b/net/ceph/debugfs.c
> @@ -35,15 +35,18 @@ static int monmap_show(struct seq_file *s, void *p)
>  {
>  	int i;
>  	struct ceph_client *client = s->private;
> +	struct ceph_monmap *monmap;
>  
>  	mutex_lock(&client->monc.mutex);
> -	if (client->monc.monmap == NULL)
> +	monmap = rcu_dereference_protected(client->monc.monmap,
> +					   lockdep_is_held(&client->monc.mutex));
> +	if (!monmap)
>  		goto out_unlock;
>  
> -	seq_printf(s, "epoch %d\n", client->monc.monmap->epoch);
> -	for (i = 0; i < client->monc.monmap->num_mon; i++) {
> +	seq_printf(s, "epoch %d\n", monmap->epoch);
> +	for (i = 0; i < monmap->num_mon; i++) {
>  		struct ceph_entity_inst *inst =
> -			&client->monc.monmap->mon_inst[i];
> +			&monmap->mon_inst[i];
>  
>  		seq_printf(s, "\t%s%lld\t%s\n",
>  			   ENTITY_NAME(inst->name),
> diff --git a/net/ceph/mon_client.c b/net/ceph/mon_client.c
> index d2cdc8ee3155..d7e600a74ca5 100644
> --- a/net/ceph/mon_client.c
> +++ b/net/ceph/mon_client.c
> @@ -206,19 +206,22 @@ static void __close_session(struct ceph_mon_client *monc)
>   */
>  static void pick_new_mon(struct ceph_mon_client *monc)
>  {
> +	struct ceph_monmap *monmap =
> +		rcu_dereference_protected(monc->monmap,
> +					  lockdep_is_held(&monc->mutex));
>  	int old_mon = monc->cur_mon;
>  
> -	BUG_ON(monc->monmap->num_mon < 1);
> +	BUG_ON(monmap->num_mon < 1);
>  
> -	if (monc->monmap->num_mon == 1) {
> +	if (monmap->num_mon == 1) {
>  		monc->cur_mon = 0;
>  	} else {
> -		int max = monc->monmap->num_mon;
> +		int max = monmap->num_mon;
>  		int o = -1;
>  		int n;
>  
>  		if (monc->cur_mon >= 0) {
> -			if (monc->cur_mon < monc->monmap->num_mon)
> +			if (monc->cur_mon < monmap->num_mon)
>  				o = monc->cur_mon;
>  			if (o >= 0)
>  				max--;
> @@ -232,7 +235,7 @@ static void pick_new_mon(struct ceph_mon_client *monc)
>  	}
>  
>  	dout("%s mon%d -> mon%d out of %d mons\n", __func__, old_mon,
> -	     monc->cur_mon, monc->monmap->num_mon);
> +	     monc->cur_mon, monmap->num_mon);
>  }
>  
>  /*
> @@ -240,6 +243,9 @@ static void pick_new_mon(struct ceph_mon_client *monc)
>   */
>  static void __open_session(struct ceph_mon_client *monc)
>  {
> +	struct ceph_monmap *monmap =
> +		rcu_dereference_protected(monc->monmap,
> +					  lockdep_is_held(&monc->mutex));
>  	int ret;
>  
>  	pick_new_mon(monc);
> @@ -256,7 +262,7 @@ static void __open_session(struct ceph_mon_client *monc)
>  
>  	dout("%s opening mon%d\n", __func__, monc->cur_mon);
>  	ceph_con_open(&monc->con, CEPH_ENTITY_TYPE_MON, monc->cur_mon,
> -		      &monc->monmap->mon_inst[monc->cur_mon].addr);
> +		      &monmap->mon_inst[monc->cur_mon].addr);
>  
>  	/*
>  	 * Queue a keepalive to ensure that in case of an early fault
> @@ -542,6 +548,7 @@ static void ceph_monc_handle_map(struct ceph_mon_client *monc,
>  				 struct ceph_msg *msg)
>  {
>  	struct ceph_client *client = monc->client;
> +	struct ceph_monmap *old_monmap;
>  	struct ceph_monmap *monmap;
>  	void *p, *end;
>  
> @@ -564,10 +571,12 @@ static void ceph_monc_handle_map(struct ceph_mon_client *monc,
>  		goto out;
>  	}
>  
> -	kfree(monc->monmap);
> -	monc->monmap = monmap;
> +	old_monmap = rcu_dereference_protected(monc->monmap,
> +					       lockdep_is_held(&monc->mutex));
> +	rcu_assign_pointer(monc->monmap, monmap);
> +	kfree_rcu(old_monmap, rcu);
>  
> -	__ceph_monc_got_map(monc, CEPH_SUB_MONMAP, monc->monmap->epoch);
> +	__ceph_monc_got_map(monc, CEPH_SUB_MONMAP, monmap->epoch);
>  	client->have_fsid = true;
>  
>  out:
> @@ -775,6 +784,7 @@ static void handle_statfs_reply(struct ceph_mon_client *monc,
>  int ceph_monc_do_statfs(struct ceph_mon_client *monc, u64 data_pool,
>  			struct ceph_statfs *buf)
>  {
> +	struct ceph_monmap *monmap;
>  	struct ceph_mon_generic_request *req;
>  	struct ceph_mon_statfs *h;
>  	int ret = -ENOMEM;
> @@ -802,7 +812,9 @@ int ceph_monc_do_statfs(struct ceph_mon_client *monc, u64 data_pool,
>  	h->monhdr.have_version = 0;
>  	h->monhdr.session_mon = cpu_to_le16(-1);
>  	h->monhdr.session_mon_tid = 0;
> -	h->fsid = monc->monmap->fsid;
> +	monmap = rcu_dereference_protected(monc->monmap,
> +					   lockdep_is_held(&monc->mutex));
> +	h->fsid = monmap->fsid;
>  	h->contains_data_pool = (data_pool != CEPH_NOPOOL);
>  	h->data_pool = cpu_to_le64(data_pool);
>  	send_generic_request(monc, req);
> @@ -975,6 +987,7 @@ static __printf(2, 0)
>  int do_mon_command_vargs(struct ceph_mon_client *monc, const char *fmt,
>  			 va_list ap)
>  {
> +	struct ceph_monmap *monmap;
>  	struct ceph_mon_generic_request *req;
>  	struct ceph_mon_command *h;
>  	int ret = -ENOMEM;
> @@ -999,7 +1012,9 @@ int do_mon_command_vargs(struct ceph_mon_client *monc, const char *fmt,
>  	h->monhdr.have_version = 0;
>  	h->monhdr.session_mon = cpu_to_le16(-1);
>  	h->monhdr.session_mon_tid = 0;
> -	h->fsid = monc->monmap->fsid;
> +	monmap = rcu_dereference_protected(monc->monmap,
> +					   lockdep_is_held(&monc->mutex));
> +	h->fsid = monmap->fsid;
>  	h->num_strs = cpu_to_le32(1);
>  	len = vsprintf(h->str, fmt, ap);
>  	h->str_len = cpu_to_le32(len);
> @@ -1138,17 +1153,18 @@ static int build_initial_monmap(struct ceph_mon_client *monc)
>  	__le32 my_type = ceph_msgr2(monc->client) ?
>  		CEPH_ENTITY_ADDR_TYPE_MSGR2 : CEPH_ENTITY_ADDR_TYPE_LEGACY;
>  	struct ceph_options *opt = monc->client->options;
> +	struct ceph_monmap *monmap;
>  	int num_mon = opt->num_mon;
>  	int i;
>  
>  	/* build initial monmap */
> -	monc->monmap = kzalloc_flex(*monc->monmap, mon_inst, num_mon);
> -	if (!monc->monmap)
> +	monmap = kzalloc_flex(*monmap, mon_inst, num_mon);
> +	if (!monmap)
>  		return -ENOMEM;
> -	monc->monmap->num_mon = num_mon;
> +	monmap->num_mon = num_mon;
>  
>  	for (i = 0; i < num_mon; i++) {
> -		struct ceph_entity_inst *inst = &monc->monmap->mon_inst[i];
> +		struct ceph_entity_inst *inst = &monmap->mon_inst[i];
>  
>  		memcpy(&inst->addr.in_addr, &opt->mon_addr[i].in_addr,
>  		       sizeof(inst->addr.in_addr));
> @@ -1157,6 +1173,7 @@ static int build_initial_monmap(struct ceph_mon_client *monc)
>  		inst->name.type = CEPH_ENTITY_TYPE_MON;
>  		inst->name.num = cpu_to_le64(i);
>  	}
> +	RCU_INIT_POINTER(monc->monmap, monmap);
>  	return 0;
>  }
>  
> @@ -1232,7 +1249,7 @@ int ceph_monc_init(struct ceph_mon_client *monc, struct ceph_client *cl)
>  out_auth:
>  	ceph_auth_destroy(monc->auth);
>  out_monmap:
> -	kfree(monc->monmap);
> +	kfree(rcu_access_pointer(monc->monmap));
>  out:
>  	return err;
>  }
> @@ -1267,7 +1284,7 @@ void ceph_monc_stop(struct ceph_mon_client *monc)
>  	ceph_msg_put(monc->m_subscribe);
>  	ceph_msg_put(monc->m_subscribe_ack);
>  
> -	kfree(monc->monmap);
> +	kfree_rcu(rcu_access_pointer(monc->monmap), rcu);
>  }
>  EXPORT_SYMBOL(ceph_monc_stop);
>