Skip to contentSkip to conversation

Review Request

PATCH Superseded Divya Indi 3 participants 13 messages Jun 8, 2020 – Jun 21, 2020 InfiniBand

v3 of 4 Superseded Latest is v4 →

lore
Share & tools
lore

Divya Indi

author
#0 lore raw

[PATCH v3] IB/sa: Resolving use-after-free in ib_nl_send_msg

Hi,

Please review the patch that follows.

v3 addresses the previously raised concerns. Changes include -

1. To resolve the race where the timer can kick in before request has been sent out, we now add the request to the list after sending out the request.

2. To handle the race where the response can come in before we got a chance to add the req to the list, sending and adding the request to request list is done under spinlock - request_lock.

3. To make sure there is no blocking op/delay while holding the spinlock, using GFP_NOWAIT for memory allocation.

Thanks Jason for providing your valuable feedback.

Let me know if you have any suggestions or concerns.

Thanks,
Divya

author
#1 lore raw

PATCH v3 1/1 IB/sa: Resolving use-after-free in ib_nl_send_msg

Commit 3ebd2fd0d011 ("IB/sa: Put netlink request into the request list before sending")' -
1. Adds the query to the request list before ib_nl_snd_msg.
2. Removes ib_nl_send_msg from within the spinlock which also makes it possible to allocate memory with GFP_KERNEL.

However, if there is a delay in sending out the request (For
eg: Delay due to low memory situation) the timer to handle request timeout might kick in before the request is sent out to ibacm via netlink. ib_nl_request_timeout may release the query causing a use after free situation while accessing the query in ib_nl_send_msg.

Call Trace for the above race:

[<ffffffffa02f43cb>] ? ib_pack+0x17b/0x240 [ib_core]

[<ffffffffa032aef1>] ib_sa_path_rec_get+0x181/0x200 [ib_sa]
[<ffffffffa0379db0>] rdma_resolve_route+0x3c0/0x8d0 [rdma_cm]
[<ffffffffa0374450>] ? cma_bind_port+0xa0/0xa0 [rdma_cm]
[<ffffffffa040f850>] ? rds_rdma_cm_event_handler_cmn+0x850/0x850
[rds_rdma]
[<ffffffffa040f22c>] rds_rdma_cm_event_handler_cmn+0x22c/0x850
[rds_rdma]
[<ffffffffa040f860>] rds_rdma_cm_event_handler+0x10/0x20 [rds_rdma]
[<ffffffffa037778e>] addr_handler+0x9e/0x140 [rdma_cm]
[<ffffffffa026cdb4>] process_req+0x134/0x190 [ib_addr]
[<ffffffff810a02f9>] process_one_work+0x169/0x4a0
[<ffffffff810a0b2b>] worker_thread+0x5b/0x560
[<ffffffff810a0ad0>] ? flush_delayed_work+0x50/0x50
[<ffffffff810a68fb>] kthread+0xcb/0xf0
[<ffffffff816ec49a>] ? __schedule+0x24a/0x810
[<ffffffff816ec49a>] ? __schedule+0x24a/0x810
[<ffffffff810a6830>] ? kthread_create_on_node+0x180/0x180
[<ffffffff816f25a7>] ret_from_fork+0x47/0x90
[<ffffffff810a6830>] ? kthread_create_on_node+0x180/0x180

....
RIP [<ffffffffa03296cd>] send_mad+0x33d/0x5d0 [ib_sa]

To resolve the above issue -
1. Add the req to the request list only after the request has been sent out.
2. To handle the race where response comes in before adding request to the request list, send(rdma_nl_multicast) and add to list while holding the spinlock - request_lock.
3. Use GFP_NOWAIT for rdma_nl_multicast since it is called while holding a spinlock. In case of memory allocation failure, request will go out to SA.

Signed-off-by Divya Indi <[email protected]>
Fixes 3ebd2fd0d011 ("IB/sa: Put netlink request into the request list

before sending")

---
 drivers/infiniband/core/sa_query.c | 34 +++++++++++++++++-----------------
 1 file changed, 17 insertions(+), 17 deletions(-)
drivers/infiniband/core/sa_query.c +17 -17
index 74e0058..042c99b 100644
@@ -836,6 +836,9 @@ static int ib_nl_send_msg(struct ib_sa_query *query, gfp_t gfp_mask)
 	void *data;
 	struct ib_sa_mad *mad;
 	int len;
+	unsigned long flags;
+	unsigned long delay;
+	int ret;
 
 	mad = query->mad_buf->mad;
 	len = ib_nl_get_path_rec_attrs_len(mad->sa_hdr.comp_mask);

@@ -860,35 +863,32 @@ static int ib_nl_send_msg(struct ib_sa_query *query, gfp_t gfp_mask)
 	/* Repair the nlmsg header length */
 	nlmsg_end(skb, nlh);
 
-	return rdma_nl_multicast(&init_net, skb, RDMA_NL_GROUP_LS, gfp_mask);
+	spin_lock_irqsave(&ib_nl_request_lock, flags);
+	ret =  rdma_nl_multicast(&init_net, skb, RDMA_NL_GROUP_LS, GFP_NOWAIT);
+	if (!ret) {
+		/* Put the request on the list.*/
+		delay = msecs_to_jiffies(sa_local_svc_timeout_ms);
+		query->timeout = delay + jiffies;
+		list_add_tail(&query->list, &ib_nl_request_list);
+		/* Start the timeout if this is the only request */
+		if (ib_nl_request_list.next == &query->list)
+			queue_delayed_work(ib_nl_wq, &ib_nl_timed_work, delay);
+	}
+	spin_unlock_irqrestore(&ib_nl_request_lock, flags);
+
+	return ret;
 }
 
 static int ib_nl_make_request(struct ib_sa_query *query, gfp_t gfp_mask)
 {
-	unsigned long flags;
-	unsigned long delay;
 	int ret;
 
 	INIT_LIST_HEAD(&query->list);
 	query->seq = (u32)atomic_inc_return(&ib_nl_sa_request_seq);
 
-	/* Put the request on the list first.*/
-	spin_lock_irqsave(&ib_nl_request_lock, flags);
-	delay = msecs_to_jiffies(sa_local_svc_timeout_ms);
-	query->timeout = delay + jiffies;
-	list_add_tail(&query->list, &ib_nl_request_list);
-	/* Start the timeout if this is the only request */
-	if (ib_nl_request_list.next == &query->list)
-		queue_delayed_work(ib_nl_wq, &ib_nl_timed_work, delay);
-	spin_unlock_irqrestore(&ib_nl_request_lock, flags);
-
 	ret = ib_nl_send_msg(query, gfp_mask);
 	if (ret) {
 		ret = -EIO;
-		/* Remove the request */
-		spin_lock_irqsave(&ib_nl_request_lock, flags);
-		list_del(&query->list);
-		spin_unlock_irqrestore(&ib_nl_request_lock, flags);
 	}
 
 	return ret;
Signature · 1 line
1.8.3.1
#2 lore raw

Re: [PATCH v3] IB/sa: Resolving use-after-free in ib_nl_send_msg

Quoted 45 lines · Divya Indi

Commit 3ebd2fd0d011 ("IB/sa: Put netlink request into the request list before sending")' -
1. Adds the query to the request list before ib_nl_snd_msg.
2. Removes ib_nl_send_msg from within the spinlock which also makes it possible to allocate memory with GFP_KERNEL.

However, if there is a delay in sending out the request (For
eg: Delay due to low memory situation) the timer to handle request timeout might kick in before the request is sent out to ibacm via netlink. ib_nl_request_timeout may release the query causing a use after free situation while accessing the query in ib_nl_send_msg.

Call Trace for the above race:

[<ffffffffa02f43cb>] ? ib_pack+0x17b/0x240 [ib_core]

[<ffffffffa032aef1>] ib_sa_path_rec_get+0x181/0x200 [ib_sa]
[<ffffffffa0379db0>] rdma_resolve_route+0x3c0/0x8d0 [rdma_cm]
[<ffffffffa0374450>] ? cma_bind_port+0xa0/0xa0 [rdma_cm]
[<ffffffffa040f850>] ? rds_rdma_cm_event_handler_cmn+0x850/0x850
[rds_rdma]
[<ffffffffa040f22c>] rds_rdma_cm_event_handler_cmn+0x22c/0x850
[rds_rdma]
[<ffffffffa040f860>] rds_rdma_cm_event_handler+0x10/0x20 [rds_rdma]
[<ffffffffa037778e>] addr_handler+0x9e/0x140 [rdma_cm]
[<ffffffffa026cdb4>] process_req+0x134/0x190 [ib_addr]
[<ffffffff810a02f9>] process_one_work+0x169/0x4a0
[<ffffffff810a0b2b>] worker_thread+0x5b/0x560
[<ffffffff810a0ad0>] ? flush_delayed_work+0x50/0x50
[<ffffffff810a68fb>] kthread+0xcb/0xf0
[<ffffffff816ec49a>] ? __schedule+0x24a/0x810
[<ffffffff816ec49a>] ? __schedule+0x24a/0x810
[<ffffffff810a6830>] ? kthread_create_on_node+0x180/0x180
[<ffffffff816f25a7>] ret_from_fork+0x47/0x90
[<ffffffff810a6830>] ? kthread_create_on_node+0x180/0x180

....
RIP [<ffffffffa03296cd>] send_mad+0x33d/0x5d0 [ib_sa]

To resolve the above issue -
1. Add the req to the request list only after the request has been sent out.
2. To handle the race where response comes in before adding request to the request list, send(rdma_nl_multicast) and add to list while holding the spinlock - request_lock.
3. Use GFP_NOWAIT for rdma_nl_multicast since it is called while holding a spinlock. In case of memory allocation failure, request will go out to SA.

Quoted 3 lines
Signed-off-by Divya Indi <[email protected]>
Fixes 3ebd2fd0d011 ("IB/sa: Put netlink request into the request list

before sending")

Author SOB should be after "Fixes" line.

Quoted 19 lines
---
 drivers/infiniband/core/sa_query.c | 34 +++++++++++++++++-----------------
 1 file changed, 17 insertions(+), 17 deletions(-)
drivers/infiniband/core/sa_query.c +3 -0
index 74e0058..042c99b 100644
@@ -836,6 +836,9 @@ static int ib_nl_send_msg(struct ib_sa_query *query, gfp_t gfp_mask)
 	void *data;
 	struct ib_sa_mad *mad;
 	int len;
+	unsigned long flags;
+	unsigned long delay;
+	int ret;
 
 	mad = query->mad_buf->mad;
 	len = ib_nl_get_path_rec_attrs_len(mad->sa_hdr.comp_mask);

drivers/infiniband/core/sa_query.c static int ib_nl_send_msg(struct ib_sa_query *query, gfp_t gfp_mask) · line 867

Quoted 6 lines
drivers/infiniband/core/sa_query.c +2 -1
@@ -860,35 +863,32 @@ static int ib_nl_send_msg(struct ib_sa_query *query, gfp_t gfp_mask)
 	/* Repair the nlmsg header length */
 	nlmsg_end(skb, nlh);
 
-	return rdma_nl_multicast(&init_net, skb, RDMA_NL_GROUP_LS, gfp_mask);
+	spin_lock_irqsave(&ib_nl_request_lock, flags);
+	ret =  rdma_nl_multicast(&init_net, skb, RDMA_NL_GROUP_LS, GFP_NOWAIT);

It is hard to be convinced that this is correct solution. The mix of gfp_flags and GFP_NOWAIT at the same time and usage of ib_nl_request_lock to protect lists and suddenly rdma_nl_multicast() too makes this code unreadable/non-maintainable.

drivers/infiniband/core/sa_query.c static int ib_nl_send_msg(struct ib_sa_query *query, gfp_t gfp_mask) · line 868

Quoted 1 line
drivers/infiniband/core/sa_query.c +1 -0
+	if (!ret) {

Please use kernel coding style.

if (ret) {
spin_unlock_irqrestore(&ib_nl_request_lock, flags); return ret;
}

....

Quoted 30 lines
drivers/infiniband/core/sa_query.c +11 -10
+		/* Put the request on the list.*/
+		delay = msecs_to_jiffies(sa_local_svc_timeout_ms);
+		query->timeout = delay + jiffies;
+		list_add_tail(&query->list, &ib_nl_request_list);
+		/* Start the timeout if this is the only request */
+		if (ib_nl_request_list.next == &query->list)
+			queue_delayed_work(ib_nl_wq, &ib_nl_timed_work, delay);
+	}
+	spin_unlock_irqrestore(&ib_nl_request_lock, flags);
+
+	return ret;
 }
 
 static int ib_nl_make_request(struct ib_sa_query *query, gfp_t gfp_mask)
 {
-	unsigned long flags;
-	unsigned long delay;
 	int ret;
 
 	INIT_LIST_HEAD(&query->list);
 	query->seq = (u32)atomic_inc_return(&ib_nl_sa_request_seq);
 
-	/* Put the request on the list first.*/
-	spin_lock_irqsave(&ib_nl_request_lock, flags);
-	delay = msecs_to_jiffies(sa_local_svc_timeout_ms);
-	query->timeout = delay + jiffies;
-	list_add_tail(&query->list, &ib_nl_request_list);
-	/* Start the timeout if this is the only request */
-	if (ib_nl_request_list.next == &query->list)
-		queue_delayed_work(ib_nl_wq, &ib_nl_timed_work, delay);

drivers/infiniband/core/sa_query.c static int ib_nl_send_msg(struct ib_sa_query *query, gfp_t gfp_mask) · line 892

Quoted 10 lines
drivers/infiniband/core/sa_query.c +0 -6
-	spin_unlock_irqrestore(&ib_nl_request_lock, flags);
-
 	ret = ib_nl_send_msg(query, gfp_mask);
 	if (ret) {
 		ret = -EIO;
-		/* Remove the request */
-		spin_lock_irqsave(&ib_nl_request_lock, flags);
-		list_del(&query->list);
-		spin_unlock_irqrestore(&ib_nl_request_lock, flags);
 	}

Brackets should be removed too.

Quoted 5 lines
drivers/infiniband/core/sa_query.c +0 -1
 	return ret;
--

1.8.3.1

author
#3 lore raw

Re: [PATCH v3] IB/sa: Resolving use-after-free in ib_nl_send_msg

Hi Leon,

Thanks for taking the time to review.

Please find my comments inline -

Quoted 46 lines · Leon Romanovsky
Quoted 45 lines · Divya Indi

Commit 3ebd2fd0d011 ("IB/sa: Put netlink request into the request list before sending")' -
1. Adds the query to the request list before ib_nl_snd_msg.
2. Removes ib_nl_send_msg from within the spinlock which also makes it possible to allocate memory with GFP_KERNEL.

However, if there is a delay in sending out the request (For
eg: Delay due to low memory situation) the timer to handle request timeout might kick in before the request is sent out to ibacm via netlink. ib_nl_request_timeout may release the query causing a use after free situation while accessing the query in ib_nl_send_msg.

Call Trace for the above race:

[<ffffffffa02f43cb>] ? ib_pack+0x17b/0x240 [ib_core]

[<ffffffffa032aef1>] ib_sa_path_rec_get+0x181/0x200 [ib_sa]
[<ffffffffa0379db0>] rdma_resolve_route+0x3c0/0x8d0 [rdma_cm]
[<ffffffffa0374450>] ? cma_bind_port+0xa0/0xa0 [rdma_cm]
[<ffffffffa040f850>] ? rds_rdma_cm_event_handler_cmn+0x850/0x850
[rds_rdma]
[<ffffffffa040f22c>] rds_rdma_cm_event_handler_cmn+0x22c/0x850
[rds_rdma]
[<ffffffffa040f860>] rds_rdma_cm_event_handler+0x10/0x20 [rds_rdma]
[<ffffffffa037778e>] addr_handler+0x9e/0x140 [rdma_cm]
[<ffffffffa026cdb4>] process_req+0x134/0x190 [ib_addr]
[<ffffffff810a02f9>] process_one_work+0x169/0x4a0
[<ffffffff810a0b2b>] worker_thread+0x5b/0x560
[<ffffffff810a0ad0>] ? flush_delayed_work+0x50/0x50
[<ffffffff810a68fb>] kthread+0xcb/0xf0
[<ffffffff816ec49a>] ? __schedule+0x24a/0x810
[<ffffffff816ec49a>] ? __schedule+0x24a/0x810
[<ffffffff810a6830>] ? kthread_create_on_node+0x180/0x180
[<ffffffff816f25a7>] ret_from_fork+0x47/0x90
[<ffffffff810a6830>] ? kthread_create_on_node+0x180/0x180

....
RIP [<ffffffffa03296cd>] send_mad+0x33d/0x5d0 [ib_sa]

To resolve the above issue -
1. Add the req to the request list only after the request has been sent out.
2. To handle the race where response comes in before adding request to the request list, send(rdma_nl_multicast) and add to list while holding the spinlock - request_lock.
3. Use GFP_NOWAIT for rdma_nl_multicast since it is called while holding a spinlock. In case of memory allocation failure, request will go out to SA.

Quoted 4 lines
Quoted 3 lines
Signed-off-by Divya Indi <[email protected]>
Fixes 3ebd2fd0d011 ("IB/sa: Put netlink request into the request list

before sending")

Author SOB should be after "Fixes" line.

My bad. Noted.

Quoted 23 lines
Quoted 23 lines
---
 drivers/infiniband/core/sa_query.c | 34 +++++++++++++++++-----------------
 1 file changed, 17 insertions(+), 17 deletions(-)
drivers/infiniband/core/sa_query.c +3 -0
index 74e0058..042c99b 100644
@@ -836,6 +836,9 @@ static int ib_nl_send_msg(struct ib_sa_query *query, gfp_t gfp_mask)
 	void *data;
 	struct ib_sa_mad *mad;
 	int len;
+	unsigned long flags;
+	unsigned long delay;
+	int ret;
 
 	mad = query->mad_buf->mad;
 	len = ib_nl_get_path_rec_attrs_len(mad->sa_hdr.comp_mask);

@@ -860,35 +863,32 @@ static int ib_nl_send_msg(struct ib_sa_query *query, gfp_t gfp_mask)
 	/* Repair the nlmsg header length */
 	nlmsg_end(skb, nlh);
Quoted 7 lines

drivers/infiniband/core/sa_query.c static int ib_nl_send_msg(struct ib_sa_query *query, gfp_t gfp_mask) · line 867

Quoted 3 lines
drivers/infiniband/core/sa_query.c +2 -1
-	return rdma_nl_multicast(&init_net, skb, RDMA_NL_GROUP_LS, gfp_mask);
+	spin_lock_irqsave(&ib_nl_request_lock, flags);
+	ret =  rdma_nl_multicast(&init_net, skb, RDMA_NL_GROUP_LS, GFP_NOWAIT);

It is hard to be convinced that this is correct solution. The mix of gfp_flags and GFP_NOWAIT at the same time and usage of ib_nl_request_lock to protect lists and suddenly rdma_nl_multicast() too makes this code unreadable/non-maintainable.

Prior to 3ebd2fd0d011 ("IB/sa: Put netlink request into the request list before sending"), we had ib_nl_send_msg under the spinlock ib_nl_request_lock.

ie we had -

1. Get spinlock - ib_nl_request_lock
2. ib_nl_send_msg
2.a) rdma_nl_multicast
3. Add request to the req list
4. Arm the timer if needed.
5. Release spinlock

However, ib_nl_send_msg involved a memory allocation using GFP_KERNEL. hence, was moved out of the spinlock. In addition, req was now being added prior to ib_nl_send_msg [To handle the race where response can come in before we get a chance to add the request back to the list].

This introduced another race resulting in use-after-free.[Described in the commit.]

To resolve this, sending out the request and adding it to list need to happen while holding the request_lock.
To ensure minimum allocations while holding the lock, instead of having the entire ib_nl_send_msg under the lock, we only have rdma_nl_multicast under this spinlock.

However, do you think it would be a good idea to split ib_nl_send_msg into 2 functions -
1. Prepare the req/query [Outside the spinlock]
2. Sending the req - rdma_nl_multicast [while holding spinlock]

Would this be more intuitive?

Quoted 9 lines

drivers/infiniband/core/sa_query.c static int ib_nl_send_msg(struct ib_sa_query *query, gfp_t gfp_mask) · line 868

Quoted 1 line
drivers/infiniband/core/sa_query.c +1 -0
+	if (!ret) {

Please use kernel coding style.

if (ret) {
spin_unlock_irqrestore(&ib_nl_request_lock, flags); return ret;
}

....

Noted. Will make this change.

Quoted 34 lines
Quoted 34 lines
drivers/infiniband/core/sa_query.c +11 -12
+		/* Put the request on the list.*/
+		delay = msecs_to_jiffies(sa_local_svc_timeout_ms);
+		query->timeout = delay + jiffies;
+		list_add_tail(&query->list, &ib_nl_request_list);
+		/* Start the timeout if this is the only request */
+		if (ib_nl_request_list.next == &query->list)
+			queue_delayed_work(ib_nl_wq, &ib_nl_timed_work, delay);
+	}
+	spin_unlock_irqrestore(&ib_nl_request_lock, flags);
+
+	return ret;
 }
 
 static int ib_nl_make_request(struct ib_sa_query *query, gfp_t gfp_mask)
 {
-	unsigned long flags;
-	unsigned long delay;
 	int ret;
 
 	INIT_LIST_HEAD(&query->list);
 	query->seq = (u32)atomic_inc_return(&ib_nl_sa_request_seq);
 
-	/* Put the request on the list first.*/
-	spin_lock_irqsave(&ib_nl_request_lock, flags);
-	delay = msecs_to_jiffies(sa_local_svc_timeout_ms);
-	query->timeout = delay + jiffies;
-	list_add_tail(&query->list, &ib_nl_request_list);
-	/* Start the timeout if this is the only request */
-	if (ib_nl_request_list.next == &query->list)
-		queue_delayed_work(ib_nl_wq, &ib_nl_timed_work, delay);
-	spin_unlock_irqrestore(&ib_nl_request_lock, flags);
-
 	ret = ib_nl_send_msg(query, gfp_mask);
Quoted 8 lines

drivers/infiniband/core/sa_query.c static int ib_nl_send_msg(struct ib_sa_query *query, gfp_t gfp_mask) · line 892

Quoted 7 lines
drivers/infiniband/core/sa_query.c +0 -4
 	if (ret) {
 		ret = -EIO;
-		/* Remove the request */
-		spin_lock_irqsave(&ib_nl_request_lock, flags);
-		list_del(&query->list);
-		spin_unlock_irqrestore(&ib_nl_request_lock, flags);
 	}

Brackets should be removed too.

Noted.

Quoted 4 lines
Quoted 4 lines
drivers/infiniband/core/sa_query.c +0 -1
 	return ret;
--

1.8.3.1

#4 lore raw

Re: [PATCH v3] IB/sa: Resolving use-after-free in ib_nl_send_msg

Quoted 116 lines · Divya Indi

Hi Leon,

Thanks for taking the time to review.

Please find my comments inline -

Quoted 46 lines · Leon Romanovsky
Quoted 45 lines · Divya Indi

Commit 3ebd2fd0d011 ("IB/sa: Put netlink request into the request list before sending")' -
1. Adds the query to the request list before ib_nl_snd_msg.
2. Removes ib_nl_send_msg from within the spinlock which also makes it possible to allocate memory with GFP_KERNEL.

However, if there is a delay in sending out the request (For
eg: Delay due to low memory situation) the timer to handle request timeout might kick in before the request is sent out to ibacm via netlink. ib_nl_request_timeout may release the query causing a use after free situation while accessing the query in ib_nl_send_msg.

Call Trace for the above race:

[<ffffffffa02f43cb>] ? ib_pack+0x17b/0x240 [ib_core]

[<ffffffffa032aef1>] ib_sa_path_rec_get+0x181/0x200 [ib_sa]
[<ffffffffa0379db0>] rdma_resolve_route+0x3c0/0x8d0 [rdma_cm]
[<ffffffffa0374450>] ? cma_bind_port+0xa0/0xa0 [rdma_cm]
[<ffffffffa040f850>] ? rds_rdma_cm_event_handler_cmn+0x850/0x850
[rds_rdma]
[<ffffffffa040f22c>] rds_rdma_cm_event_handler_cmn+0x22c/0x850
[rds_rdma]
[<ffffffffa040f860>] rds_rdma_cm_event_handler+0x10/0x20 [rds_rdma]
[<ffffffffa037778e>] addr_handler+0x9e/0x140 [rdma_cm]
[<ffffffffa026cdb4>] process_req+0x134/0x190 [ib_addr]
[<ffffffff810a02f9>] process_one_work+0x169/0x4a0
[<ffffffff810a0b2b>] worker_thread+0x5b/0x560
[<ffffffff810a0ad0>] ? flush_delayed_work+0x50/0x50
[<ffffffff810a68fb>] kthread+0xcb/0xf0
[<ffffffff816ec49a>] ? __schedule+0x24a/0x810
[<ffffffff816ec49a>] ? __schedule+0x24a/0x810
[<ffffffff810a6830>] ? kthread_create_on_node+0x180/0x180
[<ffffffff816f25a7>] ret_from_fork+0x47/0x90
[<ffffffff810a6830>] ? kthread_create_on_node+0x180/0x180

....
RIP [<ffffffffa03296cd>] send_mad+0x33d/0x5d0 [ib_sa]

To resolve the above issue -
1. Add the req to the request list only after the request has been sent out.
2. To handle the race where response comes in before adding request to the request list, send(rdma_nl_multicast) and add to list while holding the spinlock - request_lock.
3. Use GFP_NOWAIT for rdma_nl_multicast since it is called while holding a spinlock. In case of memory allocation failure, request will go out to SA.

Quoted 4 lines
Quoted 3 lines
Signed-off-by Divya Indi <[email protected]>
Fixes 3ebd2fd0d011 ("IB/sa: Put netlink request into the request list

before sending")

Author SOB should be after "Fixes" line.

My bad. Noted.

Quoted 23 lines
Quoted 23 lines
---
 drivers/infiniband/core/sa_query.c | 34 +++++++++++++++++-----------------
 1 file changed, 17 insertions(+), 17 deletions(-)
drivers/infiniband/core/sa_query.c +3 -0
index 74e0058..042c99b 100644
@@ -836,6 +836,9 @@ static int ib_nl_send_msg(struct ib_sa_query *query, gfp_t gfp_mask)
 	void *data;
 	struct ib_sa_mad *mad;
 	int len;
+	unsigned long flags;
+	unsigned long delay;
+	int ret;
 
 	mad = query->mad_buf->mad;
 	len = ib_nl_get_path_rec_attrs_len(mad->sa_hdr.comp_mask);

@@ -860,35 +863,32 @@ static int ib_nl_send_msg(struct ib_sa_query *query, gfp_t gfp_mask)
 	/* Repair the nlmsg header length */
 	nlmsg_end(skb, nlh);
Quoted 7 lines

drivers/infiniband/core/sa_query.c static int ib_nl_send_msg(struct ib_sa_query *query, gfp_t gfp_mask) · line 867

Quoted 3 lines
drivers/infiniband/core/sa_query.c +2 -1
-	return rdma_nl_multicast(&init_net, skb, RDMA_NL_GROUP_LS, gfp_mask);
+	spin_lock_irqsave(&ib_nl_request_lock, flags);
+	ret =  rdma_nl_multicast(&init_net, skb, RDMA_NL_GROUP_LS, GFP_NOWAIT);

It is hard to be convinced that this is correct solution. The mix of gfp_flags and GFP_NOWAIT at the same time and usage of ib_nl_request_lock to protect lists and suddenly rdma_nl_multicast() too makes this code unreadable/non-maintainable.

Prior to 3ebd2fd0d011 ("IB/sa: Put netlink request into the request list before sending"), we had ib_nl_send_msg under the spinlock ib_nl_request_lock.

ie we had -

1. Get spinlock - ib_nl_request_lock
2. ib_nl_send_msg
2.a) rdma_nl_multicast
3. Add request to the req list
4. Arm the timer if needed.
5. Release spinlock

However, ib_nl_send_msg involved a memory allocation using GFP_KERNEL. hence, was moved out of the spinlock. In addition, req was now being added prior to ib_nl_send_msg [To handle the race where response can come in before we get a chance to add the request back to the list].

This introduced another race resulting in use-after-free.[Described in the commit.]

To resolve this, sending out the request and adding it to list need to happen while holding the request_lock.
To ensure minimum allocations while holding the lock, instead of having the entire ib_nl_send_msg under the lock, we only have rdma_nl_multicast under this spinlock.

Quoted 6 lines

However, do you think it would be a good idea to split ib_nl_send_msg into 2 functions -
1. Prepare the req/query [Outside the spinlock]
2. Sending the req - rdma_nl_multicast [while holding spinlock]

Would this be more intuitive?

While it is always good idea to minimize the locked period. It still doesn't answer concern about mixing gfp_flags and direct GFP_NOWAIT. For example if user provides GFP_ATOMIC, the GFP_NOWAIT allocation will cause a trouble because latter is more lax than first one.

Thanks

Quoted 60 lines
Quoted 10 lines

drivers/infiniband/core/sa_query.c static int ib_nl_send_msg(struct ib_sa_query *query, gfp_t gfp_mask) · line 868

Quoted 2 lines
drivers/infiniband/core/sa_query.c +1 -0
+	if (!ret) {

Please use kernel coding style.

if (ret) {
spin_unlock_irqrestore(&ib_nl_request_lock, flags); return ret;
}

....

Noted. Will make this change.

Quoted 34 lines
Quoted 34 lines
drivers/infiniband/core/sa_query.c +11 -12
+		/* Put the request on the list.*/
+		delay = msecs_to_jiffies(sa_local_svc_timeout_ms);
+		query->timeout = delay + jiffies;
+		list_add_tail(&query->list, &ib_nl_request_list);
+		/* Start the timeout if this is the only request */
+		if (ib_nl_request_list.next == &query->list)
+			queue_delayed_work(ib_nl_wq, &ib_nl_timed_work, delay);
+	}
+	spin_unlock_irqrestore(&ib_nl_request_lock, flags);
+
+	return ret;
 }
 
 static int ib_nl_make_request(struct ib_sa_query *query, gfp_t gfp_mask)
 {
-	unsigned long flags;
-	unsigned long delay;
 	int ret;
 
 	INIT_LIST_HEAD(&query->list);
 	query->seq = (u32)atomic_inc_return(&ib_nl_sa_request_seq);
 
-	/* Put the request on the list first.*/
-	spin_lock_irqsave(&ib_nl_request_lock, flags);
-	delay = msecs_to_jiffies(sa_local_svc_timeout_ms);
-	query->timeout = delay + jiffies;
-	list_add_tail(&query->list, &ib_nl_request_list);
-	/* Start the timeout if this is the only request */
-	if (ib_nl_request_list.next == &query->list)
-		queue_delayed_work(ib_nl_wq, &ib_nl_timed_work, delay);
-	spin_unlock_irqrestore(&ib_nl_request_lock, flags);
-
 	ret = ib_nl_send_msg(query, gfp_mask);
Quoted 8 lines

drivers/infiniband/core/sa_query.c static int ib_nl_send_msg(struct ib_sa_query *query, gfp_t gfp_mask) · line 892

Quoted 7 lines
drivers/infiniband/core/sa_query.c +0 -4
 	if (ret) {
 		ret = -EIO;
-		/* Remove the request */
-		spin_lock_irqsave(&ib_nl_request_lock, flags);
-		list_del(&query->list);
-		spin_unlock_irqrestore(&ib_nl_request_lock, flags);
 	}

Brackets should be removed too.

Noted.

Quoted 4 lines
Quoted 4 lines
drivers/infiniband/core/sa_query.c +0 -1
 	return ret;
--

1.8.3.1

author
#5 lore raw

Re: [PATCH v3] IB/sa: Resolving use-after-free in ib_nl_send_msg

Hi Leon,

Please find my comments inline -

Quoted 119 lines · Leon Romanovsky
Quoted 118 lines · Divya Indi

Hi Leon,

Thanks for taking the time to review.

Please find my comments inline -

Quoted 46 lines · Leon Romanovsky
Quoted 45 lines · Divya Indi

Commit 3ebd2fd0d011 ("IB/sa: Put netlink request into the request list before sending")' -
1. Adds the query to the request list before ib_nl_snd_msg.
2. Removes ib_nl_send_msg from within the spinlock which also makes it possible to allocate memory with GFP_KERNEL.

However, if there is a delay in sending out the request (For
eg: Delay due to low memory situation) the timer to handle request timeout might kick in before the request is sent out to ibacm via netlink. ib_nl_request_timeout may release the query causing a use after free situation while accessing the query in ib_nl_send_msg.

Call Trace for the above race:

[<ffffffffa02f43cb>] ? ib_pack+0x17b/0x240 [ib_core]

[<ffffffffa032aef1>] ib_sa_path_rec_get+0x181/0x200 [ib_sa]
[<ffffffffa0379db0>] rdma_resolve_route+0x3c0/0x8d0 [rdma_cm]
[<ffffffffa0374450>] ? cma_bind_port+0xa0/0xa0 [rdma_cm]
[<ffffffffa040f850>] ? rds_rdma_cm_event_handler_cmn+0x850/0x850
[rds_rdma]
[<ffffffffa040f22c>] rds_rdma_cm_event_handler_cmn+0x22c/0x850
[rds_rdma]
[<ffffffffa040f860>] rds_rdma_cm_event_handler+0x10/0x20 [rds_rdma]
[<ffffffffa037778e>] addr_handler+0x9e/0x140 [rdma_cm]
[<ffffffffa026cdb4>] process_req+0x134/0x190 [ib_addr]
[<ffffffff810a02f9>] process_one_work+0x169/0x4a0
[<ffffffff810a0b2b>] worker_thread+0x5b/0x560
[<ffffffff810a0ad0>] ? flush_delayed_work+0x50/0x50
[<ffffffff810a68fb>] kthread+0xcb/0xf0
[<ffffffff816ec49a>] ? __schedule+0x24a/0x810
[<ffffffff816ec49a>] ? __schedule+0x24a/0x810
[<ffffffff810a6830>] ? kthread_create_on_node+0x180/0x180
[<ffffffff816f25a7>] ret_from_fork+0x47/0x90
[<ffffffff810a6830>] ? kthread_create_on_node+0x180/0x180

....
RIP [<ffffffffa03296cd>] send_mad+0x33d/0x5d0 [ib_sa]

To resolve the above issue -
1. Add the req to the request list only after the request has been sent out.
2. To handle the race where response comes in before adding request to the request list, send(rdma_nl_multicast) and add to list while holding the spinlock - request_lock.
3. Use GFP_NOWAIT for rdma_nl_multicast since it is called while holding a spinlock. In case of memory allocation failure, request will go out to SA.

Quoted 4 lines
Quoted 3 lines
Signed-off-by Divya Indi <[email protected]>
Fixes 3ebd2fd0d011 ("IB/sa: Put netlink request into the request list

before sending")

Author SOB should be after "Fixes" line.

My bad. Noted.

Quoted 22 lines
Quoted 22 lines
---
 drivers/infiniband/core/sa_query.c | 34 +++++++++++++++++-----------------
 1 file changed, 17 insertions(+), 17 deletions(-)
drivers/infiniband/core/sa_query.c +3 -0
index 74e0058..042c99b 100644
@@ -836,6 +836,9 @@ static int ib_nl_send_msg(struct ib_sa_query *query, gfp_t gfp_mask)
 	void *data;
 	struct ib_sa_mad *mad;
 	int len;
+	unsigned long flags;
+	unsigned long delay;
+	int ret;
 
 	mad = query->mad_buf->mad;
 	len = ib_nl_get_path_rec_attrs_len(mad->sa_hdr.comp_mask);

@@ -860,35 +863,32 @@ static int ib_nl_send_msg(struct ib_sa_query *query, gfp_t gfp_mask)
 	/* Repair the nlmsg header length */
 	nlmsg_end(skb, nlh);
Quoted 7 lines

drivers/infiniband/core/sa_query.c static int ib_nl_send_msg(struct ib_sa_query *query, gfp_t gfp_mask) · line 867

Quoted 3 lines
drivers/infiniband/core/sa_query.c +2 -1
-	return rdma_nl_multicast(&init_net, skb, RDMA_NL_GROUP_LS, gfp_mask);
+	spin_lock_irqsave(&ib_nl_request_lock, flags);
+	ret =  rdma_nl_multicast(&init_net, skb, RDMA_NL_GROUP_LS, GFP_NOWAIT);

It is hard to be convinced that this is correct solution. The mix of gfp_flags and GFP_NOWAIT at the same time and usage of ib_nl_request_lock to protect lists and suddenly rdma_nl_multicast() too makes this code unreadable/non-maintainable.

Prior to 3ebd2fd0d011 ("IB/sa: Put netlink request into the request list before sending"), we had ib_nl_send_msg under the spinlock ib_nl_request_lock.

ie we had -

1. Get spinlock - ib_nl_request_lock
2. ib_nl_send_msg
2.a) rdma_nl_multicast
3. Add request to the req list
4. Arm the timer if needed.
5. Release spinlock

However, ib_nl_send_msg involved a memory allocation using GFP_KERNEL. hence, was moved out of the spinlock. In addition, req was now being added prior to ib_nl_send_msg [To handle the race where response can come in before we get a chance to add the request back to the list].

This introduced another race resulting in use-after-free.[Described in the commit.]

To resolve this, sending out the request and adding it to list need to happen while holding the request_lock.
To ensure minimum allocations while holding the lock, instead of having the entire ib_nl_send_msg under the lock, we only have rdma_nl_multicast under this spinlock.

However, do you think it would be a good idea to split ib_nl_send_msg into 2 functions -
1. Prepare the req/query [Outside the spinlock]
2. Sending the req - rdma_nl_multicast [while holding spinlock]

Quoted 5 lines
Quoted 1 line

Would this be more intuitive?

While it is always good idea to minimize the locked period. It still doesn't answer concern about mixing gfp_flags and direct GFP_NOWAIT. For example if user provides GFP_ATOMIC, the GFP_NOWAIT allocation will cause a trouble because latter is more lax than first one.

Makes sense, and we do have callers passing GFP_ATOMIC with gfp_mask.

However, in this case when we fail to send the request to ibacm, we then fallback to sending it to the SA with gfp_mask. So, the request will eventually go out with GFP_ATOMIC to SA. From the caller perspective the request will not fail due to memory pressure.

-------
send_mad(...gfp_mask)
- send to ibacm with GFP_NOWAIT
- If fails, send to SA with gfp_mask
-------

So, using GFP_NOWAIT may not cause trouble here.

The other option might be to use GFP_NOWAIT conditionally ie (only use GFP_NOWAIT when GFP_ATOMIC is not specified in gfp_mask else use GFP_ATOMIC). Eventual goal being to not have a blocking memory allocation.

Your thoughts?

Really appreciate your feedback. Thanks!

Regards,
Divya

Quoted 60 lines

Thanks

Quoted 57 lines
Quoted 9 lines

drivers/infiniband/core/sa_query.c static int ib_nl_send_msg(struct ib_sa_query *query, gfp_t gfp_mask) · line 868

Quoted 1 line
drivers/infiniband/core/sa_query.c +1 -0
+	if (!ret) {

Please use kernel coding style.

if (ret) {
spin_unlock_irqrestore(&ib_nl_request_lock, flags); return ret;
}

....

Noted. Will make this change.

Quoted 33 lines
Quoted 33 lines
drivers/infiniband/core/sa_query.c +11 -12
+		/* Put the request on the list.*/
+		delay = msecs_to_jiffies(sa_local_svc_timeout_ms);
+		query->timeout = delay + jiffies;
+		list_add_tail(&query->list, &ib_nl_request_list);
+		/* Start the timeout if this is the only request */
+		if (ib_nl_request_list.next == &query->list)
+			queue_delayed_work(ib_nl_wq, &ib_nl_timed_work, delay);
+	}
+	spin_unlock_irqrestore(&ib_nl_request_lock, flags);
+
+	return ret;
 }
 
 static int ib_nl_make_request(struct ib_sa_query *query, gfp_t gfp_mask)
 {
-	unsigned long flags;
-	unsigned long delay;
 	int ret;
 
 	INIT_LIST_HEAD(&query->list);
 	query->seq = (u32)atomic_inc_return(&ib_nl_sa_request_seq);
 
-	/* Put the request on the list first.*/
-	spin_lock_irqsave(&ib_nl_request_lock, flags);
-	delay = msecs_to_jiffies(sa_local_svc_timeout_ms);
-	query->timeout = delay + jiffies;
-	list_add_tail(&query->list, &ib_nl_request_list);
-	/* Start the timeout if this is the only request */
-	if (ib_nl_request_list.next == &query->list)
-		queue_delayed_work(ib_nl_wq, &ib_nl_timed_work, delay);
-	spin_unlock_irqrestore(&ib_nl_request_lock, flags);
-
 	ret = ib_nl_send_msg(query, gfp_mask);
Quoted 8 lines

drivers/infiniband/core/sa_query.c static int ib_nl_send_msg(struct ib_sa_query *query, gfp_t gfp_mask) · line 892

Quoted 7 lines
drivers/infiniband/core/sa_query.c +0 -4
 	if (ret) {
 		ret = -EIO;
-		/* Remove the request */
-		spin_lock_irqsave(&ib_nl_request_lock, flags);
-		list_del(&query->list);
-		spin_unlock_irqrestore(&ib_nl_request_lock, flags);
 	}

Brackets should be removed too.

Noted.

Quoted 4 lines
Quoted 4 lines
drivers/infiniband/core/sa_query.c +0 -1
 	return ret;
--

1.8.3.1

#6 lore raw

Re: [PATCH v3] IB/sa: Resolving use-after-free in ib_nl_send_msg

Quoted 143 lines · Divya Indi

Hi Leon,

Please find my comments inline -

Quoted 119 lines · Leon Romanovsky
Quoted 118 lines · Divya Indi

Hi Leon,

Thanks for taking the time to review.

Please find my comments inline -

Quoted 46 lines · Leon Romanovsky
Quoted 45 lines · Divya Indi

Commit 3ebd2fd0d011 ("IB/sa: Put netlink request into the request list before sending")' -
1. Adds the query to the request list before ib_nl_snd_msg.
2. Removes ib_nl_send_msg from within the spinlock which also makes it possible to allocate memory with GFP_KERNEL.

However, if there is a delay in sending out the request (For
eg: Delay due to low memory situation) the timer to handle request timeout might kick in before the request is sent out to ibacm via netlink. ib_nl_request_timeout may release the query causing a use after free situation while accessing the query in ib_nl_send_msg.

Call Trace for the above race:

[<ffffffffa02f43cb>] ? ib_pack+0x17b/0x240 [ib_core]

[<ffffffffa032aef1>] ib_sa_path_rec_get+0x181/0x200 [ib_sa]
[<ffffffffa0379db0>] rdma_resolve_route+0x3c0/0x8d0 [rdma_cm]
[<ffffffffa0374450>] ? cma_bind_port+0xa0/0xa0 [rdma_cm]
[<ffffffffa040f850>] ? rds_rdma_cm_event_handler_cmn+0x850/0x850
[rds_rdma]
[<ffffffffa040f22c>] rds_rdma_cm_event_handler_cmn+0x22c/0x850
[rds_rdma]
[<ffffffffa040f860>] rds_rdma_cm_event_handler+0x10/0x20 [rds_rdma]
[<ffffffffa037778e>] addr_handler+0x9e/0x140 [rdma_cm]
[<ffffffffa026cdb4>] process_req+0x134/0x190 [ib_addr]
[<ffffffff810a02f9>] process_one_work+0x169/0x4a0
[<ffffffff810a0b2b>] worker_thread+0x5b/0x560
[<ffffffff810a0ad0>] ? flush_delayed_work+0x50/0x50
[<ffffffff810a68fb>] kthread+0xcb/0xf0
[<ffffffff816ec49a>] ? __schedule+0x24a/0x810
[<ffffffff816ec49a>] ? __schedule+0x24a/0x810
[<ffffffff810a6830>] ? kthread_create_on_node+0x180/0x180
[<ffffffff816f25a7>] ret_from_fork+0x47/0x90
[<ffffffff810a6830>] ? kthread_create_on_node+0x180/0x180

....
RIP [<ffffffffa03296cd>] send_mad+0x33d/0x5d0 [ib_sa]

To resolve the above issue -
1. Add the req to the request list only after the request has been sent out.
2. To handle the race where response comes in before adding request to the request list, send(rdma_nl_multicast) and add to list while holding the spinlock - request_lock.
3. Use GFP_NOWAIT for rdma_nl_multicast since it is called while holding a spinlock. In case of memory allocation failure, request will go out to SA.

Quoted 4 lines
Quoted 3 lines
Signed-off-by Divya Indi <[email protected]>
Fixes 3ebd2fd0d011 ("IB/sa: Put netlink request into the request list

before sending")

Author SOB should be after "Fixes" line.

My bad. Noted.

Quoted 22 lines
Quoted 22 lines
---
 drivers/infiniband/core/sa_query.c | 34 +++++++++++++++++-----------------
 1 file changed, 17 insertions(+), 17 deletions(-)
drivers/infiniband/core/sa_query.c +3 -0
index 74e0058..042c99b 100644
@@ -836,6 +836,9 @@ static int ib_nl_send_msg(struct ib_sa_query *query, gfp_t gfp_mask)
 	void *data;
 	struct ib_sa_mad *mad;
 	int len;
+	unsigned long flags;
+	unsigned long delay;
+	int ret;
 
 	mad = query->mad_buf->mad;
 	len = ib_nl_get_path_rec_attrs_len(mad->sa_hdr.comp_mask);

@@ -860,35 +863,32 @@ static int ib_nl_send_msg(struct ib_sa_query *query, gfp_t gfp_mask)
 	/* Repair the nlmsg header length */
 	nlmsg_end(skb, nlh);
Quoted 7 lines

drivers/infiniband/core/sa_query.c static int ib_nl_send_msg(struct ib_sa_query *query, gfp_t gfp_mask) · line 867

Quoted 3 lines
drivers/infiniband/core/sa_query.c +2 -1
-	return rdma_nl_multicast(&init_net, skb, RDMA_NL_GROUP_LS, gfp_mask);
+	spin_lock_irqsave(&ib_nl_request_lock, flags);
+	ret =  rdma_nl_multicast(&init_net, skb, RDMA_NL_GROUP_LS, GFP_NOWAIT);

It is hard to be convinced that this is correct solution. The mix of gfp_flags and GFP_NOWAIT at the same time and usage of ib_nl_request_lock to protect lists and suddenly rdma_nl_multicast() too makes this code unreadable/non-maintainable.

Prior to 3ebd2fd0d011 ("IB/sa: Put netlink request into the request list before sending"), we had ib_nl_send_msg under the spinlock ib_nl_request_lock.

ie we had -

1. Get spinlock - ib_nl_request_lock
2. ib_nl_send_msg
2.a) rdma_nl_multicast
3. Add request to the req list
4. Arm the timer if needed.
5. Release spinlock

However, ib_nl_send_msg involved a memory allocation using GFP_KERNEL. hence, was moved out of the spinlock. In addition, req was now being added prior to ib_nl_send_msg [To handle the race where response can come in before we get a chance to add the request back to the list].

This introduced another race resulting in use-after-free.[Described in the commit.]

To resolve this, sending out the request and adding it to list need to happen while holding the request_lock.
To ensure minimum allocations while holding the lock, instead of having the entire ib_nl_send_msg under the lock, we only have rdma_nl_multicast under this spinlock.

However, do you think it would be a good idea to split ib_nl_send_msg into 2 functions -
1. Prepare the req/query [Outside the spinlock]
2. Sending the req - rdma_nl_multicast [while holding spinlock]

Quoted 5 lines
Quoted 1 line

Would this be more intuitive?

While it is always good idea to minimize the locked period. It still doesn't answer concern about mixing gfp_flags and direct GFP_NOWAIT. For example if user provides GFP_ATOMIC, the GFP_NOWAIT allocation will cause a trouble because latter is more lax than first one.

Makes sense, and we do have callers passing GFP_ATOMIC with gfp_mask.

However, in this case when we fail to send the request to ibacm, we then fallback to sending it to the SA with gfp_mask. So, the request will eventually go out with GFP_ATOMIC to SA. From the caller perspective the request will not fail due to memory pressure.

-------
send_mad(...gfp_mask)
- send to ibacm with GFP_NOWAIT
- If fails, send to SA with gfp_mask
-------

Quoted 7 lines

So, using GFP_NOWAIT may not cause trouble here.

The other option might be to use GFP_NOWAIT conditionally ie (only use GFP_NOWAIT when GFP_ATOMIC is not specified in gfp_mask else use GFP_ATOMIC). Eventual goal being to not have a blocking memory allocation.

Your thoughts?

My thoughts that everything here hints me that state machine and locking are implemented wrongly. In ideal world, the expectation is that REQ message will have a state in it (PREPARED, SENT, ACK e.t.c.) and list manipulations are done accordingly with proper locks, while rdma_nl_multicast() is done outside of the locks.

I don't know if it is possible to fix.

Quoted 67 lines

Really appreciate your feedback. Thanks!

Regards,
Divya

Quoted 60 lines

Thanks

Quoted 57 lines
Quoted 9 lines

drivers/infiniband/core/sa_query.c static int ib_nl_send_msg(struct ib_sa_query *query, gfp_t gfp_mask) · line 868

Quoted 1 line
drivers/infiniband/core/sa_query.c +1 -0
+	if (!ret) {

Please use kernel coding style.

if (ret) {
spin_unlock_irqrestore(&ib_nl_request_lock, flags); return ret;
}

....

Noted. Will make this change.

Quoted 33 lines
Quoted 33 lines
drivers/infiniband/core/sa_query.c +11 -12
+		/* Put the request on the list.*/
+		delay = msecs_to_jiffies(sa_local_svc_timeout_ms);
+		query->timeout = delay + jiffies;
+		list_add_tail(&query->list, &ib_nl_request_list);
+		/* Start the timeout if this is the only request */
+		if (ib_nl_request_list.next == &query->list)
+			queue_delayed_work(ib_nl_wq, &ib_nl_timed_work, delay);
+	}
+	spin_unlock_irqrestore(&ib_nl_request_lock, flags);
+
+	return ret;
 }
 
 static int ib_nl_make_request(struct ib_sa_query *query, gfp_t gfp_mask)
 {
-	unsigned long flags;
-	unsigned long delay;
 	int ret;
 
 	INIT_LIST_HEAD(&query->list);
 	query->seq = (u32)atomic_inc_return(&ib_nl_sa_request_seq);
 
-	/* Put the request on the list first.*/
-	spin_lock_irqsave(&ib_nl_request_lock, flags);
-	delay = msecs_to_jiffies(sa_local_svc_timeout_ms);
-	query->timeout = delay + jiffies;
-	list_add_tail(&query->list, &ib_nl_request_list);
-	/* Start the timeout if this is the only request */
-	if (ib_nl_request_list.next == &query->list)
-		queue_delayed_work(ib_nl_wq, &ib_nl_timed_work, delay);
-	spin_unlock_irqrestore(&ib_nl_request_lock, flags);
-
 	ret = ib_nl_send_msg(query, gfp_mask);
Quoted 8 lines

drivers/infiniband/core/sa_query.c static int ib_nl_send_msg(struct ib_sa_query *query, gfp_t gfp_mask) · line 892

Quoted 7 lines
drivers/infiniband/core/sa_query.c +0 -4
 	if (ret) {
 		ret = -EIO;
-		/* Remove the request */
-		spin_lock_irqsave(&ib_nl_request_lock, flags);
-		list_del(&query->list);
-		spin_unlock_irqrestore(&ib_nl_request_lock, flags);
 	}

Brackets should be removed too.

Noted.

Quoted 4 lines
Quoted 4 lines
drivers/infiniband/core/sa_query.c +0 -1
 	return ret;
--

1.8.3.1

#7 lore raw

Re: [PATCH v3] IB/sa: Resolving use-after-free in ib_nl_send_msg

Quoted 6 lines · Leon Romanovsky

My thoughts that everything here hints me that state machine and locking are implemented wrongly. In ideal world, the expectation is that REQ message will have a state in it (PREPARED, SENT, ACK e.t.c.) and list manipulations are done accordingly with proper locks, while rdma_nl_multicast() is done outside of the locks.

It can't be done outside the lock without creating races - once rdma_nl_multicast happens it is possible for the other leg of the operation to begin processing.

The list must be updated before this happens.

What is missing here is refcounting - the lifetime model of this data is too implicit, but it is not worth adding I think

Jason

#8 lore raw

Re: [PATCH v3] IB/sa: Resolving use-after-free in ib_nl_send_msg

Quoted 11 lines · Jason Gunthorpe
Quoted 6 lines · Leon Romanovsky

My thoughts that everything here hints me that state machine and locking are implemented wrongly. In ideal world, the expectation is that REQ message will have a state in it (PREPARED, SENT, ACK e.t.c.) and list manipulations are done accordingly with proper locks, while rdma_nl_multicast() is done outside of the locks.

It can't be done outside the lock without creating races - once rdma_nl_multicast happens it is possible for the other leg of the operation to begin processing.

It means that the state machine is wrong, not complete.

Quoted 5 lines

The list must be updated before this happens.

What is missing here is refcounting - the lifetime model of this data is too implicit, but it is not worth adding I think

I have same feeling for now, but it will flip if new fixes be in this area.

Thanks

Quoted 2 lines

Jason

#9 lore raw

Re: [PATCH v3] IB/sa: Resolving use-after-free in ib_nl_send_msg

In reply to Divya Indi
Quoted 3 lines · Divya Indi

The other option might be to use GFP_NOWAIT conditionally ie (only use GFP_NOWAIT when GFP_ATOMIC is not specified in gfp_mask else use GFP_ATOMIC). Eventual goal being to not have a blocking memory allocation.

This is probably safest for now, unless you can audit all callers and see if they can switch to GFP_NOWAIT as well

Jason

author
#10 lore raw

Re: [PATCH v3] IB/sa: Resolving use-after-free in ib_nl_send_msg

Hi Jason,

Thanks for taking the time to review!

Quoted 6 lines · Jason Gunthorpe
Quoted 3 lines · Divya Indi

The other option might be to use GFP_NOWAIT conditionally ie (only use GFP_NOWAIT when GFP_ATOMIC is not specified in gfp_mask else use GFP_ATOMIC). Eventual goal being to not have a blocking memory allocation.

This is probably safest for now, unless you can audit all callers and see if they can switch to GFP_NOWAIT as well

At present the callers with GFP_ATOMIC appear to be ipoib. Might not be feasible to change them all to GFP_NOWAIT.

Will incorporate the review comments and send out v3 early next week.

Thanks,
Divya

Quoted 2 lines

Jason

#11 lore raw
In reply to Divya Indi
Quoted 5 lines · Divya Indi

[PATCH v3] IB/sa: Resolving use-after-free in ib_nl_send_msg

Hi,

Please review the patch that follows.

Please read Documentation/process/submitting-patches.rst
14) The canonical patch format

You don't need an extra email "Review request" and Changelog should be put inside the patch itself under "---" marker.

Thanks

author
#12 lore raw

Thanks Leon, Noted!

Quoted 13 lines · Leon Romanovsky
Quoted 5 lines · Divya Indi

[PATCH v3] IB/sa: Resolving use-after-free in ib_nl_send_msg

Hi,

Please review the patch that follows.

Please read Documentation/process/submitting-patches.rst
14) The canonical patch format

You don't need an extra email "Review request" and Changelog should be put inside the patch itself under "---" marker.

Thanks