DPDK patches and discussions
 help / color / mirror / Atom feed
* [PATCH v2] tap: fix write-after-free and double free of intr_handle
@ 2022-05-03 15:27 Quentin Armitage
  2022-05-04 11:17 ` David Marchand
  0 siblings, 1 reply; 3+ messages in thread
From: Quentin Armitage @ 2022-05-03 15:27 UTC (permalink / raw)
  To: dev; +Cc: David Marchand, Harman Kalra, stable, Quentin Armitage

rte_pmd_tun/tap_probe() allocates pmd->intr_handle in eth_dev_tap_create()
and it should not be freed until rte_pmd_tap_remove() is called.

Inspection of tap_rx_intr_vec_set() shows that the call to
tap_tx_intr_vec_uninstall() was calling rte_intr_instance_free() but
tap_tx_intr_vec_install() can then be immediately called, and this then
uses pmd->intr_handle without it being reallocated.

This commit moves the call of rte_intr_instance_free() from
tap_tx_intr_vec_uninstall() to rte_pmd_tap_remove().

Fixes: d61138d4f0e2 ("drivers: remove direct access to interrupt handle")

Changes in v2:
  Move rte_intr_instance_free() from tap_rx_intr_vec_uninstall()
  to tap_dev_close().

Signed-off-by: Quentin Armitage <quentin@armitage.org.uk>
---
 drivers/net/tap/rte_eth_tap.c | 2 ++
 drivers/net/tap/tap_intr.c    | 2 --
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
index bc3d56a311..5495818be6 100644
--- a/drivers/net/tap/rte_eth_tap.c
+++ b/drivers/net/tap/rte_eth_tap.c
@@ -1213,6 +1213,8 @@ tap_dev_close(struct rte_eth_dev *dev)
 	TAP_LOG(DEBUG, "Closing %s Ethernet device on numa %u",
 		tuntap_types[internals->type], rte_socket_id());
 
+	rte_intr_instance_free(internals->intr_handle);
+
 	if (internals->ioctl_sock != -1) {
 		close(internals->ioctl_sock);
 		internals->ioctl_sock = -1;
diff --git a/drivers/net/tap/tap_intr.c b/drivers/net/tap/tap_intr.c
index 56c343acea..a9097def1a 100644
--- a/drivers/net/tap/tap_intr.c
+++ b/drivers/net/tap/tap_intr.c
@@ -34,8 +34,6 @@ tap_rx_intr_vec_uninstall(struct rte_eth_dev *dev)
 	rte_intr_free_epoll_fd(intr_handle);
 	rte_intr_vec_list_free(intr_handle);
 	rte_intr_nb_efd_set(intr_handle, 0);
-
-	rte_intr_instance_free(intr_handle);
 }
 
 /**
-- 
2.34.1


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH v2] tap: fix write-after-free and double free of intr_handle
  2022-05-03 15:27 [PATCH v2] tap: fix write-after-free and double free of intr_handle Quentin Armitage
@ 2022-05-04 11:17 ` David Marchand
  2022-05-12  8:12   ` Andrew Rybchenko
  0 siblings, 1 reply; 3+ messages in thread
From: David Marchand @ 2022-05-04 11:17 UTC (permalink / raw)
  To: Quentin Armitage; +Cc: dev, Harman Kalra, dpdk stable

On Tue, May 3, 2022 at 8:23 PM Quentin Armitage <quentin@armitage.org.uk> wrote:
>
> rte_pmd_tun/tap_probe() allocates pmd->intr_handle in eth_dev_tap_create()
> and it should not be freed until rte_pmd_tap_remove() is called.
>
> Inspection of tap_rx_intr_vec_set() shows that the call to
> tap_tx_intr_vec_uninstall() was calling rte_intr_instance_free() but
> tap_tx_intr_vec_install() can then be immediately called, and this then
> uses pmd->intr_handle without it being reallocated.
>
> This commit moves the call of rte_intr_instance_free() from
> tap_tx_intr_vec_uninstall() to rte_pmd_tap_remove().
>
> Fixes: d61138d4f0e2 ("drivers: remove direct access to interrupt handle")

Cc: stable@dpdk.org

https://doc.dpdk.org/guides/contributing/patches.html#patch-for-stable-releases
The reason is that backport scripts look for a "Cc: stable@dpdk.org"
in the commitlog itself.

(no need for a v3 just for this, it can be fixed when applying)

>
> Changes in v2:
>   Move rte_intr_instance_free() from tap_rx_intr_vec_uninstall()
>   to tap_dev_close().

Nit: revisions changelog should be added as annotations (i.e. put
after the --- after the commitlog).


>
> Signed-off-by: Quentin Armitage <quentin@armitage.org.uk>

I did not test the change, but the fix lgtm.
The CI failure from UNH is a false positive.

Reviewed-by: David Marchand <david.marchand@redhat.com>


-- 
David Marchand


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH v2] tap: fix write-after-free and double free of intr_handle
  2022-05-04 11:17 ` David Marchand
@ 2022-05-12  8:12   ` Andrew Rybchenko
  0 siblings, 0 replies; 3+ messages in thread
From: Andrew Rybchenko @ 2022-05-12  8:12 UTC (permalink / raw)
  To: David Marchand, Quentin Armitage; +Cc: dev, Harman Kalra, dpdk stable

On 5/4/22 14:17, David Marchand wrote:
> On Tue, May 3, 2022 at 8:23 PM Quentin Armitage <quentin@armitage.org.uk> wrote:
>>
>> rte_pmd_tun/tap_probe() allocates pmd->intr_handle in eth_dev_tap_create()
>> and it should not be freed until rte_pmd_tap_remove() is called.
>>
>> Inspection of tap_rx_intr_vec_set() shows that the call to
>> tap_tx_intr_vec_uninstall() was calling rte_intr_instance_free() but
>> tap_tx_intr_vec_install() can then be immediately called, and this then
>> uses pmd->intr_handle without it being reallocated.
>>
>> This commit moves the call of rte_intr_instance_free() from
>> tap_tx_intr_vec_uninstall() to rte_pmd_tap_remove().
>>
>> Fixes: d61138d4f0e2 ("drivers: remove direct access to interrupt handle")
> 
> Cc: stable@dpdk.org
> 
> https://doc.dpdk.org/guides/contributing/patches.html#patch-for-stable-releases
> The reason is that backport scripts look for a "Cc: stable@dpdk.org"
> in the commitlog itself.
> 
> (no need for a v3 just for this, it can be fixed when applying)
> 
>>
>> Changes in v2:
>>    Move rte_intr_instance_free() from tap_rx_intr_vec_uninstall()
>>    to tap_dev_close().
> 
> Nit: revisions changelog should be added as annotations (i.e. put
> after the --- after the commitlog).
> 
> 
>>
>> Signed-off-by: Quentin Armitage <quentin@armitage.org.uk>
> 
> I did not test the change, but the fix lgtm.
> The CI failure from UNH is a false positive.
> 
> Reviewed-by: David Marchand <david.marchand@redhat.com>
> 
> 

Applied with minor fixes in summary and description, thanks.

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2022-05-12  8:12 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-05-03 15:27 [PATCH v2] tap: fix write-after-free and double free of intr_handle Quentin Armitage
2022-05-04 11:17 ` David Marchand
2022-05-12  8:12   ` Andrew Rybchenko

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).