From d7e70e38c942baebfdc186fcd5ad48cdcc2ebde3 Mon Sep 17 00:00:00 2001 From: Etienne AUJAMES Date: Wed, 6 Sep 2023 14:24:47 +0200 Subject: [PATCH] LU-17076 orr: take a ref after lookup_get_insert_fast() We need to take a reference on all existing object returned by nrs_orr_res_get(). Otherwise, the object could be freed before nrs_orr_res_put(). Therefore, a ref should be taken if rhashtable_lookup_get_insert_fast() returns an existing object. This avoids the following use-after-free in sanityn test_77c: ptlrpc_nrs_req_stop_nolock+0x3b/0x150 ptlrpc_server_finish_active_request+0x2b/0x140 [ptlrpc] ptlrpc_server_handle_request+0x40e/0xc00 [ptlrpc] ptlrpc_main+0xc8c/0x1680 [ptlrpc] kthread+0xd1/0xe0 ret_from_fork_nospec_begin+0x21/0x21 Fixes: 42bf5f7 ("LU-8130 nrs: convert NRS ORR/TRR to rhashtable") Test-Parameters: fstype=zfs testlist=sanityn env=ONLY=77c,ONLY_REPEAT=100 Test-Parameters: fstype=zfs testlist=sanityn env=ONLY=77c,ONLY_REPEAT=100 Test-Parameters: fstype=zfs testlist=sanityn env=ONLY=77c,ONLY_REPEAT=100 Signed-off-by: Etienne AUJAMES Change-Id: I267287a1075ee91019d3a4492b57f272a4b0cadd Reviewed-on: https://review.whamcloud.com/c/fs/lustre-release/+/52295 Tested-by: jenkins Tested-by: Maloo Reviewed-by: James Simmons Reviewed-by: Alexander Boyko Reviewed-by: Oleg Drokin --- lustre/ptlrpc/nrs_orr.c | 59 +++++++++++++++++++++++++++---------------------- 1 file changed, 32 insertions(+), 27 deletions(-) diff --git a/lustre/ptlrpc/nrs_orr.c b/lustre/ptlrpc/nrs_orr.c index 42e1d62..6daedc1 100644 --- a/lustre/ptlrpc/nrs_orr.c +++ b/lustre/ptlrpc/nrs_orr.c @@ -726,7 +726,7 @@ static int nrs_orr_res_get(struct ptlrpc_nrs_policy *policy, struct ptlrpc_nrs_resource **resp, bool moving_req) { struct nrs_orr_data *orrd; - struct nrs_orr_object *orro, *orro2; + struct nrs_orr_object *orro, *new_orro; struct nrs_orr_key key = { { { 0 } } }; u32 opc; int rc = 0; @@ -770,48 +770,53 @@ static int nrs_orr_res_get(struct ptlrpc_nrs_policy *policy, rcu_read_lock(); orro = rhashtable_lookup_fast(&orrd->od_obj_hash, &key, nrs_orr_hash_params); - if (orro && refcount_inc_not_zero(&orro->oo_ref)) { - rcu_read_unlock(); + if (orro && refcount_inc_not_zero(&orro->oo_ref)) goto found_orro; - } + rcu_read_unlock(); - OBD_SLAB_CPT_ALLOC_PTR_GFP(orro, orrd->od_cache, + OBD_SLAB_CPT_ALLOC_PTR_GFP(new_orro, orrd->od_cache, nrs_pol2cptab(policy), nrs_pol2cptid(policy), moving_req ? GFP_ATOMIC : GFP_NOFS); - if (orro == NULL) + if (new_orro == NULL) RETURN(-ENOMEM); - orro->oo_key = key; - refcount_set(&orro->oo_ref, 1); + new_orro->oo_key = key; + refcount_set(&new_orro->oo_ref, 1); try_again: rcu_read_lock(); - orro2 = rhashtable_lookup_get_insert_fast(&orrd->od_obj_hash, - &orro->oo_rhead, + orro = rhashtable_lookup_get_insert_fast(&orrd->od_obj_hash, + &new_orro->oo_rhead, nrs_orr_hash_params); - /* A returned non-error orro2 means it already exist */ - if (!IS_ERR_OR_NULL(orro2)) { - OBD_SLAB_FREE_PTR(orro, orrd->od_cache); - orro = orro2; + /* insertion sucessfull */ + if (likely(orro == NULL)) { + orro = new_orro; + goto found_orro; } - /* insertion failed */ - if (IS_ERR(orro2)) { - rc = PTR_ERR(orro2); + /* A returned non-error orro means it already exist */ + rc = IS_ERR(orro) ? PTR_ERR(orro) : 0; + if (!rc && refcount_inc_not_zero(&orro->oo_ref)) { + OBD_SLAB_FREE_PTR(new_orro, orrd->od_cache); + goto found_orro; + } + rcu_read_unlock(); - /* hash table could be resizing. */ - if (rc == -ENOMEM || rc == -EBUSY) { - rcu_read_unlock(); - mdelay(20); - goto try_again; - } - OBD_SLAB_FREE_PTR(orro, orrd->od_cache); - rcu_read_unlock(); + /* oo_ref == 0, orro will be freed */ + if (!rc) + goto try_again; - RETURN(rc); + /* hash table could be resizing. */ + if (rc == -ENOMEM || rc == -EBUSY) { + mdelay(20); + goto try_again; } - rcu_read_unlock(); + OBD_SLAB_FREE_PTR(new_orro, orrd->od_cache); + + RETURN(rc); + found_orro: + rcu_read_unlock(); /** * For debugging purposes */ -- 1.8.3.1