linux-kernel - Re: [PATCH net] ipmr: Fix access to mfc_cache

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] [thread-next>] [day] [month] [year] [list]
Message-ID: <CANn89i+kYM_QRsqGXfbw-nzTe5K=sbVW3G+Nb2pCW3so5Tr-7w@mail.gmail.com>
Date: Thu, 7 Nov 2024 15:04:19 +0100
From: Eric Dumazet <edumazet@...gle.com>
To: Breno Leitao <leitao@...ian.org>
Cc: "David S. Miller" <davem@...emloft.net>, David Ahern <dsahern@...nel.org>, 
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, Simon Horman <horms@...nel.org>, 
	David Ahern <dsahern@...il.com>, netdev@...r.kernel.org, linux-kernel@...r.kernel.org, 
	kernel-team@...a.com
Subject: Re: [PATCH net] ipmr: Fix access to mfc_cache_list without lock held

On Thu, Nov 7, 2024 at 2:51 PM Breno Leitao <leitao@...ian.org> wrote:
>
> Hello Eric,
>
> On Thu, Nov 07, 2024 at 02:13:14PM +0100, Eric Dumazet wrote:
> > On Thu, Nov 7, 2024 at 12:03 PM Breno Leitao <leitao@...ian.org> wrote:
> > >
> > > Accessing `mr_table->mfc_cache_list` is protected by an RCU lock. In the
> > > following code flow, the lock is not held, causing the following error
> > > when `RCU_PROVE` is not held.
> > >
> > >         6.12.0-rc5-kbuilder-01145-gbac17284bdcb #33 Tainted: G            E    N
> > >         -----------------------------
> > >         net/ipv4/ipmr_base.c:313 RCU-list traversed in non-reader section!!
> > >
> > >         rcu_scheduler_active = 2, debug_locks = 1
> > >                    2 locks held by RetransmitAggre/3519:
> > >                     #0: ffff88816188c6c0 (nlk_cb_mutex-ROUTE){+.+.}-{3:3}, at: __netlink_dump_start+0x8a/0x290
> > >                     #1: ffffffff83fcf7a8 (rtnl_mutex){+.+.}-{3:3}, at: rtnl_dumpit+0x6b/0x90
> > >
> > >         stack backtrace:
> > >                     lockdep_rcu_suspicious
> > >                     mr_table_dump
> > >                     ipmr_rtm_dumproute
> > >                     rtnl_dump_all
> > >                     rtnl_dumpit
> > >                     netlink_dump
> > >                     __netlink_dump_start
> > >                     rtnetlink_rcv_msg
> > >                     netlink_rcv_skb
> > >                     netlink_unicast
> > >                     netlink_sendmsg
> > >
> > > Fix accessing `mfc_cache_list` without holding the RCU read lock. Adds
> > > `rcu_read_lock()` and `rcu_read_unlock()` around `mr_table_dump()` to
> > > prevent RCU-list traversal in non-reader section.
> > >
> > > Since `mr_table_dump()` is the only function that touches the list, that
> > > might be the only critical section in `ipmr_rtm_dumproute()` that needs
> > > to be protected in ipmr_rtm_dumproute().
> > >
> > > Signed-off-by: Breno Leitao <leitao@...ian.org>
> > > Fixes: cb167893f41e ("net: Plumb support for filtering ipv4 and ipv6 multicast route dumps")
> > > ---
> > >  net/ipv4/ipmr.c | 2 ++
> > >  1 file changed, 2 insertions(+)
> > >
> > > diff --git a/net/ipv4/ipmr.c b/net/ipv4/ipmr.c
> > > index 089864c6a35eec146a1ba90c22d79245f8e48158..bb855f32f328024f384a2fa58f42fc227705206e 100644
> > > --- a/net/ipv4/ipmr.c
> > > +++ b/net/ipv4/ipmr.c
> > > @@ -2612,8 +2612,10 @@ static int ipmr_rtm_dumproute(struct sk_buff *skb, struct netlink_callback *cb)
> > >                         NL_SET_ERR_MSG(cb->extack, "ipv4: MR table does not exist");
> > >                         return -ENOENT;
> > >                 }
> > > +               rcu_read_lock();
> > >                 err = mr_table_dump(mrt, skb, cb, _ipmr_fill_mroute,
> > >                                     &mfc_unres_lock, &filter);
> > > +               rcu_read_unlock();
> > >                 return skb->len ? : err;
> > >         }
> > >
> > >
> >
> > What about net/ipv6/ip6mr.c ip6mr_rtm_dumproute() ?
>
> That one might require as well.
>
> > In my opinion, since we still hold RTNL in these paths, we should
> > change the lockdep annotation.
>
> I don't have much experience mixing locks like this. Is it safe to mix
> and match rtnl and RCUs like this?

If holding RTNL is preventing any updates, then surely holding RTNL is enough
to iterate through the list.



>
> I have the impression that, when iterating a RCU protected list *without* being in the read-side
> critical sections, the RCU doesn't know that someone might be traversing
> the list, and remove the element mid air (mroute_clean_tables()?). Is
> this model incorrect?
>
> > Then later we can remove RTNL from these dump operations.
>
> Do you mean that, execute the dump operation without holding the RTNL,
> thus, relying solely on RCU?

Right, you might have seen patches adding RTNL_FLAG_DUMP_UNLOCKED in
some places ?

More patches are welcomed, in net-next.

>
> > diff --git a/net/ipv4/ipmr_base.c b/net/ipv4/ipmr_base.c
> > index 271dc03fc6dbd9b35db4d5782716679134f225e4..f0af12a2f70bcdf5ba54321bf7ebebe798318abb
> > 100644
> > --- a/net/ipv4/ipmr_base.c
> > +++ b/net/ipv4/ipmr_base.c
> > @@ -310,7 +310,8 @@ int mr_table_dump(struct mr_table *mrt, struct sk_buff *skb,
> >         if (filter->filter_set)
> >                 flags |= NLM_F_DUMP_FILTERED;
> >
> > -       list_for_each_entry_rcu(mfc, &mrt->mfc_cache_list, list) {
> > +       list_for_each_entry_rcu(mfc, &mrt->mfc_cache_list, list,
> > +                               lockdep_rtnl_is_held()) {
> >                 if (e < s_e)
> >                         goto next_entry;
> >                 if (filter->dev &&
>
> Clarifying next steps: Would you like me to review/test and submit, or
> are you planning to send it officially?

For this kind of feedback, I am usually expecting you to send a new
version (after waiting one day,
maybe other reviewers have something to say)

Thank you.