From e3a9d87370c4ccc58d1d3a97ea1b221d88f9e57a Mon Sep 17 00:00:00 2001 From: Andreas Dilger Date: Fri, 26 Apr 2024 18:13:52 -0700 Subject: [PATCH] LU-17504 build: fix lock_handle array-index-out-of-bounds After Linux kernel patch "ubsan: Tighten UBSAN_BOUNDS on GCC" (commit v6.4-rc2-1-g2d47c6956ab3), flexible trailing arrays declared like 'lock_handle[2]' will generate warnings when CONFIG_UBSAN & co. is enabled: UBSAN: array-index-out-of-bounds in ldlm_request.c:1282:18 index 2 is out of range for type 'lustre_handle [2]' The declaration lock_handle[LDLM_LOCKREQ_HANDLES] confuses the compiler into thinking there are only two fields in lock_handle, but the caller often allocates extra fields beyond this for more locks to be cancelled due to Early Lock Cancellation or from LRU. Rather than have a second flexible array after lustre_handle[2], declare the whole array as flexible, and fix up the few sites that are allocating this array to ensure LDLM_LOCKREQ_HANDLES fields are allocated at a minimum. This subtly changes the checks in wiretest.c due to the removal of the 2 "base" handles in ldlm_request, but I believe this is not changing the wire protocol because it still allocates those handles directly, and I have verified interoperability with a 2.14.0 server. Test-Parameters: testlist=runtests clientversion=2.14 Test-Parameters: testlist=runtests serverversion=2.14 Test-Parameters: testlist=runtests clientversion=2.15 Test-Parameters: testlist=runtests serverversion=2.15 Test-Parameters: testlist=runtests clientversion=EXA5 Test-Parameters: testlist=runtests serverversion=EXA5 Test-Parameters: testlist=runtests clientversion=EXA6 Test-Parameters: testlist=runtests serverversion=EXA6 Signed-off-by: Andreas Dilger Change-Id: I9695fb44f1b5c84bb750d2983cdd8b939e3ebbe5 Reviewed-on: https://review.whamcloud.com/c/fs/lustre-release/+/54926 Tested-by: jenkins Tested-by: Maloo Reviewed-by: Jian Yu Reviewed-by: Shaun Tancheff Reviewed-by: Yang Sheng Reviewed-by: James Simmons Reviewed-by: Oleg Drokin --- lustre/include/uapi/linux/lustre/lustre_idl.h | 4 ++-- lustre/ldlm/ldlm_lockd.c | 5 ++--- lustre/ldlm/ldlm_request.c | 17 +++++++---------- lustre/ptlrpc/layout.c | 3 ++- lustre/ptlrpc/wiretest.c | 7 ++++--- lustre/utils/wirecheck.c | 2 +- lustre/utils/wiretest.c | 7 ++++--- 7 files changed, 22 insertions(+), 23 deletions(-) diff --git a/lustre/include/uapi/linux/lustre/lustre_idl.h b/lustre/include/uapi/linux/lustre/lustre_idl.h index 8be4759..bc88bd1 100644 --- a/lustre/include/uapi/linux/lustre/lustre_idl.h +++ b/lustre/include/uapi/linux/lustre/lustre_idl.h @@ -2636,8 +2636,8 @@ struct ldlm_lock_desc { struct ldlm_request { __u32 lock_flags; /* LDLM_FL_*,(lustre_dlm_flags.h) */ __u32 lock_count; /* Num of locks in lock_handle[] */ - struct ldlm_lock_desc lock_desc; /* lock descriptor */ - struct lustre_handle lock_handle[LDLM_LOCKREQ_HANDLES]; + struct ldlm_lock_desc lock_desc; /* lock descriptor */ + struct lustre_handle lock_handle[]; /* was LDLM_LOCKREQ_HANDLES */ }; struct ldlm_reply { diff --git a/lustre/ldlm/ldlm_lockd.c b/lustre/ldlm/ldlm_lockd.c index d7a1944..ca59464 100644 --- a/lustre/ldlm/ldlm_lockd.c +++ b/lustre/ldlm/ldlm_lockd.c @@ -1769,9 +1769,8 @@ int ldlm_request_cancel(struct ptlrpc_request *req, ENTRY; size = req_capsule_get_size(&req->rq_pill, &RMF_DLM_REQ, RCL_CLIENT); - if (size <= offsetof(struct ldlm_request, lock_handle) || - (size - offsetof(struct ldlm_request, lock_handle)) / - sizeof(struct lustre_handle) < dlm_req->lock_count) + if (size <= sizeof(*dlm_req) || dlm_req->lock_count > + (size - sizeof(*dlm_req)) / sizeof(struct lustre_handle)) RETURN(0); count = dlm_req->lock_count ? dlm_req->lock_count : 1; diff --git a/lustre/ldlm/ldlm_request.c b/lustre/ldlm/ldlm_request.c index e51cd98..8ae1c88 100644 --- a/lustre/ldlm/ldlm_request.c +++ b/lustre/ldlm/ldlm_request.c @@ -88,24 +88,21 @@ struct ldlm_async_args { * LDLM_LOCKREQ_HANDLE -1 slots are available. * Otherwise, LDLM_LOCKREQ_HANDLE slots are available. * - * \param[in] count - * \param[in] type + * \param[in] count - total number of lock handles to include for cancel + * \param[in] type - LDLM RPC request type * * \retval size of the request buffer */ static int ldlm_request_bufsize(int count, int type) { - int avail = LDLM_LOCKREQ_HANDLES; - if (type == LDLM_ENQUEUE) - avail -= LDLM_ENQUEUE_CANCEL_OFF; + count++; - if (count > avail) - avail = (count - avail) * sizeof(struct lustre_handle); - else - avail = 0; + /* keep minimum handles to keep struct size for compatibility */ + if (count < LDLM_LOCKREQ_HANDLES) + count = LDLM_LOCKREQ_HANDLES; - return sizeof(struct ldlm_request) + avail; + return offsetof(struct ldlm_request, lock_handle[count]); } static void ldlm_expired_completion_wait(struct lock_wait_data *lwd) diff --git a/lustre/ptlrpc/layout.c b/lustre/ptlrpc/layout.c index 4fc1c3c..42e0fa3 100644 --- a/lustre/ptlrpc/layout.c +++ b/lustre/ptlrpc/layout.c @@ -1147,7 +1147,8 @@ EXPORT_SYMBOL(RMF_CONNECT_DATA); struct req_msg_field RMF_DLM_REQ = DEFINE_MSGF("dlm_req", RMF_F_NO_SIZE_CHECK /* ldlm_request_bufsize */, - sizeof(struct ldlm_request), + offsetof(struct ldlm_request, + lock_handle[LDLM_LOCKREQ_HANDLES]), lustre_swab_ldlm_request, NULL); EXPORT_SYMBOL(RMF_DLM_REQ); diff --git a/lustre/ptlrpc/wiretest.c b/lustre/ptlrpc/wiretest.c index c85dc3b..b37eee1 100644 --- a/lustre/ptlrpc/wiretest.c +++ b/lustre/ptlrpc/wiretest.c @@ -3847,7 +3847,7 @@ void lustre_assert_wire_constants(void) (long long)(int)sizeof(((struct ldlm_lock_desc *)0)->l_policy_data)); /* Checks for struct ldlm_request */ - LASSERTF((int)sizeof(struct ldlm_request) == 104, "found %lld\n", + LASSERTF((int)sizeof(struct ldlm_request) == 88, "found %lld\n", (long long)(int)sizeof(struct ldlm_request)); LASSERTF((int)offsetof(struct ldlm_request, lock_flags) == 0, "found %lld\n", (long long)(int)offsetof(struct ldlm_request, lock_flags)); @@ -3863,8 +3863,9 @@ void lustre_assert_wire_constants(void) (long long)(int)sizeof(((struct ldlm_request *)0)->lock_desc)); LASSERTF((int)offsetof(struct ldlm_request, lock_handle) == 88, "found %lld\n", (long long)(int)offsetof(struct ldlm_request, lock_handle)); - LASSERTF((int)sizeof(((struct ldlm_request *)0)->lock_handle) == 16, "found %lld\n", - (long long)(int)sizeof(((struct ldlm_request *)0)->lock_handle)); + LASSERTF((int)sizeof(*((struct ldlm_request *)0)->lock_handle) == 8, "found %lld\n", + (long long)(int)sizeof(*((struct ldlm_request *)0)->lock_handle)); + BUILD_BUG_ON(offsetof(struct ldlm_request, lock_handle) != sizeof(struct ldlm_request)); /* Checks for struct ldlm_reply */ LASSERTF((int)sizeof(struct ldlm_reply) == 112, "found %lld\n", diff --git a/lustre/utils/wirecheck.c b/lustre/utils/wirecheck.c index e427c56..994df70 100644 --- a/lustre/utils/wirecheck.c +++ b/lustre/utils/wirecheck.c @@ -1725,7 +1725,7 @@ check_ldlm_request(void) CHECK_MEMBER(ldlm_request, lock_flags); CHECK_MEMBER(ldlm_request, lock_count); CHECK_MEMBER(ldlm_request, lock_desc); - CHECK_MEMBER(ldlm_request, lock_handle); + CHECK_MEMBER_IS_FLEXIBLE(ldlm_request, lock_handle); } static void diff --git a/lustre/utils/wiretest.c b/lustre/utils/wiretest.c index 42c4154..68d686d 100644 --- a/lustre/utils/wiretest.c +++ b/lustre/utils/wiretest.c @@ -3872,7 +3872,7 @@ void lustre_assert_wire_constants(void) (long long)(int)sizeof(((struct ldlm_lock_desc *)0)->l_policy_data)); /* Checks for struct ldlm_request */ - LASSERTF((int)sizeof(struct ldlm_request) == 104, "found %lld\n", + LASSERTF((int)sizeof(struct ldlm_request) == 88, "found %lld\n", (long long)(int)sizeof(struct ldlm_request)); LASSERTF((int)offsetof(struct ldlm_request, lock_flags) == 0, "found %lld\n", (long long)(int)offsetof(struct ldlm_request, lock_flags)); @@ -3888,8 +3888,9 @@ void lustre_assert_wire_constants(void) (long long)(int)sizeof(((struct ldlm_request *)0)->lock_desc)); LASSERTF((int)offsetof(struct ldlm_request, lock_handle) == 88, "found %lld\n", (long long)(int)offsetof(struct ldlm_request, lock_handle)); - LASSERTF((int)sizeof(((struct ldlm_request *)0)->lock_handle) == 16, "found %lld\n", - (long long)(int)sizeof(((struct ldlm_request *)0)->lock_handle)); + LASSERTF((int)sizeof(*((struct ldlm_request *)0)->lock_handle) == 8, "found %lld\n", + (long long)(int)sizeof(*((struct ldlm_request *)0)->lock_handle)); + BUILD_BUG_ON(offsetof(struct ldlm_request, lock_handle) != sizeof(struct ldlm_request)); /* Checks for struct ldlm_reply */ LASSERTF((int)sizeof(struct ldlm_reply) == 112, "found %lld\n", -- 1.8.3.1