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

[Lars-Peter Clausen <lars@metafoo.de>]

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@linux.dev>
>> Co-developed-by: Lars-Peter Clausen <lars@metafoo.de>
>> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
>> ---
>>   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@xilinx.com> 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.

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;
  }