| Message ID | a99e79aa8515e4b52ced83447122fbd260104f0f.1586275373.git.ka-cheong.poon@oracle.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net,1/2] net/rds: Replace direct refcount_inc() by inline function | expand |
On 4/7/20 9:08 AM, Ka-Cheong Poon wrote: > In rds_free_mr(), it calls rds_destroy_mr(mr) directly. But this > defeats the purpose of reference counting and makes MR free handling > impossible. It means that holding a reference does not guarantee that > it is safe to access some fields. For example, In > rds_cmsg_rdma_dest(), it increases the ref count, unlocks and then > calls mr->r_trans->sync_mr(). But if rds_free_mr() (and > rds_destroy_mr()) is called in between (there is no lock preventing > this to happen), r_trans_private is set to NULL, causing a panic. > Similar issue is in rds_rdma_unuse(). > > Reported-by: zerons <sironhide0null@gmail.com> > Signed-off-by: Ka-Cheong Poon <ka-cheong.poon@oracle.com> > --- Thanks for getting this out on the list. Hi zerons, Can you please review it and see it addresses your concern ? Regards, Santosh
On 4/8/20 03:30, santosh.shilimkar@oracle.com wrote: > On 4/7/20 9:08 AM, Ka-Cheong Poon wrote: >> In rds_free_mr(), it calls rds_destroy_mr(mr) directly. But this >> defeats the purpose of reference counting and makes MR free handling >> impossible. It means that holding a reference does not guarantee that >> it is safe to access some fields. For example, In >> rds_cmsg_rdma_dest(), it increases the ref count, unlocks and then >> calls mr->r_trans->sync_mr(). But if rds_free_mr() (and >> rds_destroy_mr()) is called in between (there is no lock preventing >> this to happen), r_trans_private is set to NULL, causing a panic. >> Similar issue is in rds_rdma_unuse(). >> >> Reported-by: zerons <sironhide0null@gmail.com> >> Signed-off-by: Ka-Cheong Poon <ka-cheong.poon@oracle.com> >> --- > Thanks for getting this out on the list. > > Hi zerons, > Can you please review it and see it addresses your concern ? > Yes, the MR gets freed only when the ref count decreases to zero does address my concern. I think it make the logic cleaner as well. Fantastic! Regards, zerons
On 4/7/20 7:25 PM, zerons wrote: > On 4/8/20 03:30, santosh.shilimkar@oracle.com wrote: >> On 4/7/20 9:08 AM, Ka-Cheong Poon wrote: >>> In rds_free_mr(), it calls rds_destroy_mr(mr) directly. But this >>> defeats the purpose of reference counting and makes MR free handling >>> impossible. It means that holding a reference does not guarantee that >>> it is safe to access some fields. For example, In >>> rds_cmsg_rdma_dest(), it increases the ref count, unlocks and then >>> calls mr->r_trans->sync_mr(). But if rds_free_mr() (and >>> rds_destroy_mr()) is called in between (there is no lock preventing >>> this to happen), r_trans_private is set to NULL, causing a panic. >>> Similar issue is in rds_rdma_unuse(). >>> >>> Reported-by: zerons <sironhide0null@gmail.com> >>> Signed-off-by: Ka-Cheong Poon <ka-cheong.poon@oracle.com> >>> --- >> Thanks for getting this out on the list. >> >> Hi zerons, >> Can you please review it and see it addresses your concern ? >> > > Yes, the MR gets freed only when the ref count decreases to zero does > address my concern. I think it make the logic cleaner as well. Fantastic! > Thanks for confirmation. FWIW, Acked-by: Santosh Shilimkar <santosh.shilimkar@oracle.com>
diff --git a/net/rds/rdma.c b/net/rds/rdma.c index d5abe0e..4cb5485 100644 --- a/net/rds/rdma.c +++ b/net/rds/rdma.c @@ -101,9 +101,6 @@ static void rds_destroy_mr(struct rds_mr *mr) rdsdebug("RDS: destroy mr key is %x refcnt %u\n", mr->r_key, refcount_read(&mr->r_refcount)); - if (test_and_set_bit(RDS_MR_DEAD, &mr->r_state)) - return; - spin_lock_irqsave(&rs->rs_rdma_lock, flags); if (!RB_EMPTY_NODE(&mr->r_rb_node)) rb_erase(&mr->r_rb_node, &rs->rs_rdma_keys); @@ -140,7 +137,6 @@ void rds_rdma_drop_keys(struct rds_sock *rs) rb_erase(&mr->r_rb_node, &rs->rs_rdma_keys); RB_CLEAR_NODE(&mr->r_rb_node); spin_unlock_irqrestore(&rs->rs_rdma_lock, flags); - rds_destroy_mr(mr); rds_mr_put(mr); spin_lock_irqsave(&rs->rs_rdma_lock, flags); } @@ -434,12 +430,6 @@ int rds_free_mr(struct rds_sock *rs, char __user *optval, int optlen) if (!mr) return -EINVAL; - /* - * call rds_destroy_mr() ourselves so that we're sure it's done by the time - * we return. If we let rds_mr_put() do it it might not happen until - * someone else drops their ref. - */ - rds_destroy_mr(mr); rds_mr_put(mr); return 0; } @@ -464,6 +454,14 @@ void rds_rdma_unuse(struct rds_sock *rs, u32 r_key, int force) return; } + /* Get a reference so that the MR won't go away before calling + * sync_mr() below. + */ + rds_mr_get(mr); + + /* If it is going to be freed, remove it from the tree now so + * that no other thread can find it and free it. + */ if (mr->r_use_once || force) { rb_erase(&mr->r_rb_node, &rs->rs_rdma_keys); RB_CLEAR_NODE(&mr->r_rb_node); @@ -477,12 +475,13 @@ void rds_rdma_unuse(struct rds_sock *rs, u32 r_key, int force) if (mr->r_trans->sync_mr) mr->r_trans->sync_mr(mr->r_trans_private, DMA_FROM_DEVICE); + /* Release the reference held above. */ + rds_mr_put(mr); + /* If the MR was marked as invalidate, this will * trigger an async flush. */ - if (zot_me) { - rds_destroy_mr(mr); + if (zot_me) rds_mr_put(mr); - } } void rds_rdma_free_op(struct rm_rdma_op *ro) diff --git a/net/rds/rds.h b/net/rds/rds.h index 6a665fa..e889ef8 100644 --- a/net/rds/rds.h +++ b/net/rds/rds.h @@ -299,19 +299,11 @@ struct rds_mr { unsigned int r_invalidate:1; unsigned int r_write:1; - /* This is for RDS_MR_DEAD. - * It would be nice & consistent to make this part of the above - * bit field here, but we need to use test_and_set_bit. - */ - unsigned long r_state; struct rds_sock *r_sock; /* back pointer to the socket that owns us */ struct rds_transport *r_trans; void *r_trans_private; }; -/* Flags for mr->r_state */ -#define RDS_MR_DEAD 0 - static inline rds_rdma_cookie_t rds_rdma_make_cookie(u32 r_key, u32 offset) { return r_key | (((u64) offset) << 32);
In rds_free_mr(), it calls rds_destroy_mr(mr) directly. But this defeats the purpose of reference counting and makes MR free handling impossible. It means that holding a reference does not guarantee that it is safe to access some fields. For example, In rds_cmsg_rdma_dest(), it increases the ref count, unlocks and then calls mr->r_trans->sync_mr(). But if rds_free_mr() (and rds_destroy_mr()) is called in between (there is no lock preventing this to happen), r_trans_private is set to NULL, causing a panic. Similar issue is in rds_rdma_unuse(). Reported-by: zerons <sironhide0null@gmail.com> Signed-off-by: Ka-Cheong Poon <ka-cheong.poon@oracle.com> --- net/rds/rdma.c | 25 ++++++++++++------------- net/rds/rds.h | 8 -------- 2 files changed, 12 insertions(+), 21 deletions(-)