Re: [PATCH v3] ceph: fix use-after-free bug for inodes when flushing capsnaps

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, Jun 6, 2023 at 2:55 AM <xiubli@xxxxxxxxxx> wrote:
>
> From: Xiubo Li <xiubli@xxxxxxxxxx>
>
> 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@xxxxxxxxxxxxxxx
> URL: https://bugzilla.redhat.com/show_bug.cgi?id=2209299
> Reviewed-by: Milind Changire <mchangir@xxxxxxxxxx>
> Signed-off-by: Xiubo Li <xiubli@xxxxxxxxxx>
> ---
>
> V3:
> - Fix two minor typo in commit comments.
>
>
>
>  fs/ceph/caps.c | 6 ++++++
>  fs/ceph/snap.c | 4 +++-
>  2 files changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/fs/ceph/caps.c b/fs/ceph/caps.c
> index feabf4cc0c4f..7c2cb813aba4 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 put = 0;

Hi Xiubo,

Nit: renaming this variable to need_put and making it a bool would
communicate the intent better.

>         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))
> +               put++;

What are the cases when ci is expected to not be on snap_flush_list
list (and therefore there is no corresponding reference to put)?

The reason I'm asking is that ceph_flush_snaps() is called from two
other places directly (i.e. without iterating snap_flush_list list) and
then __ceph_flush_snaps() is called from two yet other places.  The
problem that we are presented with here is that __ceph_flush_snaps()
effectively consumes a reference on ci.  Is ci protected from being
freed by handle_cap_flushsnap_ack() very soon after __send_flush_snap()
returns in all these other places?

Thanks,

                Ilya




[Index of Archives]     [CEPH Users]     [Ceph Large]     [Ceph Dev]     [Information on CEPH]     [Linux BTRFS]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux