| Message ID | 20230607025434.1119867-1-xiubli@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v4] ceph: fix use-after-free bug for inodes when flushing capsnaps | expand |
On Wed, Jun 7, 2023 at 4:57 AM <xiubli@redhat.com> wrote: > > From: Xiubo Li <xiubli@redhat.com> > > There is a race 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' > ... > == Thread C == > ceph_flush_snaps() > ->__ceph_flush_snaps() > ->__send_flush_snap() > handle_cap_flushsnap_ack() > ->iput('&ci->vfs_inode') > this also will release 'ci' > ... > == Thread D == > 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 increase the inode's i_count ref when adding 'ci' > to the 'mdsc->snap_flush_list' list. > > Cc: stable@vger.kernel.org > URL: https://bugzilla.redhat.com/show_bug.cgi?id=2209299 > Reviewed-by: Milind Changire <mchangir@redhat.com> > Signed-off-by: Xiubo Li <xiubli@redhat.com> > --- > > V4: > - s/put/need_put/ Hi Xiubo, The other part of the suggestion was to make it a bool. I made the adjustment and queued up this patch for 6.4-rc6. Thanks, Ilya
On 6/8/23 15:07, Ilya Dryomov wrote: > On Wed, Jun 7, 2023 at 4:57 AM <xiubli@redhat.com> wrote: >> From: Xiubo Li <xiubli@redhat.com> >> >> There is a race 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' >> ... >> == Thread C == >> ceph_flush_snaps() >> ->__ceph_flush_snaps() >> ->__send_flush_snap() >> handle_cap_flushsnap_ack() >> ->iput('&ci->vfs_inode') >> this also will release 'ci' >> ... >> == Thread D == >> 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 increase the inode's i_count ref when adding 'ci' >> to the 'mdsc->snap_flush_list' list. >> >> Cc: stable@vger.kernel.org >> URL: https://bugzilla.redhat.com/show_bug.cgi?id=2209299 >> Reviewed-by: Milind Changire <mchangir@redhat.com> >> Signed-off-by: Xiubo Li <xiubli@redhat.com> >> --- >> >> V4: >> - s/put/need_put/ > Hi Xiubo, > > The other part of the suggestion was to make it a bool. I made the > adjustment and queued up this patch for 6.4-rc6. Sure, thanks Ilya. - Xiubo > Thanks, > > Ilya >
diff --git a/fs/ceph/caps.c b/fs/ceph/caps.c index 8c1fc64b290c..bb78d0fbfa62 100644 --- a/fs/ceph/caps.c +++ b/fs/ceph/caps.c @@ -1684,6 +1684,7 @@ void ceph_flush_snaps(struct ceph_inode_info *ci, struct inode *inode = &ci->netfs.inode; struct ceph_mds_client *mdsc = ceph_inode_to_client(inode)->mdsc; struct ceph_mds_session *session = NULL; + int need_put = 0; int mds; dout("ceph_flush_snaps %p\n", inode); @@ -1728,8 +1729,13 @@ void ceph_flush_snaps(struct ceph_inode_info *ci, ceph_put_mds_session(session); /* we flushed them all; remove this inode from the queue */ spin_lock(&mdsc->snap_flush_lock); + if (!list_empty(&ci->i_snap_flush_item)) + need_put++; list_del_init(&ci->i_snap_flush_item); spin_unlock(&mdsc->snap_flush_lock); + + if (need_put) + iput(inode); } /* diff --git a/fs/ceph/snap.c b/fs/ceph/snap.c index 7943981914cf..e03b020d87d7 100644 --- a/fs/ceph/snap.c +++ b/fs/ceph/snap.c @@ -697,8 +697,10 @@ int __ceph_finish_cap_snap(struct ceph_inode_info *ci, capsnap->size); spin_lock(&mdsc->snap_flush_lock); - if (list_empty(&ci->i_snap_flush_item)) + if (list_empty(&ci->i_snap_flush_item)) { + ihold(inode); list_add_tail(&ci->i_snap_flush_item, &mdsc->snap_flush_list); + } spin_unlock(&mdsc->snap_flush_lock); return 1; /* caller may want to ceph_flush_snaps */ }