[PATCH] ipv6: mcast: use rcu-safe version of ipv6_get_lladdr()

[PATCH] ipv6: mcast: use rcu-safe version of ipv6_get_lladdr()

[Ignat Korchagin]

Some time ago 8965779d2c0e ("ipv6,mcast: always hold idev->lock before mca_lock")
switched ipv6_get_lladdr() to __ipv6_get_lladdr(), which is rcu-unsafe
version. That was OK, because idev->lock was held for these codepaths.

In 88e2ca308094 ("mld: convert ifmcaddr6 to RCU") these external locks were
removed, so we probably need to restore the original rcu-safe call.

Otherwise, we occasionally get a machine crashed/stalled with the following
in dmesg:

[ 3405.966610][T230589] general protection fault, probably for non-canonical address 0xdead00000000008c: 0000 [#1] SMP NOPTI
[ 3405.982083][T230589] CPU: 44 PID: 230589 Comm: kworker/44:3 Tainted: G           O      5.15.19-cloudflare-2022.2.1 #1
[ 3405.998061][T230589] Hardware name: SUPA-COOL-SERV
[ 3406.009552][T230589] Workqueue: mld mld_ifc_work
[ 3406.017224][T230589] RIP: 0010:__ipv6_get_lladdr+0x34/0x60
[ 3406.025780][T230589] Code: 57 10 48 83 c7 08 48 89 e5 48 39 d7 74 3e 48 8d 82 38 ff ff ff eb 13 48 8b 90 d0 00 00 00 48 8d 82 38 ff ff ff 48 39 d7 74 22 <66> 83 78 32 20 77 1b 75 e4 89 ca 23 50 2c 75 dd 48 8b 50 08 48 8b
[ 3406.055748][T230589] RSP: 0018:ffff94e4b3fc3d10 EFLAGS: 00010202
[ 3406.065617][T230589] RAX: dead00000000005a RBX: ffff94e4b3fc3d30 RCX: 0000000000000040
[ 3406.077477][T230589] RDX: dead000000000122 RSI: ffff94e4b3fc3d30 RDI: ffff8c3a31431008
[ 3406.089389][T230589] RBP: ffff94e4b3fc3d10 R08: 0000000000000000 R09: 0000000000000000
[ 3406.101445][T230589] R10: ffff8c3a31430000 R11: 000000000000000b R12: ffff8c2c37887100
[ 3406.113553][T230589] R13: ffff8c3a39537000 R14: 00000000000005dc R15: ffff8c3a31431000
[ 3406.125730][T230589] FS:  0000000000000000(0000) GS:ffff8c3b9fc80000(0000) knlGS:0000000000000000
[ 3406.138992][T230589] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 3406.149895][T230589] CR2: 00007f0dfea1db60 CR3: 000000387b5f2000 CR4: 0000000000350ee0
[ 3406.162421][T230589] Call Trace:
[ 3406.170235][T230589]  <TASK>
[ 3406.177736][T230589]  mld_newpack+0xfe/0x1a0
[ 3406.186686][T230589]  add_grhead+0x87/0xa0
[ 3406.195498][T230589]  add_grec+0x485/0x4e0
[ 3406.204310][T230589]  ? newidle_balance+0x126/0x3f0
[ 3406.214024][T230589]  mld_ifc_work+0x15d/0x450
[ 3406.223279][T230589]  process_one_work+0x1e6/0x380
[ 3406.232982][T230589]  worker_thread+0x50/0x3a0
[ 3406.242371][T230589]  ? rescuer_thread+0x360/0x360
[ 3406.252175][T230589]  kthread+0x127/0x150
[ 3406.261197][T230589]  ? set_kthread_struct+0x40/0x40
[ 3406.271287][T230589]  ret_from_fork+0x22/0x30
[ 3406.280812][T230589]  </TASK>
[ 3406.288937][T230589] Modules linked in: ... [last unloaded: kheaders]
[ 3406.476714][T230589] ---[ end trace 3525a7655f2f3b9e ]---

Fixes: 88e2ca308094 ("mld: convert ifmcaddr6 to RCU")
Reported-by: David Pinilla Caparros <dpini@cloudflare.com>
Signed-off-by: Ignat Korchagin <ignat@cloudflare.com>
---
 include/net/addrconf.h | 2 --
 net/ipv6/addrconf.c    | 4 ++--
 net/ipv6/mcast.c       | 2 +-
 3 files changed, 3 insertions(+), 5 deletions(-)

diff --git a/include/net/addrconf.h b/include/net/addrconf.h
index e7ce719838b5..59940e230b78 100644
--- a/include/net/addrconf.h
+++ b/include/net/addrconf.h
@@ -109,8 +109,6 @@ struct inet6_ifaddr *ipv6_get_ifaddr(struct net *net,
 int ipv6_dev_get_saddr(struct net *net, const struct net_device *dev,
 		       const struct in6_addr *daddr, unsigned int srcprefs,
 		       struct in6_addr *saddr);
-int __ipv6_get_lladdr(struct inet6_dev *idev, struct in6_addr *addr,
-		      u32 banned_flags);
 int ipv6_get_lladdr(struct net_device *dev, struct in6_addr *addr,
 		    u32 banned_flags);
 bool inet_rcv_saddr_equal(const struct sock *sk, const struct sock *sk2,
diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index f927c199a93c..3f23da8c0b10 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -1839,8 +1839,8 @@ int ipv6_dev_get_saddr(struct net *net, const struct net_device *dst_dev,
 }
 EXPORT_SYMBOL(ipv6_dev_get_saddr);
 
-int __ipv6_get_lladdr(struct inet6_dev *idev, struct in6_addr *addr,
-		      u32 banned_flags)
+static int __ipv6_get_lladdr(struct inet6_dev *idev, struct in6_addr *addr,
+			      u32 banned_flags)
 {
 	struct inet6_ifaddr *ifp;
 	int err = -EADDRNOTAVAIL;
diff --git a/net/ipv6/mcast.c b/net/ipv6/mcast.c
index bed8155508c8..a8861db52c18 100644
--- a/net/ipv6/mcast.c
+++ b/net/ipv6/mcast.c
@@ -1759,7 +1759,7 @@ static struct sk_buff *mld_newpack(struct inet6_dev *idev, unsigned int mtu)
 	skb_reserve(skb, hlen);
 	skb_tailroom_reserve(skb, mtu, tlen);
 
-	if (__ipv6_get_lladdr(idev, &addr_buf, IFA_F_TENTATIVE)) {
+	if (ipv6_get_lladdr(dev, &addr_buf, IFA_F_TENTATIVE)) {
 		/* <draft-ietf-magma-mld-source-05.txt>:
 		 * use unspecified address as the source address
 		 * when a valid link-local address is not available.
-- 
2.20.1

Re: [PATCH] ipv6: mcast: use rcu-safe version of ipv6_get_lladdr()

[David Ahern]

On 2/11/22 9:30 AM, Ignat Korchagin wrote:
> diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
> index f927c199a93c..3f23da8c0b10 100644
> --- a/net/ipv6/addrconf.c
> +++ b/net/ipv6/addrconf.c
> @@ -1839,8 +1839,8 @@ int ipv6_dev_get_saddr(struct net *net, const struct net_device *dst_dev,
>  }
>  EXPORT_SYMBOL(ipv6_dev_get_saddr);
>  
> -int __ipv6_get_lladdr(struct inet6_dev *idev, struct in6_addr *addr,
> -		      u32 banned_flags)
> +static int __ipv6_get_lladdr(struct inet6_dev *idev, struct in6_addr *addr,
> +			      u32 banned_flags)
>  {
>  	struct inet6_ifaddr *ifp;
>  	int err = -EADDRNOTAVAIL;
> diff --git a/net/ipv6/mcast.c b/net/ipv6/mcast.c
> index bed8155508c8..a8861db52c18 100644
> --- a/net/ipv6/mcast.c
> +++ b/net/ipv6/mcast.c
> @@ -1759,7 +1759,7 @@ static struct sk_buff *mld_newpack(struct inet6_dev *idev, unsigned int mtu)
>  	skb_reserve(skb, hlen);
>  	skb_tailroom_reserve(skb, mtu, tlen);
>  
> -	if (__ipv6_get_lladdr(idev, &addr_buf, IFA_F_TENTATIVE)) {
> +	if (ipv6_get_lladdr(dev, &addr_buf, IFA_F_TENTATIVE)) {
>  		/* <draft-ietf-magma-mld-source-05.txt>:
>  		 * use unspecified address as the source address
>  		 * when a valid link-local address is not available.

Why not add read_lock_bh(&idev->lock); ... read_unlock_bh(&idev->lock);
around the call to __ipv6_get_lladdr? you already have the idev.

Re: [PATCH] ipv6: mcast: use rcu-safe version of ipv6_get_lladdr()

[Ignat Korchagin]

Stupid me - forgot to reply to all and a discussion between me and
David happend off list. Below, is the transcript for posterity:

On Sun, Feb 13, 2022 at 5:53 PM David Ahern <dsahern@gmail.com> wrote:
>
> On 2/13/22 8:43 AM, Ignat Korchagin wrote:
> > On Sun, Feb 13, 2022 at 4:17 PM David Ahern <dsahern@gmail.com> wrote:
> >>
> >> On 2/12/22 1:46 PM, Ignat Korchagin wrote:
> >>> In 8965779d2c0e ("ipv6,mcast: always hold idev->lock before mca_lock")
> >>> mld_newpack() was actually migrated from "dev" to "idev' just for this
> >>> use case. It seems the most reasonable approach would be to revert
> >>> mld_newpack() back to dev and use the original code.
> >>
> >>
> >> pmc already has the reference on idev and idev->dev is the source of dev
> >> passed to mld_newpack. There is no reason to go back to the idev -> dev
> >> -> idev dance.
> >
> > I don't know. Three things which make it more reasonable in my opinion:
> >   * we're already using idev->dev in mld_newpack() - that is we're not
> > adding an extra variable here in mld_newpack() - we need it anyway, so
> > can use in multiple places
> >   * it makes the code more consistent with the same code for the same
> > reason in igmp6_send() in the same file, which uses "dev" and
> > ipv6_get_lladdr()
> >   * we're making __ipv6_get_lladdr() static again and everything in
> > the kernel is now using the public version of ipv6_get_lladdr() - I
> > think the extra indirection of idev->dev-idev is a reasonable price to
> > pay to avoid customized locking code in the caller, which may backfire
> > later again in the same way it backfired this time
>
> which is why I later said move the locking to __ipv6_get_lladdr.
> ipv6_get_lladdr takes a net_dev, looks up the idev and calls
> __ipv6_get_lladdr. __ipv6_get_lladdr handles the idev locking needs.
>
> Users of the get_lladdr API that already have the idev reference use
> __ipv6_get_lladdr. That is a common paradigm in the stack.

Ah. I see now. This does make sense as well to me.

> igmp6 code can use some modernization - but that is a net-next change.
> This is a -net change.

Re: [PATCH] ipv6: mcast: use rcu-safe version of ipv6_get_lladdr()

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (master)
by David S. Miller <davem@davemloft.net>:

On Fri, 11 Feb 2022 17:30:42 +0000 you wrote:
> Some time ago 8965779d2c0e ("ipv6,mcast: always hold idev->lock before mca_lock")
> switched ipv6_get_lladdr() to __ipv6_get_lladdr(), which is rcu-unsafe
> version. That was OK, because idev->lock was held for these codepaths.
> 
> In 88e2ca308094 ("mld: convert ifmcaddr6 to RCU") these external locks were
> removed, so we probably need to restore the original rcu-safe call.
> 
> [...]

Here is the summary with links:
  - ipv6: mcast: use rcu-safe version of ipv6_get_lladdr()
    https://git.kernel.org/netdev/net/c/26394fc118d6

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html