From: Joshua Washington joshwash@google.com
In GVE, dedicated XDP queues only exist when an XDP program is installed and the interface is up. As such, the NDO XDP XMIT callback should return early if either of these conditions are false.
In the case of no loaded XDP program, priv->num_xdp_queues=0 which can cause a divide-by-zero error, and in the case of interface down, num_xdp_queues remains untouched to persist XDP queue count for the next interface up, but the TX pointer itself would be NULL.
The XDP xmit callback also needs to synchronize with a device transitioning from open to close. This synchronization will happen via the GVE_PRIV_FLAGS_NAPI_ENABLED bit along with a synchronize_net() call, which waits for any RCU critical sections at call-time to complete.
Fixes: 39a7f4aa3e4a ("gve: Add XDP REDIRECT support for GQI-QPL format") Cc: stable@vger.kernel.org Signed-off-by: Joshua Washington joshwash@google.com Signed-off-by: Praveen Kaligineedi pkaligineedi@google.com Reviewed-by: Praveen Kaligineedi pkaligineedi@google.com Reviewed-by: Shailend Chand shailend@google.com Reviewed-by: Willem de Bruijn willemb@google.com --- drivers/net/ethernet/google/gve/gve_main.c | 3 +++ drivers/net/ethernet/google/gve/gve_tx.c | 5 ++++- 2 files changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/google/gve/gve_main.c b/drivers/net/ethernet/google/gve/gve_main.c index e171ca248f9a..5d7b0cc59959 100644 --- a/drivers/net/ethernet/google/gve/gve_main.c +++ b/drivers/net/ethernet/google/gve/gve_main.c @@ -1899,6 +1899,9 @@ static void gve_turndown(struct gve_priv *priv)
gve_clear_napi_enabled(priv); gve_clear_report_stats(priv); + + /* Make sure that all traffic is finished processing. */ + synchronize_net(); }
static void gve_turnup(struct gve_priv *priv) diff --git a/drivers/net/ethernet/google/gve/gve_tx.c b/drivers/net/ethernet/google/gve/gve_tx.c index 83ad278ec91f..852f8c7e39d2 100644 --- a/drivers/net/ethernet/google/gve/gve_tx.c +++ b/drivers/net/ethernet/google/gve/gve_tx.c @@ -837,9 +837,12 @@ int gve_xdp_xmit(struct net_device *dev, int n, struct xdp_frame **frames, struct gve_tx_ring *tx; int i, err = 0, qid;
- if (unlikely(flags & ~XDP_XMIT_FLAGS_MASK)) + if (unlikely(flags & ~XDP_XMIT_FLAGS_MASK) || !priv->xdp_prog) return -EINVAL;
+ if (!gve_get_napi_enabled(priv)) + return -ENETDOWN; + qid = gve_xdp_tx_queue_id(priv, smp_processor_id() % priv->num_xdp_queues);
From: Praveen Kaligineedi pkaligineedi@google.com Date: Wed, 18 Dec 2024 05:34:12 -0800
From: Joshua Washington joshwash@google.com
In GVE, dedicated XDP queues only exist when an XDP program is installed and the interface is up. As such, the NDO XDP XMIT callback should return early if either of these conditions are false.
In the case of no loaded XDP program, priv->num_xdp_queues=0 which can cause a divide-by-zero error, and in the case of interface down, num_xdp_queues remains untouched to persist XDP queue count for the next interface up, but the TX pointer itself would be NULL.
The XDP xmit callback also needs to synchronize with a device transitioning from open to close. This synchronization will happen via the GVE_PRIV_FLAGS_NAPI_ENABLED bit along with a synchronize_net() call, which waits for any RCU critical sections at call-time to complete.
Fixes: 39a7f4aa3e4a ("gve: Add XDP REDIRECT support for GQI-QPL format") Cc: stable@vger.kernel.org Signed-off-by: Joshua Washington joshwash@google.com Signed-off-by: Praveen Kaligineedi pkaligineedi@google.com Reviewed-by: Praveen Kaligineedi pkaligineedi@google.com Reviewed-by: Shailend Chand shailend@google.com Reviewed-by: Willem de Bruijn willemb@google.com
drivers/net/ethernet/google/gve/gve_main.c | 3 +++ drivers/net/ethernet/google/gve/gve_tx.c | 5 ++++- 2 files changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/google/gve/gve_main.c b/drivers/net/ethernet/google/gve/gve_main.c index e171ca248f9a..5d7b0cc59959 100644 --- a/drivers/net/ethernet/google/gve/gve_main.c +++ b/drivers/net/ethernet/google/gve/gve_main.c @@ -1899,6 +1899,9 @@ static void gve_turndown(struct gve_priv *priv) gve_clear_napi_enabled(priv); gve_clear_report_stats(priv);
- /* Make sure that all traffic is finished processing. */
- synchronize_net();
Wouldn't synchronize_rcu() be enough, have you checked?
} static void gve_turnup(struct gve_priv *priv) diff --git a/drivers/net/ethernet/google/gve/gve_tx.c b/drivers/net/ethernet/google/gve/gve_tx.c index 83ad278ec91f..852f8c7e39d2 100644 --- a/drivers/net/ethernet/google/gve/gve_tx.c +++ b/drivers/net/ethernet/google/gve/gve_tx.c @@ -837,9 +837,12 @@ int gve_xdp_xmit(struct net_device *dev, int n, struct xdp_frame **frames, struct gve_tx_ring *tx; int i, err = 0, qid;
- if (unlikely(flags & ~XDP_XMIT_FLAGS_MASK))
- if (unlikely(flags & ~XDP_XMIT_FLAGS_MASK) || !priv->xdp_prog) return -EINVAL;
The first condition (weird xmit flags) is certainly EINVAL. Lack of installed XDP prog is *not*.
You need to use xdp_features_{set,clear}_redirect_target() when you install/remove XDP prog to notify the kernel that ndo_start_xmit is now available / not available anymore.
If you want to leave this check, I'd suggest changing it to
if (unlikely(!priv->num_xdp_queues)) return -ENXIO;
if (unlikely(flags & ~XDP_XMIT_FLAGS_MASK)) return -EINVAL;
- if (!gve_get_napi_enabled(priv))
return -ENETDOWN;
- qid = gve_xdp_tx_queue_id(priv, smp_processor_id() % priv->num_xdp_queues);
Thanks, Olek
Wouldn't synchronize_rcu() be enough, have you checked?
I based usage of synchronize_net() instead of synchronize_rcu() based on other drivers deciding to use it due to synchronize_rcu_expedited() when holding rtnl_lock() being more performant.
ICE: https://lore.kernel.org/all/20240529112337.3639084-4-maciej.fijalkowski@inte... Mellanox: https://lore.kernel.org/netdev/20210212025641.323844-8-saeed@kernel.org/
You need to use xdp_features_{set,clear}_redirect_target() when you
install/remove XDP prog to notify the kernel that ndo_start_xmit is now available / not available anymore.
Thank you for the suggestion. Given that the fix has gone in, I was planning to make this change as part of a future net-next release with other XDP changes. Would it make sense to make those changes there, given that the patches as they went up, while not completely correct, should at least cover the vulnerability?
Thanks, Josh
linux-stable-mirror@lists.linaro.org