[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <87zfhg9dww.fsf@waldekranz.com>
Date: Thu, 20 Mar 2025 11:47:43 +0100
From: Tobias Waldekranz <tobias@...dekranz.com>
To: Maxime Chevallier <maxime.chevallier@...tlin.com>
Cc: davem@...emloft.net, kuba@...nel.org, marcin.s.wojtas@...il.com,
linux@...linux.org.uk, andrew@...n.ch, edumazet@...gle.com,
pabeni@...hat.com, ezequiel.garcia@...e-electrons.com,
netdev@...r.kernel.org
Subject: Re: [PATCH net] net: mvpp2: Prevent parser TCAM memory corruption
On tor, mar 20, 2025 at 10:57, Maxime Chevallier <maxime.chevallier@...tlin.com> wrote:
> Hi Tobias,
>
> On Thu, 20 Mar 2025 10:17:00 +0100
> Tobias Waldekranz <tobias@...dekranz.com> wrote:
>
>> Protect the parser TCAM/SRAM memory, and the cached (shadow) SRAM
>> information, from concurrent modifications.
>>
>> Both the TCAM and SRAM tables are indirectly accessed by configuring
>> an index register that selects the row to read or write to. This means
>> that operations must be atomic in order to, e.g., avoid spreading
>> writes across multiple rows. Since the shadow SRAM array is used to
>> find free rows in the hardware table, it must also be protected in
>> order to avoid TOCTOU errors where multiple cores allocate the same
>> row.
>>
>> This issue was detected in a situation where `mvpp2_set_rx_mode()` ran
>> concurrently on two CPUs. In this particular case the
>> MVPP2_PE_MAC_UC_PROMISCUOUS entry was corrupted, causing the
>> classifier unit to drop all incoming unicast - indicated by the
>> `rx_classifier_drops` counter.
>>
>> Fixes: 3f518509dedc ("ethernet: Add new driver for Marvell Armada 375 network unit")
>> Signed-off-by: Tobias Waldekranz <tobias@...dekranz.com>
>> ---
>
> [...]
>
>> +int mvpp2_prs_init_from_hw(struct mvpp2 *priv, struct mvpp2_prs_entry *pe,
>> + int tid)
>> +{
>> + unsigned long flags;
>> + int err;
>> +
>> + spin_lock_irqsave(&priv->prs_spinlock, flags);
>> + err = mvpp2_prs_init_from_hw_unlocked(priv, pe, tid);
>> + spin_unlock_irqrestore(&priv->prs_spinlock, flags);
>
> That's indeed an issue, I'm wondering however if you really need to
> irqsave/irqrestore everytime you protect the accesses to the Parser.
>
> From what I remember we don't touch the Parser in the interrupt path,
> it's mostly a consequence to netdev ops being called (promisc, vlan
> add/kill, mc/uc filtering and a lot in the init path).
Good point! Indeed, I can not find any access to the parser in IRQ
context.
We still need to disable bottom halves though, right? Because otherwise
we could reach mvpp2_set_rx_mode() from net-rx by processing an IGMP/MLD
frame, for example.
Powered by blists - more mailing lists