| 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: <CANLc=asOJE-pGV74hXaZT5C73gVvbmDC1Zr6F4wJ31cqLFqcFg@mail.gmail.com>
Date: Fri, 19 Apr 2024 15:23:33 -0700
From: Shailend Chand <shailend@...gle.com>
To: netdev@...r.kernel.org
Cc: almasrymina@...gle.com, davem@...emloft.net, edumazet@...gle.com,
kuba@...nel.org, pabeni@...hat.com, willemb@...gle.com
Subject: Re: [RFC PATCH net-next 9/9] gve: Implement queue api
On Thu, Apr 18, 2024 at 12:52 PM Shailend Chand <shailend@...gle.com> wrote:
>
> An api enabling the net stack to reset driver queues is implemented for
> gve.
>
> Signed-off-by: Shailend Chand <shailend@...gle.com>
> ---
> drivers/net/ethernet/google/gve/gve.h | 6 +
> drivers/net/ethernet/google/gve/gve_dqo.h | 6 +
> drivers/net/ethernet/google/gve/gve_main.c | 143 +++++++++++++++++++
> drivers/net/ethernet/google/gve/gve_rx.c | 12 +-
> drivers/net/ethernet/google/gve/gve_rx_dqo.c | 12 +-
> 5 files changed, 167 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/net/ethernet/google/gve/gve.h b/drivers/net/ethernet/google/gve/gve.h
> index 9f6a897c87cb..d752e525bde7 100644
> --- a/drivers/net/ethernet/google/gve/gve.h
> +++ b/drivers/net/ethernet/google/gve/gve.h
> @@ -1147,6 +1147,12 @@ bool gve_tx_clean_pending(struct gve_priv *priv, struct gve_tx_ring *tx);
> void gve_rx_write_doorbell(struct gve_priv *priv, struct gve_rx_ring *rx);
> int gve_rx_poll(struct gve_notify_block *block, int budget);
> bool gve_rx_work_pending(struct gve_rx_ring *rx);
> +int gve_rx_alloc_ring_gqi(struct gve_priv *priv,
> + struct gve_rx_alloc_rings_cfg *cfg,
> + struct gve_rx_ring *rx,
> + int idx);
> +void gve_rx_free_ring_gqi(struct gve_priv *priv, struct gve_rx_ring *rx,
> + struct gve_rx_alloc_rings_cfg *cfg);
> int gve_rx_alloc_rings(struct gve_priv *priv);
> int gve_rx_alloc_rings_gqi(struct gve_priv *priv,
> struct gve_rx_alloc_rings_cfg *cfg);
> diff --git a/drivers/net/ethernet/google/gve/gve_dqo.h b/drivers/net/ethernet/google/gve/gve_dqo.h
> index b81584829c40..e83773fb891f 100644
> --- a/drivers/net/ethernet/google/gve/gve_dqo.h
> +++ b/drivers/net/ethernet/google/gve/gve_dqo.h
> @@ -44,6 +44,12 @@ void gve_tx_free_rings_dqo(struct gve_priv *priv,
> struct gve_tx_alloc_rings_cfg *cfg);
> void gve_tx_start_ring_dqo(struct gve_priv *priv, int idx);
> void gve_tx_stop_ring_dqo(struct gve_priv *priv, int idx);
> +int gve_rx_alloc_ring_dqo(struct gve_priv *priv,
> + struct gve_rx_alloc_rings_cfg *cfg,
> + struct gve_rx_ring *rx,
> + int idx);
> +void gve_rx_free_ring_dqo(struct gve_priv *priv, struct gve_rx_ring *rx,
> + struct gve_rx_alloc_rings_cfg *cfg);
> int gve_rx_alloc_rings_dqo(struct gve_priv *priv,
> struct gve_rx_alloc_rings_cfg *cfg);
> void gve_rx_free_rings_dqo(struct gve_priv *priv,
> diff --git a/drivers/net/ethernet/google/gve/gve_main.c b/drivers/net/ethernet/google/gve/gve_main.c
> index c348dff7cca6..5e652958f10f 100644
> --- a/drivers/net/ethernet/google/gve/gve_main.c
> +++ b/drivers/net/ethernet/google/gve/gve_main.c
> @@ -17,6 +17,7 @@
> #include <linux/workqueue.h>
> #include <linux/utsname.h>
> #include <linux/version.h>
> +#include <net/netdev_queues.h>
> #include <net/sch_generic.h>
> #include <net/xdp_sock_drv.h>
> #include "gve.h"
> @@ -2070,6 +2071,15 @@ static void gve_turnup(struct gve_priv *priv)
> gve_set_napi_enabled(priv);
> }
>
> +static void gve_turnup_and_check_status(struct gve_priv *priv)
> +{
> + u32 status;
> +
> + gve_turnup(priv);
> + status = ioread32be(&priv->reg_bar0->device_status);
> + gve_handle_link_status(priv, GVE_DEVICE_STATUS_LINK_STATUS_MASK & status);
> +}
> +
> static void gve_tx_timeout(struct net_device *dev, unsigned int txqueue)
> {
> struct gve_notify_block *block;
> @@ -2530,6 +2540,138 @@ static void gve_write_version(u8 __iomem *driver_version_register)
> writeb('\n', driver_version_register);
> }
>
> +static int gve_rx_queue_stop(struct net_device *dev, int idx,
> + void **out_per_q_mem)
> +{
> + struct gve_priv *priv = netdev_priv(dev);
> + struct gve_rx_ring *rx;
> + int err;
> +
> + if (!priv->rx)
> + return -EAGAIN;
> + if (idx < 0 || idx >= priv->rx_cfg.max_queues)
> + return -ERANGE;
> +
> + /* Destroying queue 0 while other queues exist is not supported in DQO */
> + if (!gve_is_gqi(priv) && idx == 0)
> + return -ERANGE;
> +
> + rx = kvzalloc(sizeof(*rx), GFP_KERNEL);
> + if (!rx)
> + return -ENOMEM;
> + *rx = priv->rx[idx];
> +
> + /* Single-queue destruction requires quiescence on all queues */
> + gve_turndown(priv);
> +
> + /* This failure will trigger a reset - no need to clean up */
> + err = gve_adminq_destroy_single_rx_queue(priv, idx);
> + if (err) {
> + kvfree(rx);
> + return err;
> + }
> +
> + if (gve_is_gqi(priv))
> + gve_rx_stop_ring_gqi(priv, idx);
> + else
> + gve_rx_stop_ring_dqo(priv, idx);
> +
> + /* Turn the unstopped queues back up */
> + gve_turnup_and_check_status(priv);
> +
> + *out_per_q_mem = rx;
> + return 0;
> +}
> +
> +static void gve_rx_queue_mem_free(struct net_device *dev, void *per_q_mem)
> +{
> + struct gve_priv *priv = netdev_priv(dev);
> + struct gve_rx_alloc_rings_cfg cfg = {0};
> + struct gve_rx_ring *rx;
> +
> + gve_rx_get_curr_alloc_cfg(priv, &cfg);
> + rx = (struct gve_rx_ring *)per_q_mem;
> + if (!rx)
> + return;
> +
> + if (gve_is_gqi(priv))
> + gve_rx_free_ring_gqi(priv, rx, &cfg);
> + else
> + gve_rx_free_ring_dqo(priv, rx, &cfg);
> +
> + kvfree(per_q_mem);
> +}
> +
> +static void *gve_rx_queue_mem_alloc(struct net_device *dev, int idx)
> +{
> + struct gve_priv *priv = netdev_priv(dev);
> + struct gve_rx_alloc_rings_cfg cfg = {0};
> + struct gve_rx_ring *rx;
> + int err;
> +
> + gve_rx_get_curr_alloc_cfg(priv, &cfg);
> + if (idx < 0 || idx >= cfg.qcfg->max_queues)
> + return NULL;
> +
> + rx = kvzalloc(sizeof(*rx), GFP_KERNEL);
> + if (!rx)
> + return NULL;
> +
> + if (gve_is_gqi(priv))
> + err = gve_rx_alloc_ring_gqi(priv, &cfg, rx, idx);
> + else
> + err = gve_rx_alloc_ring_dqo(priv, &cfg, rx, idx);
> +
> + if (err) {
> + kvfree(rx);
> + return NULL;
> + }
> + return rx;
> +}
> +
> +static int gve_rx_queue_start(struct net_device *dev, int idx, void *per_q_mem)
> +{
> + struct gve_priv *priv = netdev_priv(dev);
> + struct gve_rx_ring *rx;
> + int err;
> +
> + if (!priv->rx)
> + return -EAGAIN;
> + if (idx < 0 || idx >= priv->rx_cfg.max_queues)
> + return -ERANGE;
> + rx = (struct gve_rx_ring *)per_q_mem;
> + priv->rx[idx] = *rx;
> +
> + /* Single-queue creation requires quiescence on all queues */
> + gve_turndown(priv);
> +
> + if (gve_is_gqi(priv))
> + gve_rx_start_ring_gqi(priv, idx);
> + else
> + gve_rx_start_ring_dqo(priv, idx);
> +
> + /* This failure will trigger a reset - no need to clean up */
> + err = gve_adminq_create_single_rx_queue(priv, idx);
> + if (err)
> + return err;
> +
> + if (gve_is_gqi(priv))
> + gve_rx_write_doorbell(priv, &priv->rx[idx]);
> + else
> + gve_rx_post_buffers_dqo(&priv->rx[idx]);
> +
> + /* Turn the unstopped queues back up */
> + gve_turnup_and_check_status(priv);
> + return 0;
> +}
I realized that due to not kvfree-ing the passed-in `per_q_mem`, there
is a leak. The alloc and stop hooks kvzalloc
a temp ring struct, which means the start and free hooks ought to have
kvfreed them to keep symmetry and avoid leaking.
The free hook is doing it but I forgot to do it in the start hook.
If we go down the route of making core aware of the ring struct size,
then none of the four hooks
need to worry about the temp struct: core can just alloc and free it
for both the old and new queue.
> +
> +static const struct netdev_queue_mgmt_ops gve_queue_mgmt_ops = {
> + .ndo_queue_mem_alloc = gve_rx_queue_mem_alloc,
> + .ndo_queue_mem_free = gve_rx_queue_mem_free,
> + .ndo_queue_start = gve_rx_queue_start,
> + .ndo_queue_stop = gve_rx_queue_stop,
> +};
> +
> static int gve_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> {
> int max_tx_queues, max_rx_queues;
> @@ -2584,6 +2726,7 @@ static int gve_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> pci_set_drvdata(pdev, dev);
> dev->ethtool_ops = &gve_ethtool_ops;
> dev->netdev_ops = &gve_netdev_ops;
> + dev->queue_mgmt_ops = &gve_queue_mgmt_ops;
>
> /* Set default and supported features.
> *
> diff --git a/drivers/net/ethernet/google/gve/gve_rx.c b/drivers/net/ethernet/google/gve/gve_rx.c
> index 1d235caab4c5..307bf97d4778 100644
> --- a/drivers/net/ethernet/google/gve/gve_rx.c
> +++ b/drivers/net/ethernet/google/gve/gve_rx.c
> @@ -101,8 +101,8 @@ void gve_rx_stop_ring_gqi(struct gve_priv *priv, int idx)
> gve_rx_reset_ring_gqi(priv, idx);
> }
>
> -static void gve_rx_free_ring_gqi(struct gve_priv *priv, struct gve_rx_ring *rx,
> - struct gve_rx_alloc_rings_cfg *cfg)
> +void gve_rx_free_ring_gqi(struct gve_priv *priv, struct gve_rx_ring *rx,
> + struct gve_rx_alloc_rings_cfg *cfg)
> {
> struct device *dev = &priv->pdev->dev;
> u32 slots = rx->mask + 1;
> @@ -270,10 +270,10 @@ void gve_rx_start_ring_gqi(struct gve_priv *priv, int idx)
> gve_add_napi(priv, ntfy_idx, gve_napi_poll);
> }
>
> -static int gve_rx_alloc_ring_gqi(struct gve_priv *priv,
> - struct gve_rx_alloc_rings_cfg *cfg,
> - struct gve_rx_ring *rx,
> - int idx)
> +int gve_rx_alloc_ring_gqi(struct gve_priv *priv,
> + struct gve_rx_alloc_rings_cfg *cfg,
> + struct gve_rx_ring *rx,
> + int idx)
> {
> struct device *hdev = &priv->pdev->dev;
> u32 slots = cfg->ring_size;
> diff --git a/drivers/net/ethernet/google/gve/gve_rx_dqo.c b/drivers/net/ethernet/google/gve/gve_rx_dqo.c
> index dc2c6bd92e82..dcbc37118870 100644
> --- a/drivers/net/ethernet/google/gve/gve_rx_dqo.c
> +++ b/drivers/net/ethernet/google/gve/gve_rx_dqo.c
> @@ -299,8 +299,8 @@ void gve_rx_stop_ring_dqo(struct gve_priv *priv, int idx)
> gve_rx_reset_ring_dqo(priv, idx);
> }
>
> -static void gve_rx_free_ring_dqo(struct gve_priv *priv, struct gve_rx_ring *rx,
> - struct gve_rx_alloc_rings_cfg *cfg)
> +void gve_rx_free_ring_dqo(struct gve_priv *priv, struct gve_rx_ring *rx,
> + struct gve_rx_alloc_rings_cfg *cfg)
> {
> struct device *hdev = &priv->pdev->dev;
> size_t completion_queue_slots;
> @@ -373,10 +373,10 @@ void gve_rx_start_ring_dqo(struct gve_priv *priv, int idx)
> gve_add_napi(priv, ntfy_idx, gve_napi_poll_dqo);
> }
>
> -static int gve_rx_alloc_ring_dqo(struct gve_priv *priv,
> - struct gve_rx_alloc_rings_cfg *cfg,
> - struct gve_rx_ring *rx,
> - int idx)
> +int gve_rx_alloc_ring_dqo(struct gve_priv *priv,
> + struct gve_rx_alloc_rings_cfg *cfg,
> + struct gve_rx_ring *rx,
> + int idx)
> {
> struct device *hdev = &priv->pdev->dev;
> size_t size;
> --
> 2.44.0.769.g3c40516874-goog
>
Powered by blists - more mailing lists