On Tue, 2025-08-12 at 09:57 +0000, Alex Markuze wrote:
> When the parent directory's i_rwsem is not locked, req->r_parent may become
> stale due to concurrent operations (e.g. rename) between dentry lookup and
> message creation. Validate that r_parent matches the encoded parent inode
> and update to the correct inode if a mismatch is detected.
>
> Signed-off-by: Alex Markuze <amarkuze@xxxxxxxxxx>
> ---
> fs/ceph/inode.c | 52 +++++++++++++++++++++++++++++++++++++++++++++++--
> 1 file changed, 50 insertions(+), 2 deletions(-)
>
Looks good.
Reviewed-by: Viacheslav Dubeyko <Slava.Dubeyko@xxxxxxx>
Thanks,
Slava.
> diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c
> index 36981950a368..42110d133f15 100644
> --- a/fs/ceph/inode.c
> +++ b/fs/ceph/inode.c
> @@ -56,6 +56,51 @@ static int ceph_set_ino_cb(struct inode *inode, void *data)
> return 0;
> }
>
> +/*
> + * Check if the parent inode matches the vino from directory reply info
> + */
> +static inline bool ceph_vino_matches_parent(struct inode *parent, struct ceph_vino vino)
> +{
> + return ceph_ino(parent) == vino.ino && ceph_snap(parent) == vino.snap;
> +}
> +
> +/*
> + * Validate that the directory inode referenced by @req->r_parent matches the
> + * inode number and snapshot id contained in the reply's directory record. If
> + * they do not match – which can theoretically happen if the parent dentry was
> + * moved between the time the request was issued and the reply arrived – fall
> + * back to looking up the correct inode in the inode cache.
> + *
> + * A reference is *always* returned. Callers that receive a different inode
> + * than the original @parent are responsible for dropping the extra reference
> + * once the reply has been processed.
> + */
> +static struct inode *ceph_get_reply_dir(struct super_block *sb,
> + struct inode *parent,
> + struct ceph_mds_reply_info_parsed *rinfo)
> +{
> + struct ceph_vino vino;
> +
> + if (unlikely(!rinfo->diri.in))
> + return parent; /* nothing to compare against */
> +
> + /* If we didn't have a cached parent inode to begin with, just bail out. */
> + if (!parent)
> + return NULL;
> +
> + vino.ino = le64_to_cpu(rinfo->diri.in->ino);
> + vino.snap = le64_to_cpu(rinfo->diri.in->snapid);
> +
> + if (likely(ceph_vino_matches_parent(parent, vino)))
> + return parent; /* matches – use the original reference */
> +
> + /* Mismatch – this should be rare. Emit a WARN and obtain the correct inode. */
> + WARN_ONCE(1, "ceph: reply dir mismatch (parent valid %llx.%llx reply %llx.%llx)\n",
> + ceph_ino(parent), ceph_snap(parent), vino.ino, vino.snap);
> +
> + return ceph_get_inode(sb, vino, NULL);
> +}
> +
> /**
> * ceph_new_inode - allocate a new inode in advance of an expected create
> * @dir: parent directory for new inode
> @@ -1548,8 +1593,11 @@ int ceph_fill_trace(struct super_block *sb, struct ceph_mds_request *req)
> }
>
> if (rinfo->head->is_dentry) {
> - struct inode *dir = req->r_parent;
> -
> + /*
> + * r_parent may be stale, in cases when R_PARENT_LOCKED is not set,
> + * so we need to get the correct inode
> + */
> + struct inode *dir = ceph_get_reply_dir(sb, req->r_parent, rinfo);
> if (dir) {
> err = ceph_fill_inode(dir, NULL, &rinfo->diri,
> rinfo->dirfrag, session, -1,
