On Tue, Jun 14, 2022 at 9:09 AM Ciara Loftus <ciara.loftus@xxxxxxxxx> wrote:
>
> Two points of potential failure in the generic transmit function are:
> 1. completion queue (cq) reservation failure.
> 2. skb allocation failure
>
> Originally the cq reservation was performed first, followed by the skb
> allocation. Commit 675716400da6 ("xdp: fix possible cq entry leak")
> reversed the order because at the time there was no mechanism available to
> undo the cq reservation which could have led to possible cq entry leaks in
> the event of skb allocation failure. However if the skb allocation is
> performed first and the cq reservation then fails, the xsk skb destructor
> is called which blindly adds the skb address to the already full cq leading
> to undefined behavior.
>
> This commit restores the original order (cq reservation followed by skb
> allocation) and uses the xskq_prod_cancel helper to undo the cq reserve in
> event of skb allocation failure.
Thanks for fixing this Ciara.
Acked-by: Magnus Karlsson <magnus.karlsson@xxxxxxxxx>
> Fixes: 675716400da6 ("xdp: fix possible cq entry leak")
> Signed-off-by: Ciara Loftus <ciara.loftus@xxxxxxxxx>
> ---
> net/xdp/xsk.c | 16 +++++++++-------
> 1 file changed, 9 insertions(+), 7 deletions(-)
>
> diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
> index 19ac872a6624..09002387987e 100644
> --- a/net/xdp/xsk.c
> +++ b/net/xdp/xsk.c
> @@ -538,12 +538,6 @@ static int xsk_generic_xmit(struct sock *sk)
> goto out;
> }
>
> - skb = xsk_build_skb(xs, &desc);
> - if (IS_ERR(skb)) {
> - err = PTR_ERR(skb);
> - goto out;
> - }
> -
> /* This is the backpressure mechanism for the Tx path.
> * Reserve space in the completion queue and only proceed
> * if there is space in it. This avoids having to implement
> @@ -552,11 +546,19 @@ static int xsk_generic_xmit(struct sock *sk)
> spin_lock_irqsave(&xs->pool->cq_lock, flags);
> if (xskq_prod_reserve(xs->pool->cq)) {
> spin_unlock_irqrestore(&xs->pool->cq_lock, flags);
> - kfree_skb(skb);
> goto out;
> }
> spin_unlock_irqrestore(&xs->pool->cq_lock, flags);
>
> + skb = xsk_build_skb(xs, &desc);
> + if (IS_ERR(skb)) {
> + err = PTR_ERR(skb);
> + spin_lock_irqsave(&xs->pool->cq_lock, flags);
> + xskq_prod_cancel(xs->pool->cq);
> + spin_unlock_irqrestore(&xs->pool->cq_lock, flags);
> + goto out;
> + }
> +
> err = __dev_direct_xmit(skb, xs->queue_id);
> if (err == NETDEV_TX_BUSY) {
> /* Tell user-space to retry the send */
> --
> 2.25.1
>
