On 5/31/23 19:09, Ilya Dryomov wrote:

On Thu, May 25, 2023 at 4:45 AM xiubli@redhat.com wrote:

...

From: Xiubo Li xiubli@redhat.com

There is racy between capsnaps flush and removing the inode from 'mdsc->snap_flush_list' list:

Thread A                            Thread B

ceph_queue_cap_snap() -> allocate 'capsnapA' ->ihold('&ci->vfs_inode') ->add 'capsnapA' to 'ci->i_cap_snaps' ->add 'ci' to 'mdsc->snap_flush_list' ... ceph_flush_snaps() ->__ceph_flush_snaps() ->__send_flush_snap() handle_cap_flushsnap_ack() ->iput('&ci->vfs_inode') this also will release 'ci' ... ceph_handle_snap() ->flush_snaps() ->iterate 'mdsc->snap_flush_list' ->get the stale 'ci' ->remove 'ci' from ->ihold(&ci->vfs_inode) this 'mdsc->snap_flush_list' will WARNING

To fix this we will remove the 'ci' from 'mdsc->snap_flush_list' list just before '__send_flush_snaps()' to make sure the flushsnap 'ack' will always after removing the 'ci' from 'snap_flush_list'.

Hi Xiubo,

I'm not sure I'm following the logic here. If the issue is that the inode can be released by handle_cap_flushsnap_ack(), meaning that ci is unsafe to dereference after the ack is received, what makes e.g. the following snippet in __ceph_flush_snaps() work:

 ret = __send_flush_snap(inode, session, capsnap, cap->mseq,
                         oldest_flush_tid);
 if (ret < 0) {
         pr_err("__flush_snaps: error sending cap flushsnap, "
                "ino (%llx.%llx) tid %llu follows %llu\n",
                 ceph_vinop(inode), cf->tid, capsnap->follows);
 }

 ceph_put_cap_snap(capsnap);
 spin_lock(&ci->i_ceph_lock);

If the ack is handled after capsnap is put but before ci->i_ceph_lock is reacquired, could use-after-free occur inside spin_lock()?

Yeah, certainly this could happen.

After the 'ci' being freed it's possible that the 'ci' is still cached in the 'ceph_inode_cachep' slab caches or reused by others, so dereferring the 'ci' mostly won't crash. But just before freeing 'ci' the 'ci->vfs_inode.i_count' will always be 0 already, this is why we can see the 'WARN_ON(atomic_inc_return(&inode->i_count) < 2)' call trace much easier in use-after-free case.

Thanks

- Xiubo

Thanks,

             Ilya

...

Cc: stable@vger.kernel.org URL: https://bugzilla.redhat.com/show_bug.cgi?id=2209299 Signed-off-by: Xiubo Li xiubli@redhat.com

---

fs/ceph/caps.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/fs/ceph/caps.c b/fs/ceph/caps.c index feabf4cc0c4f..a8f890b3bb9a 100644 --- a/fs/ceph/caps.c +++ b/fs/ceph/caps.c @@ -1595,6 +1595,11 @@ static void __ceph_flush_snaps(struct ceph_inode_info *ci,

     dout("__flush_snaps %p session %p\n", inode, session);

•   /* we will flush them all; remove this inode from the queue */
  

•   spin_lock(&mdsc->snap_flush_lock);
  

•   list_del_init(&ci->i_snap_flush_item);
  

•   spin_unlock(&mdsc->snap_flush_lock);
  

•    list_for_each_entry(capsnap, &ci->i_cap_snaps, ci_item) {
             /*
              * we need to wait for sync writes to complete and for dirty
  

@@ -1726,10 +1731,6 @@ void ceph_flush_snaps(struct ceph_inode_info *ci, *psession = session; else ceph_put_mds_session(session);

•   /* we flushed them all; remove this inode from the queue */
  

•   spin_lock(&mdsc->snap_flush_lock);
  

•   list_del_init(&ci->i_snap_flush_item);
  

•   spin_unlock(&mdsc->snap_flush_lock);
  

  }

  /*

-- 2.40.1