[PATCH net-next 0/2] net: ipv4: remove erroneous advancement of list pointer

[PATCH net-next 0/2] net: ipv4: remove erroneous advancement of list pointer

[Florian Westphal]

Tariq reported a soft lockup on net-next that Mellanox was able to
bisect to 2638eb8b50cf ("net: ipv4: provide __rcu annotation for ifa_list").

While reviewing above patch I found a regression when addresses have a
lifetime specified.

Second patch extends rtnetlink.sh to trigger crash
(without first patch applied).

[PATCH net-next 1/2] net: ipv4: remove erroneous advancement of list pointer

[Florian Westphal]

Causes crash when lifetime expires on an adress as garbage is
dereferenced soon after.

This used to look like this:

 for (ifap = &ifa->ifa_dev->ifa_list;
      *ifap != NULL; ifap = &(*ifap)->ifa_next) {
          if (*ifap == ifa) ...

but this was changed to:

struct in_ifaddr *tmp;

ifap = &ifa->ifa_dev->ifa_list;
tmp = rtnl_dereference(*ifap);
while (tmp) {
   tmp = rtnl_dereference(tmp->ifa_next); // Bogus
   if (rtnl_dereference(*ifap) == ifa) {
     ...
   ifap = &tmp->ifa_next;		// Can be NULL
   tmp = rtnl_dereference(*ifap);	// Dereference
   }
}

Remove the bogus assigment/list entry skip.

Fixes: 2638eb8b50cf ("net: ipv4: provide __rcu annotation for ifa_list")
Signed-off-by: Florian Westphal <fw@strlen.de>
---
 net/ipv4/devinet.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/net/ipv4/devinet.c b/net/ipv4/devinet.c
index 925dffa915cb..914ccc7f192a 100644
--- a/net/ipv4/devinet.c
+++ b/net/ipv4/devinet.c
@@ -745,8 +745,7 @@ static void check_lifetime(struct work_struct *work)
 				ifap = &ifa->ifa_dev->ifa_list;
 				tmp = rtnl_dereference(*ifap);
 				while (tmp) {
-					tmp = rtnl_dereference(tmp->ifa_next);
-					if (rtnl_dereference(*ifap) == ifa) {
+					if (tmp == ifa) {
 						inet_del_ifa(ifa->ifa_dev,
 							     ifap, 1);
 						break;
-- 
2.21.0

[PATCH net-next 2/2] selftests: rtnetlink: add addresses with fixed life time

[Florian Westphal]

This exercises kernel code path that deal with addresses that have
a limited lifetime.

Without previous fix, this triggers following crash on net-next:
 BUG: KASAN: null-ptr-deref in check_lifetime+0x403/0x670
 Read of size 8 at addr 0000000000000010 by task kworker [..]

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 tools/testing/selftests/net/rtnetlink.sh | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/tools/testing/selftests/net/rtnetlink.sh b/tools/testing/selftests/net/rtnetlink.sh
index b25c9fe019d2..ed606a2e3865 100755
--- a/tools/testing/selftests/net/rtnetlink.sh
+++ b/tools/testing/selftests/net/rtnetlink.sh
@@ -249,6 +249,26 @@ kci_test_route_get()
 	echo "PASS: route get"
 }
 
+kci_test_addrlft()
+{
+	for i in $(seq 10 100) ;do
+		lft=$(((RANDOM%3) + 1))
+		ip addr add 10.23.11.$i/32 dev "$devdummy" preferred_lft $lft valid_lft $((lft+1))
+		check_err $?
+	done
+
+	sleep 5
+
+	ip addr show dev "$devdummy" | grep "10.23.11."
+	if [ $? -eq 0 ]; then
+		echo "FAIL: preferred_lft addresses remaining"
+		check_err 1
+		return
+	fi
+
+	echo "PASS: preferred_lft addresses have expired"
+}
+
 kci_test_addrlabel()
 {
 	ret=0
@@ -1140,6 +1160,7 @@ kci_test_rtnl()
 
 	kci_test_polrouting
 	kci_test_route_get
+	kci_test_addrlft
 	kci_test_tc
 	kci_test_gre
 	kci_test_gretap
-- 
2.21.0

Re: [PATCH net-next 0/2] net: ipv4: remove erroneous advancement of list pointer

[Tariq Toukan]

On 6/17/2019 5:02 PM, Florian Westphal wrote:
> Tariq reported a soft lockup on net-next that Mellanox was able to
> bisect to 2638eb8b50cf ("net: ipv4: provide __rcu annotation for ifa_list").
> 
> While reviewing above patch I found a regression when addresses have a
> lifetime specified.
> 
> Second patch extends rtnetlink.sh to trigger crash
> (without first patch applied).
> 

Thanks Florian.

Ran, can you please test?

Re: [PATCH net-next 0/2] net: ipv4: remove erroneous advancement of list pointer

[David Miller]

From: Florian Westphal <fw@strlen.de>
Date: Mon, 17 Jun 2019 16:02:26 +0200

> Tariq reported a soft lockup on net-next that Mellanox was able to
> bisect to 2638eb8b50cf ("net: ipv4: provide __rcu annotation for ifa_list").
> 
> While reviewing above patch I found a regression when addresses have a
> lifetime specified.
> 
> Second patch extends rtnetlink.sh to trigger crash
> (without first patch applied).

Series applied, thanks Florian.

RE: [PATCH net-next 0/2] net: ipv4: remove erroneous advancement of list pointer

[Ran Rozenstein]

> -----Original Message-----
> From: Tariq Toukan
> Sent: Monday, June 17, 2019 19:16
> To: Florian Westphal <fw@strlen.de>; netdev@vger.kernel.org
> Cc: Ran Rozenstein <ranro@mellanox.com>; Maor Gottlieb
> <maorg@mellanox.com>; edumazet@google.com
> Subject: Re: [PATCH net-next 0/2] net: ipv4: remove erroneous
> advancement of list pointer
> 
> 
> 
> On 6/17/2019 5:02 PM, Florian Westphal wrote:
> > Tariq reported a soft lockup on net-next that Mellanox was able to
> > bisect to 2638eb8b50cf ("net: ipv4: provide __rcu annotation for ifa_list").
> >
> > While reviewing above patch I found a regression when addresses have a
> > lifetime specified.
> >
> > Second patch extends rtnetlink.sh to trigger crash (without first
> > patch applied).
> >
> 
> Thanks Florian.
> 
> Ran, can you please test?

Tested, still reproduce.

Re: [PATCH net-next 0/2] net: ipv4: remove erroneous advancement of list pointer

[Florian Westphal]

Ran Rozenstein <ranro@mellanox.com> wrote:
> > On 6/17/2019 5:02 PM, Florian Westphal wrote:
> > > Tariq reported a soft lockup on net-next that Mellanox was able to
> > > bisect to 2638eb8b50cf ("net: ipv4: provide __rcu annotation for ifa_list").
> > >
> > > While reviewing above patch I found a regression when addresses have a
> > > lifetime specified.
> > >
> > > Second patch extends rtnetlink.sh to trigger crash (without first
> > > patch applied).
> > >
> > 
> > Thanks Florian.
> > 
> > Ran, can you please test?
> 
> Tested, still reproduce.

Can you be a little more specific? Is there any reproducer?

RE: [PATCH net-next 0/2] net: ipv4: remove erroneous advancement of list pointer

[Ran Rozenstein]

> -----Original Message-----
> From: Florian Westphal [mailto:fw@strlen.de]
> Sent: Tuesday, June 25, 2019 12:19
> To: Ran Rozenstein <ranro@mellanox.com>
> Cc: Tariq Toukan <tariqt@mellanox.com>; Florian Westphal <fw@strlen.de>;
> netdev@vger.kernel.org; Maor Gottlieb <maorg@mellanox.com>;
> edumazet@google.com
> Subject: Re: [PATCH net-next 0/2] net: ipv4: remove erroneous
> advancement of list pointer
> 
> Ran Rozenstein <ranro@mellanox.com> wrote:
> > > On 6/17/2019 5:02 PM, Florian Westphal wrote:
> > > > Tariq reported a soft lockup on net-next that Mellanox was able to
> > > > bisect to 2638eb8b50cf ("net: ipv4: provide __rcu annotation for
> ifa_list").
> > > >
> > > > While reviewing above patch I found a regression when addresses
> > > > have a lifetime specified.
> > > >
> > > > Second patch extends rtnetlink.sh to trigger crash (without first
> > > > patch applied).
> > > >
> > >
> > > Thanks Florian.
> > >
> > > Ran, can you please test?
> >
> > Tested, still reproduce.
> 
> Can you be a little more specific? Is there any reproducer?

The test dose stress on the interface by running this 2 commands in loop:

command is: /sbin/ip -f inet addr add $IP/16 brd + dev ens8f1
command is: ifconfig ens8f1 $IP netmask 255.255.0.0

when $IP change every iteration.

It execute every second when we see the reproduce somewhere between 40 to 200 seconds of execution.

Thanks,
Ran

Re: [PATCH net-next 0/2] net: ipv4: remove erroneous advancement of list pointer

[Florian Westphal]

Ran Rozenstein <ranro@mellanox.com> wrote:
> The test dose stress on the interface by running this 2 commands in loop:
> 
> command is: /sbin/ip -f inet addr add $IP/16 brd + dev ens8f1
> command is: ifconfig ens8f1 $IP netmask 255.255.0.0
> 
> when $IP change every iteration.
> 
> It execute every second when we see the reproduce somewhere between 40 to 200 seconds of execution.

I tried this without success:

DEV=dummy0
for j in $(seq 2 254);do
  for i in $(seq 2 254);do
    IP="10.$((RANDOM%254)).$((RANDOM%254)).$i"
    ip -f inet addr add $IP/16 brd + dev $DEV
    ifconfig $DEV $IP netmask 255.255.0.0
  done
done

I'll let this loop overnight, but so far nothing turned up in
dmesg (lockdep/kasan kernel).

Re: [PATCH net-next 0/2] net: ipv4: remove erroneous advancement of list pointer

[Florian Westphal]

Ran Rozenstein <ranro@mellanox.com> wrote:
> The test dose stress on the interface by running this 2 commands in loop:
> 
> command is: /sbin/ip -f inet addr add $IP/16 brd + dev ens8f1
> command is: ifconfig ens8f1 $IP netmask 255.255.0.0
> 
> when $IP change every iteration.
> 
> It execute every second when we see the reproduce somewhere between 40 to 200 seconds of execution.

I can reproduce this now,  I'll submit a fix tomorrow.