Bag of fixes and refactoring in MAD layer
cover Bag of fixes and refactoring in MAD layer
From: Leon Romanovsky <[email protected]>
Hi,
This is combination of fixes and refactoring in MAD layer. Because everything here is "old", I'm sending it to rdma-next and not "wasting" our time in attempt to separate fix patches from refactoring ones.
Thanks
Shay Drory (4):
IB/mad: Fix use after free when destroying MAD agent IB/mad: Issue complete whenever decrements agent refcount IB/mad: Refactor atomics API to refcount API IB/mad: Delete RMPP_STATE_CANCELING state
drivers/infiniband/core/mad.c | 32 +++++++++++++++--------------- drivers/infiniband/core/mad_priv.h | 2 +- drivers/infiniband/core/mad_rmpp.c | 27 +++++++++---------------- 3 files changed, 26 insertions(+), 35 deletions(-)
--
2.26.2
PATCH 1/4 IB/mad: Fix use after free when destroying MAD agent
From: Shay Drory <[email protected]>
Currently, when RMPP MADs are processed while the MAD agent is destroyed, it could result in use after free of rmpp_recv, as decribed below:
cpu-0 cpu-1 ----- -----
ib_mad_recv_done()
ib_mad_complete_recv()
ib_process_rmpp_recv_wc()
unregister_mad_agent()
ib_cancel_rmpp_recvs()
cancel_delayed_work()
process_rmpp_data()
start_rmpp()
queue_delayed_work(rmpp_recv->cleanup_work)
destroy_rmpp_recv()
free_rmpp_recv()
cleanup_work()[1]
spin_lock_irqsave(&rmpp_recv->agent->lock)->use after free[1] cleanup_work() == recv_cleanup_handler
Fix it by waiting for the MAD agent reference count becoming zero before calling to ib_cancel_rmpp_recvs().
--- drivers/infiniband/core/mad.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/infiniband/core/mad.c +1 -1
@@ -509,10 +509,10 @@ static void unregister_mad_agent(struct ib_mad_agent_private *mad_agent_priv)xa_erase (&ib_mad_clients, mad_agent_priv->agent.hi_tid);flush_workqueue (port_priv->wq); -ib_cancel_rmpp_recvs (mad_agent_priv);deref_mad_agent (mad_agent_priv);wait_for_completion (&mad_agent_priv->comp); +ib_cancel_rmpp_recvs (mad_agent_priv);ib_mad_agent_security_cleanup (&mad_agent_priv->agent);
Signature · 1 line
2.26.2
Re: [PATCH rdma-next 1/4] IB/mad: Fix use after free when destroying MAD agent
Quoted 27 lines · Leon Romanovsky
From: Shay Drory <[email protected]>
Currently, when RMPP MADs are processed while the MAD agent is destroyed, it could result in use after free of
rmpp_recv, as decribed below:cpu-0 cpu-1 ----- -----
ib_mad_recv_done()
ib_mad_complete_recv()
ib_process_rmpp_recv_wc()
unregister_mad_agent()
ib_cancel_rmpp_recvs()
cancel_delayed_work()
process_rmpp_data()start_rmpp() queue_delayed_work(rmpp_recv->cleanup_work) destroy_rmpp_recv() free_rmpp_recv() cleanup_work()[1] spin_lock_irqsave(&rmpp_recv->agent->lock)->use after free[1]
cleanup_work()==recv_cleanup_handlerFix it by waiting for the MAD agent reference count becoming zero before calling to
ib_cancel_rmpp_recvs().
Quoted 7 lines
Fixes 9a41e38a467c ("IB/mad: Use IDR for agent IDs")Signed-off-by Shay Drory <[email protected]>Reviewed-by Maor Gottlieb <[email protected]>Signed-off-by Leon Romanovsky <[email protected]>--- drivers/infiniband/core/mad.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Applied to for-rc thanks
Jason
PATCH 2/4 IB/mad: Issue complete whenever decrements agent refcount
In reply to Leon RomanovskyFrom: Shay Drory <[email protected]>
Replace calls of atomic_dec to mad_agent_priv->refcount with calls to deref_mad_agent in order to issue complete.
--- drivers/infiniband/core/mad.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-)
drivers/infiniband/core/mad.c +7 -7
@@ -1148,7 +1148,7 @@ int ib_post_send_mad(struct ib_mad_send_buf *send_buf,spin_lock_irqsave (&mad_agent_priv->lock, flags);list_del (&mad_send_wr->agent_list);spin_unlock_irqrestore (&mad_agent_priv->lock, flags); -atomic_dec (&mad_agent_priv->refcount); +deref_mad_agent (mad_agent_priv); goto error; } }
@@ -1831,7 +1831,7 @@ static void ib_mad_complete_recv(struct ib_mad_agent_private *mad_agent_priv, mad_agent_priv->agent.recv_handler ( &mad_agent_priv->agent,NULL , mad_recv_wc); -atomic_dec (&mad_agent_priv->refcount); +deref_mad_agent (mad_agent_priv); }else {/* not user rmpp, revert to normal behavior and * drop the mad */
@@ -1848,7 +1848,7 @@ static void ib_mad_complete_recv(struct ib_mad_agent_private *mad_agent_priv, &mad_agent_priv->agent, &mad_send_wr->send_buf, mad_recv_wc); -atomic_dec (&mad_agent_priv->refcount); +deref_mad_agent (mad_agent_priv); mad_send_wc.status =IB_WC_SUCCESS ; mad_send_wc.vendor_err =0 ;
@@ -2438,7 +2438,7 @@ static void cancel_mads(struct ib_mad_agent_private *mad_agent_priv)list_del (&mad_send_wr->agent_list); mad_agent_priv->agent.send_handler (&mad_agent_priv->agent, &mad_send_wc); -atomic_dec (&mad_agent_priv->refcount); +deref_mad_agent (mad_agent_priv); } }
@@ -2572,7 +2572,7 @@ static void local_completions(struct work_struct *work) &local->mad_send_wr->send_buf, &local->mad_priv->header.recv_wc);spin_lock_irqsave (&recv_mad_agent->lock, flags); -atomic_dec (&recv_mad_agent->refcount); +deref_mad_agent (recv_mad_agent);spin_unlock_irqrestore (&recv_mad_agent->lock, flags); }
@@ -2585,7 +2585,7 @@ static void local_completions(struct work_struct *work) &mad_send_wc);spin_lock_irqsave (&mad_agent_priv->lock, flags); -atomic_dec (&mad_agent_priv->refcount); +deref_mad_agent (mad_agent_priv);if (free_mad)kfree (local->mad_priv);kfree (local);
@@ -2671,7 +2671,7 @@ static void timeout_sends(struct work_struct *work) mad_agent_priv->agent.send_handler (&mad_agent_priv->agent, &mad_send_wc); -atomic_dec (&mad_agent_priv->refcount); +deref_mad_agent (mad_agent_priv);spin_lock_irqsave (&mad_agent_priv->lock, flags); }spin_unlock_irqrestore (&mad_agent_priv->lock, flags);
--
2.26.2
PATCH 3/4 IB/mad: Refactor atomics API to refcount API
In reply to Leon RomanovskyFrom: Shay Drory <[email protected]>
Refcount API provides better safety than atomics API. Therefore, refactor atomic functions to refcount functions.
--- drivers/infiniband/core/mad.c | 16 ++++++++-------- drivers/infiniband/core/mad_priv.h | 2 +- drivers/infiniband/core/mad_rmpp.c | 10 +++++----- 3 files changed, 14 insertions(+), 14 deletions(-)
drivers/infiniband/core/mad.c +8 -8
@@ -402,7 +402,7 @@ struct ib_mad_agent *ib_register_mad_agent(struct ib_device *device,INIT_DELAYED_WORK (&mad_agent_priv->timed_work, timeout_sends);INIT_LIST_HEAD (&mad_agent_priv->local_list);INIT_WORK (&mad_agent_priv->local_work, local_completions); -atomic_set (&mad_agent_priv->refcount,1 ); +refcount_set (&mad_agent_priv->refcount,1 );init_completion (&mad_agent_priv->comp); ret2 =ib_mad_agent_security_setup (&mad_agent_priv->agent, qp_type);
@@ -484,7 +484,7 @@ EXPORT_SYMBOL(ib_register_mad_agent);static inline void deref_mad_agent (struct ib_mad_agent_private *mad_agent_priv) { -if (atomic_dec_and_test (&mad_agent_priv->refcount)) +if (refcount_dec_and_test (&mad_agent_priv->refcount))complete (&mad_agent_priv->comp); }
@@ -718,7 +718,7 @@ static int handle_outgoing_dr_smp(struct ib_mad_agent_private *mad_agent_priv,* Reference MAD agent until receive * side of local completion handled */ -atomic_inc (&mad_agent_priv->refcount); +refcount_inc (&mad_agent_priv->refcount); }else kfree (mad_priv);break ;
@@ -758,7 +758,7 @@ static int handle_outgoing_dr_smp(struct ib_mad_agent_private *mad_agent_priv, local->return_wc_byte_len = mad_size; }/* Reference MAD agent until send side of local completion handled */ -atomic_inc (&mad_agent_priv->refcount); +refcount_inc (&mad_agent_priv->refcount);/* Queue local completion to local list */ spin_lock_irqsave (&mad_agent_priv->lock, flags);list_add_tail (&local->completion_list, &mad_agent_priv->local_list);
@@ -916,7 +916,7 @@ struct ib_mad_send_buf * ib_create_send_mad(struct ib_mad_agent *mad_agent, } mad_send_wr->send_buf.mad_agent = mad_agent; -atomic_inc (&mad_agent_priv->refcount); +refcount_inc (&mad_agent_priv->refcount);return &mad_send_wr->send_buf; }EXPORT_SYMBOL (ib_create_send_mad);
@@ -1131,7 +1131,7 @@ int ib_post_send_mad(struct ib_mad_send_buf *send_buf, mad_send_wr->status =IB_WC_SUCCESS ;/* Reference MAD agent until send completes */ -atomic_inc (&mad_agent_priv->refcount); +refcount_inc (&mad_agent_priv->refcount);spin_lock_irqsave (&mad_agent_priv->lock, flags);list_add_tail (&mad_send_wr->agent_list, &mad_agent_priv->send_list);
@@ -1554,7 +1554,7 @@ find_mad_agent(struct ib_mad_port_private *port_priv, hi_tid =be64_to_cpu (mad_hdr->tid) >>32 ;rcu_read_lock (); mad_agent =xa_load (&ib_mad_clients, hi_tid); -if (mad_agent && !atomic_inc_not_zero (&mad_agent->refcount)) +if (mad_agent && !refcount_inc_not_zero (&mad_agent->refcount)) mad_agent =NULL ;rcu_read_unlock (); }else {
@@ -1606,7 +1606,7 @@ find_mad_agent(struct ib_mad_port_private *port_priv, } }if (mad_agent) -atomic_inc (&mad_agent->refcount); +refcount_inc (&mad_agent->refcount); out:spin_unlock_irqrestore (&port_priv->reg_lock, flags); }
drivers/infiniband/core/mad_priv.h +1 -1
@@ -103,7 +103,7 @@ struct ib_mad_agent_private {struct work_struct local_work;struct list_head rmpp_list; -atomic_t refcount; +refcount_t refcount;union {struct completion comp;struct rcu_head rcu;
drivers/infiniband/core/mad_rmpp.c +5 -5
@@ -52,7 +52,7 @@ struct mad_rmpp_recv {struct completion comp;enum rmpp_state state;spinlock_t lock; -atomic_t refcount; +refcount_t refcount;struct ib_ah *ah;struct ib_mad_recv_wc *rmpp_wc;
@@ -73,7 +73,7 @@ struct mad_rmpp_recv {static inline void deref_rmpp_recv (struct mad_rmpp_recv *rmpp_recv) { -if (atomic_dec_and_test (&rmpp_recv->refcount)) +if (refcount_dec_and_test (&rmpp_recv->refcount))complete (&rmpp_recv->comp); }
@@ -305,7 +305,7 @@ create_rmpp_recv(struct ib_mad_agent_private *agent,INIT_DELAYED_WORK (&rmpp_recv->cleanup_work, recv_cleanup_handler);spin_lock_init (&rmpp_recv->lock); rmpp_recv->state =RMPP_STATE_ACTIVE ; -atomic_set (&rmpp_recv->refcount,1 ); +refcount_set (&rmpp_recv->refcount,1 ); rmpp_recv->rmpp_wc = mad_recv_wc; rmpp_recv->cur_seg_buf = &mad_recv_wc->recv_buf;
@@ -357,7 +357,7 @@ acquire_rmpp_recv(struct ib_mad_agent_private *agent,spin_lock_irqsave (&agent->lock, flags); rmpp_recv =find_rmpp_recv (agent, mad_recv_wc);if (rmpp_recv) -atomic_inc (&rmpp_recv->refcount); +refcount_inc (&rmpp_recv->refcount);spin_unlock_irqrestore (&agent->lock, flags);return rmpp_recv; }
@@ -553,7 +553,7 @@ start_rmpp(struct ib_mad_agent_private *agent,destroy_rmpp_recv (rmpp_recv);return continue_rmpp (agent, mad_recv_wc); } -atomic_inc (&rmpp_recv->refcount); +refcount_inc (&rmpp_recv->refcount);if (get_last_flag (&mad_recv_wc->recv_buf)) { rmpp_recv->state =RMPP_STATE_COMPLETE ;
Signature · 1 line
2.26.2
PATCH 4/4 IB/mad: Delete RMPP_STATE_CANCELING state
In reply to Leon RomanovskyFrom: Shay Drory <[email protected]>
The cancel_delayed_work can be called under lock since it doesn't sleep. This makes the RMPP_STATE_CANCELING state is not needed anymore.
--- drivers/infiniband/core/mad_rmpp.c | 17 ++++------------- 1 file changed, 4 insertions(+), 13 deletions(-)
drivers/infiniband/core/mad_rmpp.c +4 -13
@@ -40,8 +40,7 @@enum rmpp_state {RMPP_STATE_ACTIVE ,RMPP_STATE_TIMEOUT , -RMPP_STATE_COMPLETE , -RMPP_STATE_CANCELING +RMPP_STATE_COMPLETE };struct mad_rmpp_recv {
@@ -91,23 +90,19 @@ void ib_cancel_rmpp_recvs(struct ib_mad_agent_private *agent)unsigned long flags;spin_lock_irqsave (&agent->lock, flags); -list_for_each_entry (rmpp_recv, &agent->rmpp_list, list) { -if (rmpp_recv->state !=RMPP_STATE_COMPLETE ) -ib_free_recv_mad (rmpp_recv->rmpp_wc); - rmpp_recv->state =RMPP_STATE_CANCELING ; - } -spin_unlock_irqrestore (&agent->lock, flags); -list_for_each_entry (rmpp_recv, &agent->rmpp_list, list) {cancel_delayed_work (&rmpp_recv->timeout_work);cancel_delayed_work (&rmpp_recv->cleanup_work); } +spin_unlock_irqrestore (&agent->lock, flags);flush_workqueue (agent->qp_info->port_priv->wq);list_for_each_entry_safe (rmpp_recv, temp_rmpp_recv, &agent->rmpp_list, list) {list_del (&rmpp_recv->list); +if (rmpp_recv->state !=RMPP_STATE_COMPLETE ) +ib_free_recv_mad (rmpp_recv->rmpp_wc);destroy_rmpp_recv (rmpp_recv); } }
@@ -272,10 +267,6 @@ static void recv_cleanup_handler(struct work_struct *work)unsigned long flags;spin_lock_irqsave (&rmpp_recv->agent->lock, flags); -if (rmpp_recv->state ==RMPP_STATE_CANCELING ) { -spin_unlock_irqrestore (&rmpp_recv->agent->lock, flags); -return ; - }list_del (&rmpp_recv->list);spin_unlock_irqrestore (&rmpp_recv->agent->lock, flags);destroy_rmpp_recv (rmpp_recv);
Signature · 1 line
2.26.2
Quoted 8 lines · Leon Romanovsky
From: Leon Romanovsky <[email protected]>
Hi,
This is combination of fixes and refactoring in MAD layer. Because everything here is "old", I'm sending it to rdma-next and not "wasting" our time in attempt to separate fix patches from refactoring ones.
Quoted 6 lines
Thanks
Shay Drory (4):
IB/mad: Issue complete whenever decrements agent refcount IB/mad: Refactor atomics API to refcount API IB/mad: DeleteRMPP_STATE_CANCELINGstate
Applied to for-next
Thanks,
Jason