Re: [PATCH v6 4/5] ceph: record subvolume quarantine from MDS cap updates

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] ceph: epoch self-invalidation in ceph_quarantine_reply_can_clear()
- [High] ceph: __ceph_remove_cap() resets quarantine mark during cap migration
--

commit 8a2e69935d967838e52792bb963b124cfe229b88
Author: Alex Markuze <amarkuze@redhat.com>

ceph: record subvolume quarantine from MDS cap updates

This commit decodes the signed oserrno from MClientCaps v14 messages to
record a sticky subvolume quarantine mark and reporting MDS rank on each
inode, along with epoch-based caching and RCU parent walks to resolve
inherited quarantine state.

> diff --git a/fs/ceph/caps.c b/fs/ceph/caps.c
> index b27bcdc24d29f..b07814d0038cd 100644
> --- a/fs/ceph/caps.c
> +++ b/fs/ceph/caps.c

[ ... ]

> @@ -1264,6 +1265,11 @@ static void __ceph_remove_cap(struct ceph_inode_info *ci, struct ceph_cap *cap,
>  
>  	mdsc = ceph_inode_to_fs_client(&ci->netfs.inode)->mdsc;
>  
> +	/* Losing the reporting cap requires fresh MDS revalidation. */
> +	if (READ_ONCE(ci->i_qtine_mds) == cap->mds &&
> +	    ceph_quarantine_reset(ci))
> +		wake_up_all(&ci->i_cap_wq);
> +
>  	/* remove from inode's cap rbtree, and clear auth cap */
>  	rb_erase(&cap->ci_node, &ci->i_caps);
>  	if (ci->i_auth_cap == cap)
> @@ -3729,6 +3735,9 @@ struct cap_extra_info {
>  	u8 *fscrypt_auth;
>  	u32 fscrypt_auth_len;
>  	u64 fscrypt_file_size;
> +	/* MClientCaps v14 oserrno; valid only when oserrno_valid is set */
> +	s32 oserrno;
> +	bool oserrno_valid;
>  };
>  
>  /*
> @@ -3757,6 +3766,46 @@ static int parse_inline_data(void **p, void *end,
>  	return -EIO;
>  }
>  
> +/*
> + * Record quarantine state carried by an MClientCaps oserrno.
> + * Caller holds i_ceph_lock. Returns true when waiters should be woken
> + * because quarantine changed or a pending reset was resolved.
> + *
> + * -EQUARANTINED refreshes the sticky flag and its reporting MDS rank.
> + * A current non-IMPORT grant above PIN with errno 0 clears that rank's mark.
> + * IMPORT may carry a default zero without assessing quarantine permission.
> + * Another rank needs a successful metadata request for the current epoch.
> + * A PIN-only update may zero qtine_errno while leaving the flag set.
> + */
> +static bool ceph_apply_quarantine(struct ceph_inode_info *ci, int mds,
> +				  int newcaps, int oserrno, bool is_import)
> +{
> +	bool was, now, pending;
> +
> +	was = READ_ONCE(ci->i_qtine_errno) == -CEPH_EQUARANTINED ||
> +	      test_bit(CEPH_I_QUARANTINE_BIT, &ci->i_ceph_flags);
> +	pending = test_bit(CEPH_I_QUARANTINE_PENDING_BIT, &ci->i_ceph_flags);
> +
> +	if (!is_import && (newcaps & ~CEPH_CAP_PIN) && was && oserrno == 0 &&
> +	    READ_ONCE(ci->i_qtine_mds) == mds)
> +		ceph_quarantine_clear(ci);
> +
> +	if (oserrno == -CEPH_EQUARANTINED) {
> +		ceph_quarantine_set(ci);
> +		WRITE_ONCE(ci->i_qtine_mds, mds);
> +	}

[Severity: High]
When a quarantined inode's capability is migrated between MDS ranks via
handle_cap_export() or handle_cap_import(), removing the exporting rank's
cap calls ceph_remove_cap() -> __ceph_remove_cap(), which sees
ci->i_qtine_mds == cap->mds and calls ceph_quarantine_reset(ci).

When handle_cap_grant() then processes the CEPH_CAP_OP_IMPORT message from
the new authoritative rank with the default oserrno == 0,
ceph_apply_quarantine() leaves CEPH_I_QUARANTINE_PENDING_BIT set and keeps
ci->i_qtine_mds set to the old exporting rank.

As a result:
1. ceph_inode_quarantine_direct() returns false and
   ceph_quarantine_walk_once() breaks out with CEPH_QWALK_INCOMPLETE instead
   of reporting the retained quarantine mark as CEPH_Q_UNDER.
2. Subsequent normal CEPH_CAP_OP_GRANT messages (above CEPH_CAP_PIN with
   oserrno == 0) and CEPH_MSG_CLIENT_QUARANTINE_DISABLE messages from the
   new authoritative rank cannot clear the mark in ceph_apply_quarantine()
   or ceph_handle_quarantine_disable() because ci->i_qtine_mds still holds
   the pre-migration rank.

Should capability migration preserve the active mark and transfer
ci->i_qtine_mds to the importing rank instead of resetting the mark to
pending when the old rank's cap is removed?

[ ... ]

> diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c
> index 94f5215f8eed5..048e0d502eb4e 100644
> --- a/fs/ceph/inode.c
> +++ b/fs/ceph/inode.c

[ ... ]

> +static bool ceph_quarantine_reply_can_clear(struct ceph_inode_info *ci,
> +					    struct ceph_mds_session *session)
> +{
> +	struct ceph_mds_client *mdsc = ceph_sb_to_mdsc(ci->netfs.inode.i_sb);
> +	struct ceph_mds_request *req;
> +	u64 epoch;
> +
> +	if (READ_ONCE(ci->i_qtine_mds) == session->s_mds)
> +		return true;
> +	/* Different ranks have independent cap sequences. */
> +	req = ceph_current_mds_request(mdsc->fsc);
> +	epoch = (u64)atomic64_read_acquire(&mdsc->quarantine_epoch);
> +	return req && !(epoch & 1) && req->r_quarantine_epoch == epoch &&
> +		!atomic_read(&mdsc->quarantine_splices);
> +}

[ ... ]

> @@ -1407,7 +1873,7 @@ int ceph_fill_inode(struct inode *inode, struct folio *locked_folio,
>  	/* were we issued a capability? */
>  	if (info_caps) {
>  		if (!ceph_in_snap(inode)) {
> -			ceph_add_cap(inode, session,
> +			bool cap_current = ceph_add_cap(inode, session,
>  				     le64_to_cpu(info->cap.cap_id),
>  				     info_caps,
>  				     le32_to_cpu(info->cap.wanted),
> @@ -1416,6 +1882,14 @@ int ceph_fill_inode(struct inode *inode, struct folio *locked_folio,
>  				     le64_to_cpu(info->cap.realm),
>  				     info->cap.flags, &new_cap);
>  
> +			/*
> +			 * Caps above PIN mean quarantine has been lifted,
> +			 * including when the disable message was lost.
> +			 */
> +			if (cap_current && (info_caps & ~CEPH_CAP_PIN) &&
> +			    ceph_quarantine_reply_can_clear(ci, session))
> +				ceph_quarantine_clear(ci);

[Severity: High]
Can ceph_quarantine_reply_can_clear() fail to clear quarantine on target
inodes and readdir entries when the reply comes from a rank different from
ci->i_qtine_mds because reply processing itself increments
mdsc->quarantine_epoch?

req->r_quarantine_epoch is sampled once in ceph_mdsc_create_request() and
never updated:

  req->r_quarantine_epoch = atomic64_read_acquire(&mdsc->quarantine_epoch);

During reply handling, ceph_fill_trace() processes parent_dir before
req->r_target_inode:

  ceph_fill_trace()
    if (parent_dir)
      ceph_fill_inode(parent_dir, ...)
        ceph_quarantine_clear(ci) -> advances mdsc->quarantine_epoch by 2
    ...
    if (rinfo->head->is_target)
      ceph_fill_inode(in, ...)
        ceph_quarantine_reply_can_clear(ci, session)

If both parent_dir and req->r_target_inode had CEPH_I_QUARANTINE_BIT set on
another rank, clearing parent_dir increments mdsc->quarantine_epoch by 2.
When ceph_fill_inode() then runs for req->r_target_inode,
req->r_quarantine_epoch == epoch is false, leaving the target inode's
quarantine mark set.

The same self-invalidation happens in handle_reply() ->
ceph_readdir_prepopulate(), where each entry in the loop calls
ceph_fill_inode() and splice_dentry() (which calls
ceph_quarantine_splice_start() and ceph_quarantine_splice_finish(),
advancing mdsc->quarantine_epoch by 4), preventing
ceph_quarantine_reply_can_clear() from succeeding on subsequent entries in
the same readdir reply.

In addition, any unrelated dentry splice or cross-directory rename on the
superblock while the MDS request is in flight (or sampling an odd
mdsc->quarantine_epoch in ceph_mdsc_create_request()) will cause
req->r_quarantine_epoch == epoch to fail even when no new quarantine mark was
set.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.20261025T182649Z.ca9ae7a18449.git.amarkuze@redhat.com?part=4