Re: [PATCH net] net: macb: fix a memory corruption in extended buffer descriptor mode

[Roman Gushchin]

On Tue, Apr 11, 2023 at 08:13:51PM -0700, Lars-Peter Clausen wrote:
> On 4/11/23 18:48, Jakub Kicinski wrote:
> > On Fri,  7 Apr 2023 10:24:02 -0700 Roman Gushchin wrote:
> > > The problem is resolved by extending the MACB_RX_WADDR_SIZE
> > > in the extended mode.
> > > 
> > > Fixes: 7b4296148066 ("net: macb: Add support for PTP timestamps in DMA descriptors")
> > > Signed-off-by: Roman Gushchin <roman.gushchin@xxxxxxxxx>
> > > Co-developed-by: Lars-Peter Clausen <lars@xxxxxxxxxx>
> > > Signed-off-by: Lars-Peter Clausen <lars@xxxxxxxxxx>
> > > ---
> > >   drivers/net/ethernet/cadence/macb.h | 5 +++++
> > >   1 file changed, 5 insertions(+)
> > > 
> > > diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
> > > index c1fc91c97cee..1b330f7cfc09 100644
> > > --- a/drivers/net/ethernet/cadence/macb.h
> > > +++ b/drivers/net/ethernet/cadence/macb.h
> > > @@ -826,8 +826,13 @@ struct macb_dma_desc_ptp {
> > >   #define MACB_RX_USED_SIZE			1
> > >   #define MACB_RX_WRAP_OFFSET			1
> > >   #define MACB_RX_WRAP_SIZE			1
> > > +#ifdef MACB_EXT_DESC
> > > +#define MACB_RX_WADDR_OFFSET			3
> > > +#define MACB_RX_WADDR_SIZE			29
> > > +#else
> > >   #define MACB_RX_WADDR_OFFSET			2
> > >   #define MACB_RX_WADDR_SIZE			30
> > > +#endif
> > Changing register definition based on Kconfig seems a bit old school.
> > 
> > Where is the extended descriptor mode enabled? Is it always on if
> > Kconfig is set or can it be off for some platforms based on other
> > capabilities? Judging by macb_dma_desc_get_size() small descriptors
> > can still be used even with EXT_DESC?
> > 
> > If I'm grepping correctly thru the painful macro magic this register
> > is only used in macb_get_addr(). It'd seem a bit more robust to me
> > to open code the extraction of the address based on bp->hw_dma_cap
> > in that one function.
> > 
> > In addition to maintainers please also CC Harini Katakam
> > <harini.katakam@xxxxxxxxxx> on v2.
> 
> We had an alternative patch which fixes this based on runtime settings. But
> it didn't seem to be worth it considering the runtime overhead, even though
> it is small. The skb buffer address is guaranteed to be cacheline aligned,
> otherwise the DMA wouldn't work at all. So we know that the LSBs must always
> be 0. We could even unconditionally define MACB_RX_WADDR_OFFSET as 3.

I'd personally prefer this.

> 
> Alternative runtime base patch:
> 
> diff --git a/drivers/net/ethernet/cadence/macb_main.c
> b/drivers/net/ethernet/cadence/macb_main.c
> index d13fb1d31821..1a40d5a26f36 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -1042,6 +1042,10 @@ static dma_addr_t macb_get_addr(struct macb *bp,
> struct macb_dma_desc *desc)
>         }
>  #endif
>         addr |= MACB_BF(RX_WADDR, MACB_BFEXT(RX_WADDR, desc->addr));
> +#ifdef CONFIG_MACB_USE_HWSTAMP
> +       if (bp->hw_dma_cap & HW_DMA_CAP_PTP)
> +               addr &= ~GEM_BIT(DMA_RXVALID_OFFSET);
> +#endif
>         return addr;
>  }
> 

I think this version is slightly worse because it adds an unconditional
if statement, which can be removed with certain config options.
I can master a version with a helper function, if it's preferable.

But if you like this one, it's fine too, let me know, I'll send an updated
version.

Thanks!