Re: [MPTCP][PATCH v7 mptcp-next 1/6] mptcp: drop flags and ifindex arguments

[Geliang Tang <geliangtang@gmail.com>]

Paolo Abeni <pabeni@redhat.com> 于2021年7月29日周四 下午7:12写道：
>
> On Thu, 2021-07-29 at 15:20 +0800, Geliang Tang wrote:
> > From: Geliang Tang <geliangtang@xiaomi.com>
> >
> > This patch added a new helper mptcp_pm_get_flags_and_ifindex_by_id(),
> > and used it in __mptcp_subflow_connect() to get the flags and ifindex
> > values.
> >
> > Then the two arguments flags and ifindex of __mptcp_subflow_connect()
> > can be dropped.
> >
> > Signed-off-by: Geliang Tang <geliangtang@xiaomi.com>
> > ---
> >  net/mptcp/pm_netlink.c | 25 ++++++++++++++++++++++---
> >  net/mptcp/protocol.h   |  5 +++--
> >  net/mptcp/subflow.c    |  7 +++++--
> >  3 files changed, 30 insertions(+), 7 deletions(-)
> >
> > diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
> > index ba0e1d71504d..94c68d6093de 100644
> > --- a/net/mptcp/pm_netlink.c
> > +++ b/net/mptcp/pm_netlink.c
> > @@ -462,8 +462,7 @@ static void mptcp_pm_create_subflow_or_signal_addr(struct mptcp_sock *msk)
> >                       check_work_pending(msk);
> >                       remote_address((struct sock_common *)sk, &remote);
> >                       spin_unlock_bh(&msk->pm.lock);
> > -                     __mptcp_subflow_connect(sk, &local->addr, &remote,
> > -                                             local->flags, local->ifindex);
> > +                     __mptcp_subflow_connect(sk, &local->addr, &remote);
> >                       spin_lock_bh(&msk->pm.lock);
> >                       return;
> >               }
> > @@ -518,7 +517,7 @@ static void mptcp_pm_nl_add_addr_received(struct mptcp_sock *msk)
> >       local.family = remote.family;
> >
> >       spin_unlock_bh(&msk->pm.lock);
> > -     __mptcp_subflow_connect(sk, &local, &remote, 0, 0);
> > +     __mptcp_subflow_connect(sk, &local, &remote);
> >       spin_lock_bh(&msk->pm.lock);
> >
> >  add_addr_echo:
> > @@ -1103,6 +1102,26 @@ __lookup_addr_by_id(struct pm_nl_pernet *pernet, unsigned int id)
> >       return NULL;
> >  }
> >
> > +int mptcp_pm_get_flags_and_ifindex_by_id(struct net *net, unsigned int id,
> > +                                      u8 *flags, int *ifindex)
> > +{
> > +     struct mptcp_pm_addr_entry *entry;
> > +
> > +     rcu_read_lock();
> > +     entry = __lookup_addr_by_id(net_generic(net, pm_nl_pernet_id), id);
> > +     rcu_read_unlock();
> > +
> > +     if (entry) {
> > +             *flags = entry->flags;
> > +             *ifindex = entry->ifindex;
> > +     } else {
> > +             *flags = 0;
> > +             *ifindex = 0;
> > +     }
>
> You need to extend the RCU section after the entry reference above, or
> on preempt-enabled build, the entry could be deleted in between the rcu
>  unlock and the later access.

Thanks Paolo, I'll send a squash-to patch to fix this.

>
> I'm wondering if there are issues with the 0 id ?!?

The 0 id address isn't on the local_addr_list, we should set it's
flags and ifindex all to 0.

I'll add a non-zero check in the squash-to patch too.

Thanks,
-Geliang

>
> /P
>