netdev - Re: [PATCH net-next 8/8] mld: change context of mld module

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Tue, 9 Feb 2021 19:00:31 +0900
From:   Taehee Yoo <ap420073@...il.com>
To:     Julian Wiedmann <jwi@...ux.ibm.com>
Cc:     David Miller <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Netdev <netdev@...r.kernel.org>, dsahern@...nel.org,
        Cong Wang <xiyou.wangcong@...il.com>, kgraul@...ux.ibm.com,
        hca@...ux.ibm.com, gor@...ux.ibm.com, borntraeger@...ibm.com,
        mareklindner@...mailbox.ch, sw@...onwunderlich.de, a@...table.cc,
        Sven Eckelmann <sven@...fation.org>, yoshfuji@...ux-ipv6.org
Subject: Re: [PATCH net-next 8/8] mld: change context of mld module

On Tue, 9 Feb 2021 at 17:26, Julian Wiedmann <jwi@...ux.ibm.com> wrote:
>

Hi Julian,
Thank you for the review!

> On 08.02.21 18:59, Taehee Yoo wrote:
> > MLD module's context is atomic although most logic is called from
> > control-path, not data path. Only a few functions are called from
> > datapath, most of the functions are called from the control-path.
> > Furthermore, MLD's response is not processed immediately because
> > MLD protocol is using delayed response.
> > It means that If a query is received, the node should have a delay
> > in response At this point, it could change the context.
> > It means most of the functions can be implemented as the sleepable
> > context so that mld functions can use sleepable functions.
> >
> > Most resources are protected by spinlock and rwlock so the context
> > of mld functions is atomic. So, in order to change context, locking
> > scenario should be changed.
> > It switches from spinlock/rwlock to mutex and rcu.
> >
> > Some locks are deleted and added.
> > 1. ipv6->mc_socklist->sflock is deleted
> > This is rwlock and it is unnecessary.
> > Because it protects ipv6_mc_socklist-sflist but it is now protected
> > by rtnl_lock().
> >
> > 2. ifmcaddr6->mca_work_lock is added.
> > This lock protects ifmcaddr6->mca_work.
> > This workqueue can be used by both control-path and data-path.
> > It means mutex can't be used.
> > So mca_work_lock(spinlock) is added.
> >
> > 3. inet6_dev->mc_tomb_lock is deleted
> > This lock protects inet6_dev->mc_bom_list.
> > But it is protected by rtnl_lock().
> >
> > 4. inet6_dev->lock is used for protecting workqueues.
> > inet6_dev has its own workqueues(mc_gq_work, mc_ifc_work, mc_delrec_work)
> > and it can be started and stop by both control-path and data-path.
> > So, mutex can't be used.
> >
> > Suggested-by: Cong Wang <xiyou.wangcong@...il.com>
> > Signed-off-by: Taehee Yoo <ap420073@...il.com>
> > ---
> >  drivers/s390/net/qeth_l3_main.c |   6 +-
> >  include/net/if_inet6.h          |  29 +-
> >  net/batman-adv/multicast.c      |   4 +-
> >  net/ipv6/addrconf.c             |   4 +-
> >  net/ipv6/mcast.c                | 785 ++++++++++++++++----------------
> >  5 files changed, 411 insertions(+), 417 deletions(-)
> >
> > diff --git a/drivers/s390/net/qeth_l3_main.c b/drivers/s390/net/qeth_l3_main.c
> > index e49abdeff69c..085afb24482e 100644
> > --- a/drivers/s390/net/qeth_l3_main.c
> > +++ b/drivers/s390/net/qeth_l3_main.c
> > @@ -1098,8 +1098,8 @@ static int qeth_l3_add_mcast_rtnl(struct net_device *dev, int vid, void *arg)
> >       tmp.disp_flag = QETH_DISP_ADDR_ADD;
> >       tmp.is_multicast = 1;
> >
> > -     read_lock_bh(&in6_dev->lock);
> > -     list_for_each_entry(im6, in6_dev->mc_list, list) {
> > +     rcu_read_lock();
> > +     list_for_each_entry_rcu(im6, in6_dev->mc_list, list) {
>
> No need for the rcu_read_lock(), we're called under rtnl.
> So if there's a v2, please just make this
>

Thanks! I missed checking for the RTNL :)

In the next patch, mc_list will not be changed to use list macro.
So I will just remove read_{lock | unlock}_bh() in here.

>         list_for_each_entry_rcu(im6, in6_dev->mc_list, list,
>                                 lockdep_rtnl_is_held())
>
> >               tmp.u.a6.addr = im6->mca_addr;
> >
> >               ipm = qeth_l3_find_addr_by_ip(card, &tmp);
> > @@ -1117,7 +1117,7 @@ static int qeth_l3_add_mcast_rtnl(struct net_device *dev, int vid, void *arg)
> >                        qeth_l3_ipaddr_hash(ipm));
> >
> >       }
> > -     read_unlock_bh(&in6_dev->lock);
> > +     rcu_read_unlock();
> >
> >  out:
> >       return 0;
>

Thanks a lot!
Taehee Yoo