From: Patrick Farrell Date: Wed, 3 Aug 2022 16:48:13 +0000 (-0400) Subject: LU-15811 llite: Refactor DIO/AIO free code X-Git-Tag: 2.15.52~68 X-Git-Url: https://git.whamcloud.com/?p=fs%2Flustre-release.git;a=commitdiff_plain;h=f1c8ac1156ebea2b8e94a75f056c403973bb9010 LU-15811 llite: Refactor DIO/AIO free code Refactor the DIO/AIO free code and add some asserts. This removes a potential use-after-free in the freeing code. Signed-off-by: Patrick Farrell Change-Id: I335b18fc7a28fc426a25675e2449d3d192cba596 Reviewed-on: https://review.whamcloud.com/48115 Tested-by: jenkins Tested-by: Maloo Reviewed-by: Andreas Dilger Reviewed-by: Yingjin Qian Reviewed-by: Oleg Drokin --- diff --git a/lustre/include/cl_object.h b/lustre/include/cl_object.h index 1a76b18..2ef9c84 100644 --- a/lustre/include/cl_object.h +++ b/lustre/include/cl_object.h @@ -2531,10 +2531,9 @@ int cl_sync_io_wait_recycle(const struct lu_env *env, struct cl_sync_io *anchor, long timeout, int ioret); struct cl_dio_aio *cl_dio_aio_alloc(struct kiocb *iocb, struct cl_object *obj, bool is_aio); -struct cl_sub_dio *cl_sub_dio_alloc(struct cl_dio_aio *ll_aio, bool nofree); -void cl_dio_aio_free(const struct lu_env *env, struct cl_dio_aio *aio, - bool always_free); -void cl_sub_dio_free(struct cl_sub_dio *sdio, bool nofree); +struct cl_sub_dio *cl_sub_dio_alloc(struct cl_dio_aio *ll_aio, bool sync); +void cl_dio_aio_free(const struct lu_env *env, struct cl_dio_aio *aio); +void cl_sub_dio_free(struct cl_sub_dio *sdio); static inline void cl_sync_io_init(struct cl_sync_io *anchor, int nr) { cl_sync_io_init_notify(anchor, nr, NULL, NULL); @@ -2579,7 +2578,7 @@ struct cl_dio_aio { struct kiocb *cda_iocb; ssize_t cda_bytes; unsigned cda_no_aio_complete:1, - cda_no_sub_free:1; + cda_creator_free:1; }; /* Sub-dio used for splitting DIO (and AIO, because AIO is DIO) according to @@ -2591,7 +2590,7 @@ struct cl_sub_dio { ssize_t csd_bytes; struct cl_dio_aio *csd_ll_aio; struct ll_dio_pages csd_dio_pages; - unsigned csd_no_free:1; + unsigned csd_creator_free:1; }; #if defined(HAVE_DIRECTIO_ITER) || defined(HAVE_IOV_ITER_RW) || \ defined(HAVE_DIRECTIO_2ARGS) diff --git a/lustre/llite/file.c b/lustre/llite/file.c index cd440c3..f451a38 100644 --- a/lustre/llite/file.c +++ b/lustre/llite/file.c @@ -1841,7 +1841,8 @@ out: cl_sync_io_note(env, &io->ci_dio_aio->cda_sync, rc == -EIOCBQUEUED ? 0 : rc); if (!is_aio) { - cl_dio_aio_free(env, io->ci_dio_aio, true); + LASSERT(io->ci_dio_aio->cda_creator_free); + cl_dio_aio_free(env, io->ci_dio_aio); io->ci_dio_aio = NULL; } } diff --git a/lustre/llite/rw26.c b/lustre/llite/rw26.c index f558984..28d6a30 100644 --- a/lustre/llite/rw26.c +++ b/lustre/llite/rw26.c @@ -487,8 +487,10 @@ ll_direct_IO_impl(struct kiocb *iocb, struct iov_iter *iter, int rw) &pvec->ldp_count, count); if (unlikely(result <= 0)) { cl_sync_io_note(env, &ldp_aio->csd_sync, result); - if (sync_submit) - cl_sub_dio_free(ldp_aio, true); + if (sync_submit) { + LASSERT(ldp_aio->csd_creator_free); + cl_sub_dio_free(ldp_aio); + } GOTO(out, result); } @@ -508,7 +510,8 @@ ll_direct_IO_impl(struct kiocb *iocb, struct iov_iter *iter, int rw) 0); if (result == 0 && rc2) result = rc2; - cl_sub_dio_free(ldp_aio, true); + LASSERT(ldp_aio->csd_creator_free); + cl_sub_dio_free(ldp_aio); } if (unlikely(result < 0)) GOTO(out, result); diff --git a/lustre/obdclass/cl_io.c b/lustre/obdclass/cl_io.c index 60aabf2..40e17de 100644 --- a/lustre/obdclass/cl_io.c +++ b/lustre/obdclass/cl_io.c @@ -1257,9 +1257,9 @@ struct cl_dio_aio *cl_dio_aio_alloc(struct kiocb *iocb, struct cl_object *obj, * no one is waiting (in the kernel) for this to complete * * in other cases, the last user is cl_sync_io_wait, and in - * that case, the caller frees the struct after that call + * that case, the creator frees the struct after that call */ - aio->cda_no_sub_free = !is_aio; + aio->cda_creator_free = !is_aio; cl_object_get(obj); aio->cda_obj = obj; @@ -1268,7 +1268,7 @@ struct cl_dio_aio *cl_dio_aio_alloc(struct kiocb *iocb, struct cl_object *obj, } EXPORT_SYMBOL(cl_dio_aio_alloc); -struct cl_sub_dio *cl_sub_dio_alloc(struct cl_dio_aio *ll_aio, bool nofree) +struct cl_sub_dio *cl_sub_dio_alloc(struct cl_dio_aio *ll_aio, bool sync) { struct cl_sub_dio *sdio; @@ -1284,25 +1284,24 @@ struct cl_sub_dio *cl_sub_dio_alloc(struct cl_dio_aio *ll_aio, bool nofree) sdio->csd_ll_aio = ll_aio; atomic_add(1, &ll_aio->cda_sync.csi_sync_nr); - sdio->csd_no_free = nofree; + sdio->csd_creator_free = sync; } return sdio; } EXPORT_SYMBOL(cl_sub_dio_alloc); -void cl_dio_aio_free(const struct lu_env *env, struct cl_dio_aio *aio, - bool always_free) +void cl_dio_aio_free(const struct lu_env *env, struct cl_dio_aio *aio) { - if (aio && (!aio->cda_no_sub_free || always_free)) { + if (aio) { cl_object_put(env, aio->cda_obj); OBD_SLAB_FREE_PTR(aio, cl_dio_aio_kmem); } } EXPORT_SYMBOL(cl_dio_aio_free); -void cl_sub_dio_free(struct cl_sub_dio *sdio, bool always_free) +void cl_sub_dio_free(struct cl_sub_dio *sdio) { - if (sdio && (!sdio->csd_no_free || always_free)) + if (sdio) OBD_SLAB_FREE_PTR(sdio, cl_sub_dio_kmem); } EXPORT_SYMBOL(cl_sub_dio_free); @@ -1351,7 +1350,10 @@ void cl_sync_io_note(const struct lu_env *env, struct cl_sync_io *anchor, LASSERT(atomic_read(&anchor->csi_sync_nr) > 0); if (atomic_dec_and_lock(&anchor->csi_sync_nr, &anchor->csi_waitq.lock)) { - void *dio_aio = NULL; + struct cl_sub_dio *sub_dio_aio = NULL; + struct cl_dio_aio *dio_aio = NULL; + void *csi_dio_aio = NULL; + bool creator_free = true; cl_sync_io_end_t *end_io = anchor->csi_end_io; @@ -1367,18 +1369,25 @@ void cl_sync_io_note(const struct lu_env *env, struct cl_sync_io *anchor, if (end_io) end_io(env, anchor); - dio_aio = anchor->csi_dio_aio; + csi_dio_aio = anchor->csi_dio_aio; + sub_dio_aio = csi_dio_aio; + dio_aio = csi_dio_aio; + + if (csi_dio_aio && end_io == cl_dio_aio_end) + creator_free = dio_aio->cda_creator_free; + else if (csi_dio_aio && end_io == cl_sub_dio_end) + creator_free = sub_dio_aio->csd_creator_free; spin_unlock(&anchor->csi_waitq.lock); - if (dio_aio) { - if (end_io == cl_dio_aio_end) - cl_dio_aio_free(env, - (struct cl_dio_aio *) dio_aio, - false); - else if (end_io == cl_sub_dio_end) - cl_sub_dio_free((struct cl_sub_dio *) dio_aio, - false); + if (csi_dio_aio) { + if (end_io == cl_dio_aio_end) { + if (!creator_free) + cl_dio_aio_free(env, dio_aio); + } else if (end_io == cl_sub_dio_end) { + if (!creator_free) + cl_sub_dio_free(sub_dio_aio); + } } } EXIT;