Re: [PATCH v2 net] net: mvpp2: Prevent parser TCAM memory corruption

[Tobias Waldekranz <tobias@waldekranz.com>]

On fre, mar 21, 2025 at 14:18, Andrew Lunn <andrew@lunn.ch> wrote:
> On Fri, Mar 21, 2025 at 01:41:38PM +0100, Tobias Waldekranz wrote:
>> On fre, mar 21, 2025 at 13:12, Andrew Lunn <andrew@lunn.ch> wrote:
>> >> +static int mvpp2_prs_init_from_hw_unlocked(struct mvpp2 *priv,
>> >> +					   struct mvpp2_prs_entry *pe, int tid)
>> >>  {
>> >>  	int i;
>> >>  
>> >
>> > This is called from quite a few places, and the locking is not always
>> > obvious. Maybe add
>> 
>> Agreed, that was why i chose the _unlocked suffix vs. just prefixing
>> with _ or something. For sure I can add it, I just want to run something
>> by you first:
>> 
>> Originally, my idea was to just protect mvpp2_prs_init_from_hw() and
>> mvpp2_prs_hw_write(). Then I realized that the software shadow of the
>> SRAM table must also be protected, which is why locking had to be
>> hoisted up to the current scope.
>> 
>> > __must_hold(&priv->prs_spinlock)
>> >
>> > so sparse can verify the call paths ?
>> 
>> So if we add these asserts only to the hardware access leaf functions,
>> do we risk inadvertently signaling to future readers that the lock is
>> only there to protect the hardware tables?
>
> You can scatter __must_hold() anywhere you want, to indicate the lock
> must be held. It has no runtime overhead.
>
> And you can expand the comment where the mutex is defined to say what
> it is expected to cover.

Yes, I will definitely do that in v3.

> FYI: i've never personally used __must_hold(), but i reviewed a patch
> recently using it, which made me think it might be useful here. I
> don't know if you need additional markup, __acquires() & __releases()
> ?? You might want to deliberately break the locking and see if sparse
> reports it.

I have added __must_hold() now, but have thus far not been able to
provoke a sparse warning with it.

From what I understand __acquires()/__releases() is only needed when you
have "unbalanced" functions, to let sparse know that a function is
supposed to only either lock or unlock a particular resource - so I do
not think that is what I am missing.

If I remove the unlock from the early exit of mvpp2_prs_vid_entry_add(),
it does report the following...

drivers/net/ethernet/marvell/mvpp2/mvpp2_prs.c:1980:5: warning: context
imbalance in 'mvpp2_prs_vid_entry_add' - wrong count at exit

...which leads me to believe that the sparse's -Wcontext is active
(which is what I expected, based on the documentation in sparse(1))

I also tried removing some locking in the mt7530 driver, which also
makes use of __must_hold(), which did not trigger any sparse warnings
either.

I am not sure how to proceed. The documentation around how to use these
attributes is quite... sparse :)