* [PATCH rdma-next 1/4] IB/mad: Fix use after free when destroying MAD agent
2020-06-21 10:47 [PATCH rdma-next 0/4] Bag of fixes and refactoring in MAD layer Leon Romanovsky
@ 2020-06-21 10:47 ` Leon Romanovsky
2020-06-22 18:00 ` Jason Gunthorpe
2020-06-21 10:47 ` [PATCH rdma-next 2/4] IB/mad: Issue complete whenever decrements agent refcount Leon Romanovsky
` (3 subsequent siblings)
4 siblings, 1 reply; 7+ messages in thread
From: Leon Romanovsky @ 2020-06-21 10:47 UTC (permalink / raw)
To: Doug Ledford, Jason Gunthorpe
Cc: Shay Drory, Jack Morgenstein, linux-rdma, Maor Gottlieb, willy
From: Shay Drory <shayd@mellanox.com>
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().
Fixes: 9a41e38a467c ("IB/mad: Use IDR for agent IDs")
Signed-off-by: Shay Drory <shayd@mellanox.com>
Reviewed-by: Maor Gottlieb <maorg@mellanox.com>
Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
---
drivers/infiniband/core/mad.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/infiniband/core/mad.c b/drivers/infiniband/core/mad.c
index 186e0d652e8b..2da889f9b6b3 100644
--- a/drivers/infiniband/core/mad.c
+++ b/drivers/infiniband/core/mad.c
@@ -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);
--
2.26.2
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH rdma-next 1/4] IB/mad: Fix use after free when destroying MAD agent
2020-06-21 10:47 ` [PATCH rdma-next 1/4] IB/mad: Fix use after free when destroying MAD agent Leon Romanovsky
@ 2020-06-22 18:00 ` Jason Gunthorpe
0 siblings, 0 replies; 7+ messages in thread
From: Jason Gunthorpe @ 2020-06-22 18:00 UTC (permalink / raw)
To: Leon Romanovsky
Cc: Doug Ledford, Shay Drory, Jack Morgenstein, linux-rdma,
Maor Gottlieb, willy
On Sun, Jun 21, 2020 at 01:47:35PM +0300, Leon Romanovsky wrote:
> From: Shay Drory <shayd@mellanox.com>
>
> 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().
>
> Fixes: 9a41e38a467c ("IB/mad: Use IDR for agent IDs")
> Signed-off-by: Shay Drory <shayd@mellanox.com>
> Reviewed-by: Maor Gottlieb <maorg@mellanox.com>
> Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
> ---
> drivers/infiniband/core/mad.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
Applied to for-rc thanks
Jason
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH rdma-next 2/4] IB/mad: Issue complete whenever decrements agent refcount
2020-06-21 10:47 [PATCH rdma-next 0/4] Bag of fixes and refactoring in MAD layer Leon Romanovsky
2020-06-21 10:47 ` [PATCH rdma-next 1/4] IB/mad: Fix use after free when destroying MAD agent Leon Romanovsky
@ 2020-06-21 10:47 ` Leon Romanovsky
2020-06-21 10:47 ` [PATCH rdma-next 3/4] IB/mad: Refactor atomics API to refcount API Leon Romanovsky
` (2 subsequent siblings)
4 siblings, 0 replies; 7+ messages in thread
From: Leon Romanovsky @ 2020-06-21 10:47 UTC (permalink / raw)
To: Doug Ledford, Jason Gunthorpe; +Cc: Shay Drory, linux-rdma, Maor Gottlieb
From: Shay Drory <shayd@mellanox.com>
Replace calls of atomic_dec to mad_agent_priv->refcount with calls
to deref_mad_agent in order to issue complete.
Signed-off-by: Shay Drory <shayd@mellanox.com>
Reviewed-by: Maor Gottlieb <maorg@mellanox.com>
Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
---
drivers/infiniband/core/mad.c | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/drivers/infiniband/core/mad.c b/drivers/infiniband/core/mad.c
index 2da889f9b6b3..46f36acb6a44 100644
--- a/drivers/infiniband/core/mad.c
+++ b/drivers/infiniband/core/mad.c
@@ -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
^ permalink raw reply related [flat|nested] 7+ messages in thread* [PATCH rdma-next 3/4] IB/mad: Refactor atomics API to refcount API
2020-06-21 10:47 [PATCH rdma-next 0/4] Bag of fixes and refactoring in MAD layer Leon Romanovsky
2020-06-21 10:47 ` [PATCH rdma-next 1/4] IB/mad: Fix use after free when destroying MAD agent Leon Romanovsky
2020-06-21 10:47 ` [PATCH rdma-next 2/4] IB/mad: Issue complete whenever decrements agent refcount Leon Romanovsky
@ 2020-06-21 10:47 ` Leon Romanovsky
2020-06-21 10:47 ` [PATCH rdma-next 4/4] IB/mad: Delete RMPP_STATE_CANCELING state Leon Romanovsky
2020-06-24 19:45 ` [PATCH rdma-next 0/4] Bag of fixes and refactoring in MAD layer Jason Gunthorpe
4 siblings, 0 replies; 7+ messages in thread
From: Leon Romanovsky @ 2020-06-21 10:47 UTC (permalink / raw)
To: Doug Ledford, Jason Gunthorpe; +Cc: Shay Drory, linux-rdma, Maor Gottlieb
From: Shay Drory <shayd@mellanox.com>
Refcount API provides better safety than atomics API.
Therefore, refactor atomic functions to refcount functions.
Signed-off-by: Shay Drory <shayd@mellanox.com>
Reviewed-by: Maor Gottlieb <maorg@mellanox.com>
Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
---
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(-)
diff --git a/drivers/infiniband/core/mad.c b/drivers/infiniband/core/mad.c
index 46f36acb6a44..239ec5c49721 100644
--- a/drivers/infiniband/core/mad.c
+++ b/drivers/infiniband/core/mad.c
@@ -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);
}
diff --git a/drivers/infiniband/core/mad_priv.h b/drivers/infiniband/core/mad_priv.h
index 403d8673a2f9..4aa16b35dad0 100644
--- a/drivers/infiniband/core/mad_priv.h
+++ b/drivers/infiniband/core/mad_priv.h
@@ -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;
diff --git a/drivers/infiniband/core/mad_rmpp.c b/drivers/infiniband/core/mad_rmpp.c
index 5ec57abc0849..1bc9dfecae70 100644
--- a/drivers/infiniband/core/mad_rmpp.c
+++ b/drivers/infiniband/core/mad_rmpp.c
@@ -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;
--
2.26.2
^ permalink raw reply related [flat|nested] 7+ messages in thread* [PATCH rdma-next 4/4] IB/mad: Delete RMPP_STATE_CANCELING state
2020-06-21 10:47 [PATCH rdma-next 0/4] Bag of fixes and refactoring in MAD layer Leon Romanovsky
` (2 preceding siblings ...)
2020-06-21 10:47 ` [PATCH rdma-next 3/4] IB/mad: Refactor atomics API to refcount API Leon Romanovsky
@ 2020-06-21 10:47 ` Leon Romanovsky
2020-06-24 19:45 ` [PATCH rdma-next 0/4] Bag of fixes and refactoring in MAD layer Jason Gunthorpe
4 siblings, 0 replies; 7+ messages in thread
From: Leon Romanovsky @ 2020-06-21 10:47 UTC (permalink / raw)
To: Doug Ledford, Jason Gunthorpe; +Cc: Shay Drory, linux-rdma
From: Shay Drory <shayd@mellanox.com>
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.
Signed-off-by: Shay Drory <shayd@mellanox.com>
Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
---
drivers/infiniband/core/mad_rmpp.c | 17 ++++-------------
1 file changed, 4 insertions(+), 13 deletions(-)
diff --git a/drivers/infiniband/core/mad_rmpp.c b/drivers/infiniband/core/mad_rmpp.c
index 1bc9dfecae70..e0573e4d0404 100644
--- a/drivers/infiniband/core/mad_rmpp.c
+++ b/drivers/infiniband/core/mad_rmpp.c
@@ -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);
--
2.26.2
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH rdma-next 0/4] Bag of fixes and refactoring in MAD layer
2020-06-21 10:47 [PATCH rdma-next 0/4] Bag of fixes and refactoring in MAD layer Leon Romanovsky
` (3 preceding siblings ...)
2020-06-21 10:47 ` [PATCH rdma-next 4/4] IB/mad: Delete RMPP_STATE_CANCELING state Leon Romanovsky
@ 2020-06-24 19:45 ` Jason Gunthorpe
4 siblings, 0 replies; 7+ messages in thread
From: Jason Gunthorpe @ 2020-06-24 19:45 UTC (permalink / raw)
To: Leon Romanovsky
Cc: Doug Ledford, Leon Romanovsky, Jack Morgenstein, linux-kernel,
linux-rdma, Maor Gottlieb, Shay Drory, willy
On Sun, Jun 21, 2020 at 01:47:34PM +0300, Leon Romanovsky wrote:
> From: Leon Romanovsky <leonro@mellanox.com>
>
> 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: Issue complete whenever decrements agent refcount
> IB/mad: Refactor atomics API to refcount API
> IB/mad: Delete RMPP_STATE_CANCELING state
Applied to for-next
Thanks,
Jason
^ permalink raw reply [flat|nested] 7+ messages in thread