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
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
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.