| 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
| ||
|
Message-ID: <2c1e4c0f-ab98-4f74-9084-1ce1db984f2d@intel.com>
Date: Tue, 22 Apr 2025 15:08:03 +0200
From: Przemek Kitszel <przemyslaw.kitszel@...el.com>
To: Harshitha Ramamurthy <hramamurthy@...gle.com>
CC: <jeroendb@...gle.com>, <andrew+netdev@...n.ch>, <davem@...emloft.net>,
<edumazet@...gle.com>, <kuba@...nel.org>, <pabeni@...hat.com>,
<pkaligineedi@...gle.com>, <willemb@...gle.com>, <ziweixiao@...gle.com>,
<shailend@...gle.com>, <linux-kernel@...r.kernel.org>,
<stable@...r.kernel.org>, <netdev@...r.kernel.org>
Subject: Re: [PATCH net] gve: Add adminq lock for creating and destroying
multiple queues
On 4/17/25 22:43, Harshitha Ramamurthy wrote:
> From: Ziwei Xiao <ziweixiao@...gle.com>
>
> The original adminq lock is only protecting the gve_adminq_execute_cmd
> which is aimed for sending out single adminq command. However, there are
> other callers of gve_adminq_kick_and_wait and gve_adminq_issue_cmd that
> need to take the mutex lock for mutual exclusion between them, which are
> creating and destroying rx/tx queues. Add the adminq lock for those
> unprotected callers.
>
> Also this patch cleans up the error handling code of
> gve_adminq_destroy_tx_queue.
>
> Cc: stable@...r.kernel.org
> Fixes: 1108566ca509 ("gve: Add adminq mutex lock")
This looks like a correct fix, it is also nice that you have added
lockdep annotations.
Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@...el.com>
> Reviewed-by: Willem de Bruijn <willemb@...gle.com>
> Signed-off-by: Ziwei Xiao <ziweixiao@...gle.com>
> Signed-off-by: Harshitha Ramamurthy <hramamurthy@...gle.com>
> ---
> drivers/net/ethernet/google/gve/gve_adminq.c | 54 ++++++++++++++------
> 1 file changed, 37 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/net/ethernet/google/gve/gve_adminq.c b/drivers/net/ethernet/google/gve/gve_adminq.c
> index 3e8fc33cc11f..659460812276 100644
> --- a/drivers/net/ethernet/google/gve/gve_adminq.c
> +++ b/drivers/net/ethernet/google/gve/gve_adminq.c
> @@ -442,6 +442,8 @@ static int gve_adminq_kick_and_wait(struct gve_priv *priv)
> int tail, head;
> int i;
>
> + lockdep_assert_held(&priv->adminq_lock);
> +
> tail = ioread32be(&priv->reg_bar0->adminq_event_counter);
> head = priv->adminq_prod_cnt;
>
> @@ -467,9 +469,6 @@ static int gve_adminq_kick_and_wait(struct gve_priv *priv)
> return 0;
> }
>
> -/* This function is not threadsafe - the caller is responsible for any
> - * necessary locks.
> - */
> static int gve_adminq_issue_cmd(struct gve_priv *priv,
> union gve_adminq_command *cmd_orig)
> {
> @@ -477,6 +476,8 @@ static int gve_adminq_issue_cmd(struct gve_priv *priv,
> u32 opcode;
> u32 tail;
>
> + lockdep_assert_held(&priv->adminq_lock);
> +
> tail = ioread32be(&priv->reg_bar0->adminq_event_counter);
>
> // Check if next command will overflow the buffer.
> @@ -709,13 +710,19 @@ int gve_adminq_create_tx_queues(struct gve_priv *priv, u32 start_id, u32 num_que
> int err;
> int i;
>
> + mutex_lock(&priv->adminq_lock);
> +
> for (i = start_id; i < start_id + num_queues; i++) {
> err = gve_adminq_create_tx_queue(priv, i);
> if (err)
> - return err;
> + goto out;
> }
>
> - return gve_adminq_kick_and_wait(priv);
> + err = gve_adminq_kick_and_wait(priv);
> +
> +out:
> + mutex_unlock(&priv->adminq_lock);
> + return err;
> }
>
> static void gve_adminq_get_create_rx_queue_cmd(struct gve_priv *priv,
> @@ -788,19 +795,24 @@ int gve_adminq_create_rx_queues(struct gve_priv *priv, u32 num_queues)
> int err;
> int i;
>
> + mutex_lock(&priv->adminq_lock);
> +
> for (i = 0; i < num_queues; i++) {
> err = gve_adminq_create_rx_queue(priv, i);
> if (err)
> - return err;
> + goto out;
> }
>
> - return gve_adminq_kick_and_wait(priv);
> + err = gve_adminq_kick_and_wait(priv);
> +
> +out:
> + mutex_unlock(&priv->adminq_lock);
> + return err;
> }
>
> static int gve_adminq_destroy_tx_queue(struct gve_priv *priv, u32 queue_index)
> {
> union gve_adminq_command cmd;
> - int err;
>
> memset(&cmd, 0, sizeof(cmd));
> cmd.opcode = cpu_to_be32(GVE_ADMINQ_DESTROY_TX_QUEUE);
> @@ -808,11 +820,7 @@ static int gve_adminq_destroy_tx_queue(struct gve_priv *priv, u32 queue_index)
> .queue_id = cpu_to_be32(queue_index),
> };
>
> - err = gve_adminq_issue_cmd(priv, &cmd);
> - if (err)
> - return err;
> -
> - return 0;
> + return gve_adminq_issue_cmd(priv, &cmd);
> }
>
> int gve_adminq_destroy_tx_queues(struct gve_priv *priv, u32 start_id, u32 num_queues)
> @@ -820,13 +828,19 @@ int gve_adminq_destroy_tx_queues(struct gve_priv *priv, u32 start_id, u32 num_qu
> int err;
> int i;
>
> + mutex_lock(&priv->adminq_lock);
> +
> for (i = start_id; i < start_id + num_queues; i++) {
> err = gve_adminq_destroy_tx_queue(priv, i);
> if (err)
> - return err;
> + goto out;
> }
>
> - return gve_adminq_kick_and_wait(priv);
> + err = gve_adminq_kick_and_wait(priv);
> +
> +out:
> + mutex_unlock(&priv->adminq_lock);
> + return err;
> }
>
> static void gve_adminq_make_destroy_rx_queue_cmd(union gve_adminq_command *cmd,
> @@ -861,13 +875,19 @@ int gve_adminq_destroy_rx_queues(struct gve_priv *priv, u32 num_queues)
> int err;
> int i;
>
> + mutex_lock(&priv->adminq_lock);
> +
> for (i = 0; i < num_queues; i++) {
> err = gve_adminq_destroy_rx_queue(priv, i);
> if (err)
> - return err;
> + goto out;
> }
>
> - return gve_adminq_kick_and_wait(priv);
> + err = gve_adminq_kick_and_wait(priv);
> +
> +out:
> + mutex_unlock(&priv->adminq_lock);
> + return err;
> }
>
> static void gve_set_default_desc_cnt(struct gve_priv *priv,
Powered by blists - more mailing lists