Netdev Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH net-next 0/2] net: ipv4: remove erroneous advancement of list pointer
@ 2019-06-17 14:02 Florian Westphal
  2019-06-17 14:02 ` [PATCH net-next 1/2] " Florian Westphal
                   ` (3 more replies)
  0 siblings, 4 replies; 10+ messages in thread
From: Florian Westphal @ 2019-06-17 14:02 UTC (permalink / raw)
  To: netdev; +Cc: tariqt, ranro, maorg, edumazet

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).


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH net-next 1/2] net: ipv4: remove erroneous advancement of list pointer
  2019-06-17 14:02 [PATCH net-next 0/2] net: ipv4: remove erroneous advancement of list pointer Florian Westphal
@ 2019-06-17 14:02 ` " Florian Westphal
  2019-06-17 14:02 ` [PATCH net-next 2/2] selftests: rtnetlink: add addresses with fixed life time Florian Westphal
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 10+ messages in thread
From: Florian Westphal @ 2019-06-17 14:02 UTC (permalink / raw)
  To: netdev; +Cc: tariqt, ranro, maorg, edumazet, 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


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH net-next 2/2] selftests: rtnetlink: add addresses with fixed life time
  2019-06-17 14:02 [PATCH net-next 0/2] net: ipv4: remove erroneous advancement of list pointer Florian Westphal
  2019-06-17 14:02 ` [PATCH net-next 1/2] " Florian Westphal
@ 2019-06-17 14:02 ` Florian Westphal
  2019-06-17 16:16 ` [PATCH net-next 0/2] net: ipv4: remove erroneous advancement of list pointer Tariq Toukan
  2019-06-17 23:27 ` David Miller
  3 siblings, 0 replies; 10+ messages in thread
From: Florian Westphal @ 2019-06-17 14:02 UTC (permalink / raw)
  To: netdev; +Cc: tariqt, ranro, maorg, edumazet, 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


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH net-next 0/2] net: ipv4: remove erroneous advancement of list pointer
  2019-06-17 14:02 [PATCH net-next 0/2] net: ipv4: remove erroneous advancement of list pointer Florian Westphal
  2019-06-17 14:02 ` [PATCH net-next 1/2] " Florian Westphal
  2019-06-17 14:02 ` [PATCH net-next 2/2] selftests: rtnetlink: add addresses with fixed life time Florian Westphal
@ 2019-06-17 16:16 ` Tariq Toukan
  2019-06-25  9:04   ` Ran Rozenstein
  2019-06-17 23:27 ` David Miller
  3 siblings, 1 reply; 10+ messages in thread
From: Tariq Toukan @ 2019-06-17 16:16 UTC (permalink / raw)
  To: Florian Westphal, netdev; +Cc: Ran Rozenstein, Maor Gottlieb, edumazet



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?


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH net-next 0/2] net: ipv4: remove erroneous advancement of list pointer
  2019-06-17 14:02 [PATCH net-next 0/2] net: ipv4: remove erroneous advancement of list pointer Florian Westphal
                   ` (2 preceding siblings ...)
  2019-06-17 16:16 ` [PATCH net-next 0/2] net: ipv4: remove erroneous advancement of list pointer Tariq Toukan
@ 2019-06-17 23:27 ` David Miller
  3 siblings, 0 replies; 10+ messages in thread
From: David Miller @ 2019-06-17 23:27 UTC (permalink / raw)
  To: fw; +Cc: netdev, tariqt, ranro, maorg, edumazet

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.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* RE: [PATCH net-next 0/2] net: ipv4: remove erroneous advancement of list pointer
  2019-06-17 16:16 ` [PATCH net-next 0/2] net: ipv4: remove erroneous advancement of list pointer Tariq Toukan
@ 2019-06-25  9:04   ` Ran Rozenstein
  2019-06-25  9:19     ` Florian Westphal
  0 siblings, 1 reply; 10+ messages in thread
From: Ran Rozenstein @ 2019-06-25  9:04 UTC (permalink / raw)
  To: Tariq Toukan, Florian Westphal, netdev; +Cc: Maor Gottlieb, edumazet



> -----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.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH net-next 0/2] net: ipv4: remove erroneous advancement of list pointer
  2019-06-25  9:04   ` Ran Rozenstein
@ 2019-06-25  9:19     ` Florian Westphal
  2019-06-26 12:48       ` Ran Rozenstein
  0 siblings, 1 reply; 10+ messages in thread
From: Florian Westphal @ 2019-06-25  9:19 UTC (permalink / raw)
  To: Ran Rozenstein
  Cc: Tariq Toukan, Florian Westphal, netdev, Maor Gottlieb, edumazet

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?

^ permalink raw reply	[flat|nested] 10+ messages in thread

* RE: [PATCH net-next 0/2] net: ipv4: remove erroneous advancement of list pointer
  2019-06-25  9:19     ` Florian Westphal
@ 2019-06-26 12:48       ` Ran Rozenstein
  2019-06-26 20:37         ` Florian Westphal
  2019-06-26 23:32         ` Florian Westphal
  0 siblings, 2 replies; 10+ messages in thread
From: Ran Rozenstein @ 2019-06-26 12:48 UTC (permalink / raw)
  To: Florian Westphal; +Cc: Tariq Toukan, netdev, Maor Gottlieb, edumazet



> -----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




^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH net-next 0/2] net: ipv4: remove erroneous advancement of list pointer
  2019-06-26 12:48       ` Ran Rozenstein
@ 2019-06-26 20:37         ` Florian Westphal
  2019-06-26 23:32         ` Florian Westphal
  1 sibling, 0 replies; 10+ messages in thread
From: Florian Westphal @ 2019-06-26 20:37 UTC (permalink / raw)
  To: Ran Rozenstein
  Cc: Florian Westphal, Tariq Toukan, netdev, Maor Gottlieb, edumazet

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).

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH net-next 0/2] net: ipv4: remove erroneous advancement of list pointer
  2019-06-26 12:48       ` Ran Rozenstein
  2019-06-26 20:37         ` Florian Westphal
@ 2019-06-26 23:32         ` Florian Westphal
  1 sibling, 0 replies; 10+ messages in thread
From: Florian Westphal @ 2019-06-26 23:32 UTC (permalink / raw)
  To: Ran Rozenstein
  Cc: Florian Westphal, Tariq Toukan, netdev, Maor Gottlieb, edumazet

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.

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, back to index

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-06-17 14:02 [PATCH net-next 0/2] net: ipv4: remove erroneous advancement of list pointer Florian Westphal
2019-06-17 14:02 ` [PATCH net-next 1/2] " Florian Westphal
2019-06-17 14:02 ` [PATCH net-next 2/2] selftests: rtnetlink: add addresses with fixed life time Florian Westphal
2019-06-17 16:16 ` [PATCH net-next 0/2] net: ipv4: remove erroneous advancement of list pointer Tariq Toukan
2019-06-25  9:04   ` Ran Rozenstein
2019-06-25  9:19     ` Florian Westphal
2019-06-26 12:48       ` Ran Rozenstein
2019-06-26 20:37         ` Florian Westphal
2019-06-26 23:32         ` Florian Westphal
2019-06-17 23:27 ` David Miller

Netdev Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/netdev/0 netdev/git/0.git
	git clone --mirror https://lore.kernel.org/netdev/1 netdev/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 netdev netdev/ https://lore.kernel.org/netdev \
		netdev@vger.kernel.org netdev@archiver.kernel.org
	public-inbox-index netdev


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.netdev


AGPL code for this site: git clone https://public-inbox.org/ public-inbox