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

[Geliang Tang <geliang@kernel.org>]

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.

Thanks,
-Geliang

> 
> > > Are you suggesting here to keep a copy in the cb, not a local
> > > one, to
> > > do
> > > the copy only once? Are you sure it is worth it (and safe)?
> > 
> > Because dumpit will be called repeatedly, we need to ensure that
> > the
> > same bitmap is accessed each time, so we cannot use a local one,
> > but
> > need to keep a copy in the cb just like what we do in
> > mptcp_userspace_pm_dump_addr.
> > 
> > 
> > In addition, when doing bitmap_copy, we need to hold pernet->lock:
> > 
> > bitmap = (struct mptcp_id_bitmap *)cb->ctx;
> > 
> > if (!id) {
> >   spin_lock_bh(&pernet->lock);
> >   bitmap_copy(id_bitmap, pernet->id_bitmap, MPTCP_PM_MAX_ADDR_ID +
> > 1);
> >   spin_unlock_bh(&pernet->lock);
> > }
> 
> Ah yes, maybe, I need to check: I thought the modification of the
> bitmap
> was done atomically, and it was fine to read without the lock. But
> maybe
> not because it is stored in multiple bytes.
> 
> > Also, we can put this bitmap copy code into a separate patch, which
> > is
> > applied before this one. In this patch we only need to replace
> > spin_lock_bh with rcu_read_lock.
> 
> OK so we are talking about separate fix that is not directly linked
> to
> my change, apart from the fact I modified code around, right?
> 
> Cheers,
> Matt