netdev - Re: [PATCH net-next 2/2] net: axienet: Use iowrite64 to write all 64b descriptor pointers

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]

Date:   Mon, 13 Jun 2022 16:18:40 +0800
From:   Andy Chiu <andy.chiu@...ive.com>
To:     radhey.shyam.pandey@...inx.com, davem@...emloft.net,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>, pabeni@...hat.com,
        michal.simek@...inx.com, netdev@...r.kernel.org
Cc:     linux-arm-kernel@...ts.infradead.org, Max Hsu <max.hsu@...ive.com>,
        Greentime Hu <greentime.hu@...ive.com>
Subject: Re: [PATCH net-next 2/2] net: axienet: Use iowrite64 to write all 64b
 descriptor pointers

Reviewed-by: Greentime Hu <greentime.hu@...ive.com>

On Mon, Jun 13, 2022 at 11:45 AM Andy Chiu <andy.chiu@...ive.com> wrote:
>
> According to commit f735c40ed93c ("net: axienet: Autodetect 64-bit DMA
> capability") and AXI-DMA spec (pg021), on 64-bit capable dma, only
> writing MSB part of tail descriptor pointer causes DMA engine to start
> fetching descriptors. However, we found that it is true only if dma is in
> idle state. In other words, dma would use a tailp even if it only has LSB
> updated, when the dma is running.
>
> The non-atomicity of this behavior could be problematic if enough
> delay were introduced in between the 2 writes. For example, if an
> interrupt comes right after the LSB write and the cpu spends long
> enough time in the handler for the dma to get back into idle state by
> completing descriptors, then the seconcd write to MSB would treat dma
> to start fetching descriptors again. Since the descriptor next to the
> one pointed by current tail pointer is not filled by the kernel yet,
> fetching a null descriptor here causes a dma internal error and halt
> the dma engine down.
>
> We suggest that the dma engine should start process a 64-bit MMIO write
> to the descriptor pointer only if ONE 32-bit part of it is written on all
> states. Or we should restrict the use of 64-bit addressable dma on 32-bit
> platforms, since those devices have no instruction to guarantee the write
> to LSB and MSB part of tail pointer occurs atomically to the dma.
>
> initial condition:
> curp =  x-3;
> tailp = x-2;
> LSB = x;
> MSB = 0;
>
> cpu:                       |dma:
>  iowrite32(LSB, tailp)     |  completes #(x-3) desc, curp = x-3
>  ...                       |  tailp updated
>  => irq                    |  completes #(x-2) desc, curp = x-2
>     ...                    |  completes #(x-1) desc, curp = x-1
>     ...                    |  ...
>     ...                    |  completes #x desc, curp = tailp = x
>  <= irqreturn              |  reaches tailp == curp = x, idle
>  iowrite32(MSB, tailp + 4) |  ...
>                            |  tailp updated, starts fetching...
>                            |  fetches #(x + 1) desc, sees cntrl = 0
>                            |  post Tx error, halt
>
> Signed-off-by: Andy Chiu <andy.chiu@...ive.com>
> Reported-by: Max Hsu <max.hsu@...ive.com>
> ---
>  drivers/net/ethernet/xilinx/xilinx_axienet.h | 21 +++++++++++++++++---
>  1 file changed, 18 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet.h b/drivers/net/ethernet/xilinx/xilinx_axienet.h
> index 6c95676ba172..97ddc0273b8a 100644
> --- a/drivers/net/ethernet/xilinx/xilinx_axienet.h
> +++ b/drivers/net/ethernet/xilinx/xilinx_axienet.h
> @@ -564,13 +564,28 @@ static inline void axienet_dma_out32(struct axienet_local *lp,
>  }
>
>  #ifdef CONFIG_64BIT
> +/**
> + * axienet_dma_out64 - Memory mapped Axi DMA register write.
> + * @lp:                Pointer to axienet local structure
> + * @reg:       Address offset from the base address of the Axi DMA core
> + * @value:     Value to be written into the Axi DMA register
> + *
> + * This function writes the desired value into the corresponding Axi DMA
> + * register.
> + */
> +static inline void axienet_dma_out64(struct axienet_local *lp,
> +                                    off_t reg, u64 value)
> +{
> +       iowrite64(value, lp->dma_regs + reg);
> +}
> +
>  static void axienet_dma_out_addr(struct axienet_local *lp, off_t reg,
>                                  dma_addr_t addr)
>  {
> -       axienet_dma_out32(lp, reg, lower_32_bits(addr));
> -
>         if (lp->features & XAE_FEATURE_DMA_64BIT)
> -               axienet_dma_out32(lp, reg + 4, upper_32_bits(addr));
> +               axienet_dma_out64(lp, reg, addr);
> +       else
> +               axienet_dma_out32(lp, reg, lower_32_bits(addr));
>  }
>
>  #else /* CONFIG_64BIT */
> --
> 2.36.0
>