Re: [PATCH mptcp-net v2 2/3] mptcp: pm: lockless list traversal

[Matthieu Baerts <matttbe@kernel.org>]

Hi Mat,

On 01/11/2024 00:24, Mat Martineau wrote:
> On Tue, 29 Oct 2024, Geliang Tang wrote:
> 
>> Hi Matt,
>>
>> On Mon, 2024-10-28 at 12:48 +0100, Matthieu Baerts wrote:
>>> Hi Geliang,
>>>
>>> On 28/10/2024 03:08, Geliang Tang wrote:
>>>> Hi Matt,
>>>>
>>>> On Fri, 2024-10-25 at 17:26 +0200, Matthieu Baerts wrote:
>>>>> Hi Geliang,
>>>>>
>>>>> Thank you for the review!
>>>>>
>>>>> On 25/10/2024 16:25, Geliang Tang wrote:
>>>>>> On Fri, 2024-10-25 at 11:32 +0200, Matthieu Baerts (NGI0)
>>>>>> wrote:
>>>>>>> In a few places -- to get an endpoint, dump all of them, and
>>>>>>> change
>>>>>>> their flags -- the list is iterated while holding the pernet-
>>>>>>>> lock,
>>>>>>> but
>>>>>>> only to read the content of the list. In these cases, we can
>>>>>>> replace
>>>>>>> the
>>>>>>> spin locks, by RCU read ones, and use the _rcu variants to
>>>>>>> iterate
>>>>>>> over
>>>>>>> the entries list in a lockless way.
>>>>>>>
>>>>>>> To make it clear, the lookup helpers using the _rcu variant
>>>>>>> are
>>>>>>> renamed
>>>>>>> with a _rcu suffix. The previous __lookup_addr() helper can
>>>>>>> then
>>>>>>> be
>>>>>>> removed, but __lookup_addr_by_id() is still needed.
>>>>>>>
>>>>>>> While at it, the IDs bitmap is copied before iterating the
>>>>>>> list
>>>>>>> to
>>>>>>> dump
>>>>>>> the different addresses, to avoid any consistencies.
>>>>>>>
>>>>>>> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
>>>>>>> ---
>>>>>>> Notes:
>>>>>>>  - This is not a fix, a small improvement for -next.
>>>>>>> ---
>>>>>>>  net/mptcp/pm_netlink.c | 36 +++++++++++++++++++-------------
>>>>>>> ----
>>>>>>>  1 file changed, 19 insertions(+), 17 deletions(-)
>>>>>>>
>>>>>>> diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
>>>>>>> index
>>>>>>> a93b9b7776b48781a883673fe5fd521a978487ff..f38e1ccd34e95cd88b1
>>>>>>> 79a8
>>>>>>> b50e
>>>>>>> 6965731542871 100644
>>>>>>> --- a/net/mptcp/pm_netlink.c
>>>>>>> +++ b/net/mptcp/pm_netlink.c
>>>>>
>>>>> (...)
>>>>>
>>>>>>> @@ -1872,16 +1872,18 @@ int mptcp_pm_nl_dump_addr(struct
>>>>>>> sk_buff
>>>>>>> *msg,
>>>>>>>   struct net *net = sock_net(msg->sk);
>>>>>>>   struct mptcp_pm_addr_entry *entry;
>>>>>>>   struct pm_nl_pernet *pernet;
>>>>>>> + unsigned long id_bitmap[4];
>>>>>>>   int id = cb->args[0];
>>>>>>>   void *hdr;
>>>>>>>   int i;
>>>>>>>
>>>>>>   pernet = pm_nl_get_pernet(net);
>>>>>>> + bitmap_copy(id_bitmap, pernet->id_bitmap,
>>>>>>> MPTCP_PM_MAX_ADDR_ID + 1);
>>>>>>
>>>>>> I think the id bitmap should only be copied when id is 0:
>>>>>>
>>>>>> if (!id)
>>>>>>   bitmap_copy(id_bitmap, pernet->id_bitmap,
>>>>>> MPTCP_PM_MAX_ADDR_ID +
>>>>>> 1);
>>>>>>
>>>>>> Since this mptcp_pm_nl_dump_addr() may be called repeatedly
>>>>>> when
>>>>>> the
>>>>>> dump information is very long. We only copy it the first time
>>>>>> it is
>>>>
>>>> A correction is needed here. Regardless of whether the dump
>>>> information
>>>> is large or small, the dumpit function will be called repeatedly
>>>> when
>>>> dumpit returns non-zero. The loop stops when dumpit returns 0.
>>>>
>>>>>> called, and subsequent calls cannot copy it again. WDYT?
>>>>>
>>>>> Sorry, I'm not sure to understand. Here, I did a local copy of
>>>>> 'id_bitmap' just to keep a certain consistency if the bitmap is
>>>>> changed
>>>>> during the RCU read section below: not to have this bitmap being
>>>>> changed
>>>>> during the for loop here below. Before, we had this protection
>>>>> because
>>>>> pernet->lock was held.
>>>>
>>>> It is precisely because we need to maintain this consistency that
>>>> we
>>>> need to copy the bitmap only once. If we copy the bitmap when
>>>> dumpit is
>>>> called a second time, the bitmap obtained at this time is a new
>>>> bitmap,
>>>> which destroys this consistency, right?
>>>
>>> OK, but then I have a few questions:
>>>
>>> - Was the code before my patch here prevented such consistency
>>> issues?
>>> Or said differently: is there a regression introduced by this patch?
>>>
>>> - Is it really an issue that can lead to a crash or reading freed
>>> info?
>>>
>>> - Where would you store the copy of the bitmap? In cb-args?
>>>
>>> - If we store this copy somewhere, could you not have situations
>>> where
>>> the bitmap would no longer be in sync with the addresses that are
>>> stored
>>> in 'pernet->local_addr_list', and then displaying wrong/freed info?
>>> It
>>> looks like more would be needed to cope with that. And maybe that's
>>> not
>>> worth it?
>>
>> Your doubts are very reasonable. It seems that I have not considered it
>> carefully enough. So we do need to define a local bitmap and copy it
>> every time. It's fine to me and it's OK to implement BPF path manager
>> based on it.
>>
> 
> Matthieu and Geliang -
> 
> I think the only way to guarantee strict consistency would be to acquire
> the lock at the beginning of the dump and make a complete copy of both
> the bitmap and the address list at that moment. Even with all the work
> to achieve this "strictly consistent" snapshot, the userspace caller
> doesn't have any guarantee that the dumped data is accurate at the
> moment the netlink response completes.
> 
> 
> The good news is that I don't think this strict consistency is needed
> for the dump function. If the list is being modified concurrently, it's
> not a big problem to miss newly-added entries or show recently-deleted
> ones.
> 
> 
> Since bitmap_copy() is non-atomic, my suggestion is to not copy the
> bitmap. Instead use test_bit() (which is atomic) directly on pernet-
>>id_bitmap without locking.
> 
> Then, make sure that inconsistencies between the bitmap and the
> local_addr_list are handled cleanly. The two cases are:
> 
> 1. Bitmap has a 0 even though there's a new entry in local_addr_list. In
> this case it gets skipped. No problem here.
> 
> 2. Bitmap has a 1 even though there's no entry in local_addr_list
> (ignore it!), or there's an entry that is in the process of getting
> freed. For the latter, RCU will ensure there's no risk of an invalid
> dereference. If we want to detect a deletion-in-progress, we would have
> to add an atomic flag to 'struct mptcp_pm_addr_entry' that is set before
> the deletion code calls list_del_rcu(). Even if the address is in this
> state, I would argue that the just-barely-stale data is fine to show.

Thank you for your review!

> Sound reasonable? Do you see a reason that bitmap/addr_list
> inconsistencies would cause issues?
Yes, that's sounds reasonable, as long as we don't try to access freed
resources, I don't think we need to worry about inconsistencies if there
are ongoing changes while the userspace is requesting the list.

When I did the copy of the bitmap, it was mainly "as a matter of
principle", but if it is not needed, that's even simpler.

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.