netdev - Re: [PATCH net v1] octeon_ep: initialise control mbox tasks before using APIs

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CADEbmW12OWS6et2wp3skicUM=V81x8dS4_aySYP1Ok0kEc2M9Q@mail.gmail.com>
Date: Mon, 4 Dec 2023 23:10:47 +0100
From: Michal Schmidt <mschmidt@...hat.com>
To: Shinas Rasheed <srasheed@...vell.com>
Cc: netdev@...r.kernel.org, linux-kernel@...r.kernel.org, hgani@...vell.com, 
	vimleshk@...vell.com, egallen@...hat.com, pabeni@...hat.com, horms@...nel.org, 
	kuba@...nel.org, davem@...emloft.net, wizhao@...hat.com, konguyen@...hat.com, 
	Veerasenareddy Burru <vburru@...vell.com>, Sathesh Edara <sedara@...vell.com>, 
	Eric Dumazet <edumazet@...gle.com>
Subject: Re: [PATCH net v1] octeon_ep: initialise control mbox tasks before
 using APIs

On Sat, Dec 2, 2023 at 4:08 PM Shinas Rasheed <srasheed@...vell.com> wrote:
> Do INIT_WORK for the various workqueue tasks before the first
> invocation of any control net APIs. Since octep_ctrl_net_get_info
> was called before the control net receive work task was even
> initialised, the function call wasn't returning actual firmware
> info queried from Octeon.

It might be more accurate to say that octep_ctrl_net_get_info depends
on the processing of OEI events. This happens in intr_poll_task.
That's why intr_poll_task needs to be queued earlier.
Did octep_send_mbox_req previously always fail with EAGAIN after
running into the 500 ms timeout in octep_send_mbox_req?

Apropos octep_send_mbox_req... I think it has a race. "d" is put on
the ctrl_req_wait_list after sending the request to the hardware. If
the response arrives quickly, "d" might not yet be on the list when
process_mbox_resp looks for it.
Also, what protects ctrl_req_wait_list from concurrent access?

Michal

> Fixes: 8d6198a14e2b ("octeon_ep: support to fetch firmware info")
> Signed-off-by: Shinas Rasheed <srasheed@...vell.com>
> ---
>  .../net/ethernet/marvell/octeon_ep/octep_main.c    | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/net/ethernet/marvell/octeon_ep/octep_main.c b/drivers/net/ethernet/marvell/octeon_ep/octep_main.c
> index 552970c7dec0..3e7bfd3e0f56 100644
> --- a/drivers/net/ethernet/marvell/octeon_ep/octep_main.c
> +++ b/drivers/net/ethernet/marvell/octeon_ep/octep_main.c
> @@ -1193,6 +1193,13 @@ int octep_device_setup(struct octep_device *oct)
>         if (ret)
>                 return ret;
>
> +       INIT_WORK(&oct->tx_timeout_task, octep_tx_timeout_task);
> +       INIT_WORK(&oct->ctrl_mbox_task, octep_ctrl_mbox_task);
> +       INIT_DELAYED_WORK(&oct->intr_poll_task, octep_intr_poll_task);
> +       oct->poll_non_ioq_intr = true;
> +       queue_delayed_work(octep_wq, &oct->intr_poll_task,
> +                          msecs_to_jiffies(OCTEP_INTR_POLL_TIME_MSECS));
> +
>         atomic_set(&oct->hb_miss_cnt, 0);
>         INIT_DELAYED_WORK(&oct->hb_task, octep_hb_timeout_task);
>
> @@ -1333,13 +1340,6 @@ static int octep_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>         queue_delayed_work(octep_wq, &octep_dev->hb_task,
>                            msecs_to_jiffies(octep_dev->conf->fw_info.hb_interval));
>
> -       INIT_WORK(&octep_dev->tx_timeout_task, octep_tx_timeout_task);
> -       INIT_WORK(&octep_dev->ctrl_mbox_task, octep_ctrl_mbox_task);
> -       INIT_DELAYED_WORK(&octep_dev->intr_poll_task, octep_intr_poll_task);
> -       octep_dev->poll_non_ioq_intr = true;
> -       queue_delayed_work(octep_wq, &octep_dev->intr_poll_task,
> -                          msecs_to_jiffies(OCTEP_INTR_POLL_TIME_MSECS));
> -
>         netdev->netdev_ops = &octep_netdev_ops;
>         octep_set_ethtool_ops(netdev);
>         netif_carrier_off(netdev);
> --
> 2.25.1
>