On Tue, Feb 08, 2022 at 03:16:45PM -0600, Bob Pearson wrote:
> /* check to see if the qp is already a member of the group */
> - spin_lock_bh(&rxe->mcg_lock);
> - list_for_each_entry(mca, &mcg->qp_list, qp_list) {
> + rcu_read_lock();
> + list_for_each_entry_rcu(mca, &mcg->qp_list, qp_list) {
> if (mca->qp == qp) {
The use of mca->qp protected by RCU isn't Ok..
Look at the free path:
> +static void __rxe_cleanup_mca_rcu(struct rxe_mca *mca, struct rxe_mcg *mcg)
> {
> + list_del_rcu(&mca->qp_list);
>
> atomic_dec(&mcg->qp_num);
> atomic_dec(&mcg->rxe->mcg_attach);
> atomic_dec(&mca->qp->mcg_num);
> rxe_drop_ref(mca->qp);
>
> + kfree_rcu(mca);
So the mca deref won't segfault but mca->qp is garbage now since it
might have been freed and reallocated due to the rxe_drop_ref()
It looks easy enough to fix, just use a call_rcu to free the thing
instead of kfree_rcu and do the rxe_drop_ref, and maybe others, inside
the call_rcu function.
> diff --git a/drivers/infiniband/sw/rxe/rxe_recv.c b/drivers/infiniband/sw/rxe/rxe_recv.c
> index 9b21cbb22602..c2cab85c6576 100644
> +++ b/drivers/infiniband/sw/rxe/rxe_recv.c
> @@ -265,15 +265,15 @@ static void rxe_rcv_mcast_pkt(struct rxe_dev *rxe, struct sk_buff *skb)
> qp_array = kmalloc_array(nmax, sizeof(qp), GFP_KERNEL);
> n = 0;
>
> - spin_lock_bh(&rxe->mcg_lock);
> - list_for_each_entry(mca, &mcg->qp_list, qp_list) {
> + rcu_read_lock();
> + list_for_each_entry_rcu(mca, &mcg->qp_list, qp_list) {
> /* protect the qp pointers in the list */
> rxe_add_ref(mca->qp);
And this one could use after free qp
If the mca->qp is guarenteed to have a valid refcount as above then it
is OK.
Jason
