[PATCH net 0/4] octeon_ep: fixes for error and remove paths

[PATCH net 0/4] octeon_ep: fixes for error and remove paths

[Michal Schmidt]

I have an Octeon card that's misconfigured in a way that exposes a
couple of bugs in the octeon_ep driver's error paths. It can reproduce
the issues that patches 1 & 4 are fixing. Patches 2 & 3 are a result of
reviewing the nearby code.

Michal Schmidt (4):
  octeon_ep: fix timeout value for waiting on mbox response
  octeon_ep: cancel tx_timeout_task later in remove sequence
  octeon_ep: cancel ctrl_mbox_task after intr_poll_task
  octeon_ep: cancel queued works in probe error path

 drivers/net/ethernet/marvell/octeon_ep/octep_ctrl_net.c | 2 +-
 drivers/net/ethernet/marvell/octeon_ep/octep_main.c     | 9 +++++----
 2 files changed, 6 insertions(+), 5 deletions(-)

-- 
2.41.0

[PATCH net 1/4] octeon_ep: fix timeout value for waiting on mbox response

[Michal Schmidt]

The intention was to wait up to 500 ms for the mbox response.
The third argument to wait_event_interruptible_timeout() is supposed to
be the timeout duration. The driver mistakenly passed absolute time
instead.

Fixes: 577f0d1b1c5f ("octeon_ep: add separate mailbox command and response queues")
Signed-off-by: Michal Schmidt <mschmidt@redhat.com>
---
 drivers/net/ethernet/marvell/octeon_ep/octep_ctrl_net.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/marvell/octeon_ep/octep_ctrl_net.c b/drivers/net/ethernet/marvell/octeon_ep/octep_ctrl_net.c
index 1cc6af2feb38..565320ec24f8 100644
--- a/drivers/net/ethernet/marvell/octeon_ep/octep_ctrl_net.c
+++ b/drivers/net/ethernet/marvell/octeon_ep/octep_ctrl_net.c
@@ -55,7 +55,7 @@ static int octep_send_mbox_req(struct octep_device *oct,
 	list_add_tail(&d->list, &oct->ctrl_req_wait_list);
 	ret = wait_event_interruptible_timeout(oct->ctrl_req_wait_q,
 					       (d->done != 0),
-					       jiffies + msecs_to_jiffies(500));
+					       msecs_to_jiffies(500));
 	list_del(&d->list);
 	if (ret == 0 || ret == 1)
 		return -EAGAIN;
-- 
2.41.0

Re: [PATCH net 1/4] octeon_ep: fix timeout value for waiting on mbox response

[Simon Horman]

On Thu, Aug 10, 2023 at 05:01:11PM +0200, Michal Schmidt wrote:
> The intention was to wait up to 500 ms for the mbox response.
> The third argument to wait_event_interruptible_timeout() is supposed to
> be the timeout duration. The driver mistakenly passed absolute time
> instead.
> 
> Fixes: 577f0d1b1c5f ("octeon_ep: add separate mailbox command and response queues")
> Signed-off-by: Michal Schmidt <mschmidt@redhat.com>

Reviewed-by: Simon Horman <horms@kernel.org>

[PATCH net 2/4] octeon_ep: cancel tx_timeout_task later in remove sequence

[Michal Schmidt]

tx_timeout_task is canceled too early when removing the driver. Nothing
prevents .ndo_tx_timeout from triggering and queuing the work again.

Better cancel it after the netdev is unregistered.
It's harmless for octep_tx_timeout_task to run in the window between the
unregistration and cancelation, because it checks netif_running.

Fixes: 862cd659a6fb ("octeon_ep: Add driver framework and device initialization")
Signed-off-by: Michal Schmidt <mschmidt@redhat.com>
---
 drivers/net/ethernet/marvell/octeon_ep/octep_main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/marvell/octeon_ep/octep_main.c b/drivers/net/ethernet/marvell/octeon_ep/octep_main.c
index 43eb6e871351..d8066bff5f7b 100644
--- a/drivers/net/ethernet/marvell/octeon_ep/octep_main.c
+++ b/drivers/net/ethernet/marvell/octeon_ep/octep_main.c
@@ -1200,12 +1200,12 @@ static void octep_remove(struct pci_dev *pdev)
 	if (!oct)
 		return;
 
-	cancel_work_sync(&oct->tx_timeout_task);
 	cancel_work_sync(&oct->ctrl_mbox_task);
 	netdev = oct->netdev;
 	if (netdev->reg_state == NETREG_REGISTERED)
 		unregister_netdev(netdev);
 
+	cancel_work_sync(&oct->tx_timeout_task);
 	oct->poll_non_ioq_intr = false;
 	cancel_delayed_work_sync(&oct->intr_poll_task);
 	octep_device_cleanup(oct);
-- 
2.41.0

Re: [PATCH net 2/4] octeon_ep: cancel tx_timeout_task later in remove sequence

[Simon Horman]

On Thu, Aug 10, 2023 at 05:01:12PM +0200, Michal Schmidt wrote:
> tx_timeout_task is canceled too early when removing the driver. Nothing

nit: canceled -> cancelled

     also elsewhere in this patchset

     ./checkpatch.pl --codespell is your friend here

...

Re: [PATCH net 2/4] octeon_ep: cancel tx_timeout_task later in remove sequence

[Michal Schmidt]

On Thu, Aug 10, 2023 at 5:48 PM Simon Horman <horms@kernel.org> wrote:
>
> On Thu, Aug 10, 2023 at 05:01:12PM +0200, Michal Schmidt wrote:
> > tx_timeout_task is canceled too early when removing the driver. Nothing
>
> nit: canceled -> cancelled
>
>      also elsewhere in this patchset
>
>      ./checkpatch.pl --codespell is your friend here
>
> ...

Hi Simon,
thank you for the review.

I think both spellings are valid.
https://www.grammarly.com/blog/canceled-vs-cancelled/

Do you want me to resend the patchset for this?

Michal

Re: [PATCH net 2/4] octeon_ep: cancel tx_timeout_task later in remove sequence

[Simon Horman]

On Fri, Aug 11, 2023 at 05:58:44PM +0200, Michal Schmidt wrote:
> On Thu, Aug 10, 2023 at 5:48 PM Simon Horman <horms@kernel.org> wrote:
> >
> > On Thu, Aug 10, 2023 at 05:01:12PM +0200, Michal Schmidt wrote:
> > > tx_timeout_task is canceled too early when removing the driver. Nothing
> >
> > nit: canceled -> cancelled
> >
> >      also elsewhere in this patchset
> >
> >      ./checkpatch.pl --codespell is your friend here
> >
> > ...
> 
> Hi Simon,
> thank you for the review.
> 
> I think both spellings are valid.
> https://www.grammarly.com/blog/canceled-vs-cancelled/

Thanks, I did not know that.

> Do you want me to resend the patchset for this?

No, not in light of your previous point.

-- 
pw-bot: under-review

[PATCH net 3/4] octeon_ep: cancel ctrl_mbox_task after intr_poll_task

[Michal Schmidt]

intr_poll_task may queue ctrl_mbox_task. The function
octep_poll_non_ioq_interrupts_cn93_pf does this.

When removing the driver and canceling these two works, cancel
ctrl_mbox_task last to guarantee it does not run anymore.

Fixes: 24d4333233b3 ("octeon_ep: poll for control messages")
Signed-off-by: Michal Schmidt <mschmidt@redhat.com>
---
 drivers/net/ethernet/marvell/octeon_ep/octep_main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/marvell/octeon_ep/octep_main.c b/drivers/net/ethernet/marvell/octeon_ep/octep_main.c
index d8066bff5f7b..ab69b6d62509 100644
--- a/drivers/net/ethernet/marvell/octeon_ep/octep_main.c
+++ b/drivers/net/ethernet/marvell/octeon_ep/octep_main.c
@@ -1200,7 +1200,6 @@ static void octep_remove(struct pci_dev *pdev)
 	if (!oct)
 		return;
 
-	cancel_work_sync(&oct->ctrl_mbox_task);
 	netdev = oct->netdev;
 	if (netdev->reg_state == NETREG_REGISTERED)
 		unregister_netdev(netdev);
@@ -1208,6 +1207,7 @@ static void octep_remove(struct pci_dev *pdev)
 	cancel_work_sync(&oct->tx_timeout_task);
 	oct->poll_non_ioq_intr = false;
 	cancel_delayed_work_sync(&oct->intr_poll_task);
+	cancel_work_sync(&oct->ctrl_mbox_task);
 	octep_device_cleanup(oct);
 	pci_release_mem_regions(pdev);
 	free_netdev(netdev);
-- 
2.41.0

[PATCH net 4/4] octeon_ep: cancel queued works in probe error path

[Michal Schmidt]

If it fails to get the devices's MAC address, octep_probe exits while
leaving the delayed work intr_poll_task queued. When the work later
runs, it's a use after free.

Move the cancelation of intr_poll_task from octep_remove into
octep_device_cleanup. This does not change anything in the octep_remove
flow, but octep_device_cleanup is called also in the octep_probe error
path, where the cancelation is needed.

Note that the cancelation of ctrl_mbox_task has to follow
intr_poll_task's, because the ctrl_mbox_task may be queued by
intr_poll_task.

Fixes: 24d4333233b3 ("octeon_ep: poll for control messages")
Signed-off-by: Michal Schmidt <mschmidt@redhat.com>
---
 drivers/net/ethernet/marvell/octeon_ep/octep_main.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/marvell/octeon_ep/octep_main.c b/drivers/net/ethernet/marvell/octeon_ep/octep_main.c
index ab69b6d62509..4424de2ffd70 100644
--- a/drivers/net/ethernet/marvell/octeon_ep/octep_main.c
+++ b/drivers/net/ethernet/marvell/octeon_ep/octep_main.c
@@ -1038,6 +1038,10 @@ static void octep_device_cleanup(struct octep_device *oct)
 {
 	int i;
 
+	oct->poll_non_ioq_intr = false;
+	cancel_delayed_work_sync(&oct->intr_poll_task);
+	cancel_work_sync(&oct->ctrl_mbox_task);
+
 	dev_info(&oct->pdev->dev, "Cleaning up Octeon Device ...\n");
 
 	for (i = 0; i < OCTEP_MAX_VF; i++) {
@@ -1205,9 +1209,6 @@ static void octep_remove(struct pci_dev *pdev)
 		unregister_netdev(netdev);
 
 	cancel_work_sync(&oct->tx_timeout_task);
-	oct->poll_non_ioq_intr = false;
-	cancel_delayed_work_sync(&oct->intr_poll_task);
-	cancel_work_sync(&oct->ctrl_mbox_task);
 	octep_device_cleanup(oct);
 	pci_release_mem_regions(pdev);
 	free_netdev(netdev);
-- 
2.41.0

Re: [PATCH net 0/4] octeon_ep: fixes for error and remove paths

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Thu, 10 Aug 2023 17:01:10 +0200 you wrote:
> I have an Octeon card that's misconfigured in a way that exposes a
> couple of bugs in the octeon_ep driver's error paths. It can reproduce
> the issues that patches 1 & 4 are fixing. Patches 2 & 3 are a result of
> reviewing the nearby code.
> 
> Michal Schmidt (4):
>   octeon_ep: fix timeout value for waiting on mbox response
>   octeon_ep: cancel tx_timeout_task later in remove sequence
>   octeon_ep: cancel ctrl_mbox_task after intr_poll_task
>   octeon_ep: cancel queued works in probe error path
> 
> [...]

Here is the summary with links:
  - [net,1/4] octeon_ep: fix timeout value for waiting on mbox response
    https://git.kernel.org/netdev/net/c/519b227904f0
  - [net,2/4] octeon_ep: cancel tx_timeout_task later in remove sequence
    https://git.kernel.org/netdev/net/c/28458c80006b
  - [net,3/4] octeon_ep: cancel ctrl_mbox_task after intr_poll_task
    https://git.kernel.org/netdev/net/c/607a7a45cdf3
  - [net,4/4] octeon_ep: cancel queued works in probe error path
    https://git.kernel.org/netdev/net/c/758c91078165

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html