[PATCH net-next v3] net-loopback: set lo dev initial state to UP

[PATCH net-next v3] net-loopback: set lo dev initial state to UP

[Jian Yang]

From: Jian Yang <jianyang@google.com>

Traditionally loopback devices come up with initial state as DOWN for
any new network-namespace. This would mean that anyone needing this
device would have to bring this UP by issuing something like 'ip link
set lo up'. This can be avoided if the initial state is set as UP.

Signed-off-by: Mahesh Bandewar <maheshb@google.com>
Signed-off-by: Jian Yang <jianyang@google.com>
---
v3:
  * Addressed Jakub's comment to remove the sysctl knob

v2:
  * Updated sysctl name from `netdev_loopback_state` to `loopback_init_state`
  * Fixed the linking error when CONFIG_SYSCTL is not defined

 drivers/net/loopback.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/net/loopback.c b/drivers/net/loopback.c
index a1c77cc00416..24487ec17f8b 100644
--- a/drivers/net/loopback.c
+++ b/drivers/net/loopback.c
@@ -219,6 +219,12 @@ static __net_init int loopback_net_init(struct net *net)
 
 	BUG_ON(dev->ifindex != LOOPBACK_IFINDEX);
 	net->loopback_dev = dev;
+
+	/* bring loopback device UP */
+	rtnl_lock();
+	dev_open(dev, NULL);
+	rtnl_unlock();
+
 	return 0;
 
 out_free_netdev:
-- 
2.30.0.365.g02bc693789-goog

Re: [PATCH net-next v3] net-loopback: set lo dev initial state to UP

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net-next.git (refs/heads/master):

On Mon,  1 Feb 2021 15:34:45 -0800 you wrote:
> From: Jian Yang <jianyang@google.com>
> 
> Traditionally loopback devices come up with initial state as DOWN for
> any new network-namespace. This would mean that anyone needing this
> device would have to bring this UP by issuing something like 'ip link
> set lo up'. This can be avoided if the initial state is set as UP.
> 
> [...]

Here is the summary with links:
  - [net-next,v3] net-loopback: set lo dev initial state to UP
    https://git.kernel.org/netdev/net-next/c/c9dca822c729

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

Re: [PATCH net-next v3] net-loopback: set lo dev initial state to UP

[Petr Machata]

Jian Yang <jianyang.kernel@gmail.com> writes:

> From: Jian Yang <jianyang@google.com>
>
> Traditionally loopback devices come up with initial state as DOWN for
> any new network-namespace. This would mean that anyone needing this
> device would have to bring this UP by issuing something like 'ip link
> set lo up'. This can be avoided if the initial state is set as UP.

This will break user scripts, and it fact breaks kernel's very own
selftest. We currently have this internally:

    diff --git a/tools/testing/selftests/net/fib_nexthops.sh b/tools/testing/selftests/net/fib_nexthops.sh
    index 4c7d33618437..bf8ed24ab3ba 100755
    --- a/tools/testing/selftests/net/fib_nexthops.sh
    +++ b/tools/testing/selftests/net/fib_nexthops.sh
    @@ -121,8 +121,6 @@ create_ns()
     	set -e
     	ip netns add ${n}
     	ip netns set ${n} $((nsid++))
    -	ip -netns ${n} addr add 127.0.0.1/8 dev lo
    -	ip -netns ${n} link set lo up

     	ip netns exec ${n} sysctl -qw net.ipv4.ip_forward=1
     	ip netns exec ${n} sysctl -qw net.ipv4.fib_multipath_use_neigh=1
    -- 
    2.26.2

This now fails because the ip commands are run within a "set -e" block,
and kernel rejects addition of a duplicate address.

Re: [PATCH net-next v3] net-loopback: set lo dev initial state to UP

[Jakub Kicinski]

On Tue, 9 Feb 2021 12:54:59 +0100 Petr Machata wrote:
> Jian Yang <jianyang.kernel@gmail.com> writes:
> 
> > From: Jian Yang <jianyang@google.com>
> >
> > Traditionally loopback devices come up with initial state as DOWN for
> > any new network-namespace. This would mean that anyone needing this
> > device would have to bring this UP by issuing something like 'ip link
> > set lo up'. This can be avoided if the initial state is set as UP.  
> 
> This will break user scripts, and it fact breaks kernel's very own
> selftest. We currently have this internally:
> 
>     diff --git a/tools/testing/selftests/net/fib_nexthops.sh b/tools/testing/selftests/net/fib_nexthops.sh
>     index 4c7d33618437..bf8ed24ab3ba 100755
>     --- a/tools/testing/selftests/net/fib_nexthops.sh
>     +++ b/tools/testing/selftests/net/fib_nexthops.sh
>     @@ -121,8 +121,6 @@ create_ns()
>      	set -e
>      	ip netns add ${n}
>      	ip netns set ${n} $((nsid++))
>     -	ip -netns ${n} addr add 127.0.0.1/8 dev lo
>     -	ip -netns ${n} link set lo up
> 
>      	ip netns exec ${n} sysctl -qw net.ipv4.ip_forward=1
>      	ip netns exec ${n} sysctl -qw net.ipv4.fib_multipath_use_neigh=1
> 
> This now fails because the ip commands are run within a "set -e" block,
> and kernel rejects addition of a duplicate address.

Thanks for the report, could you send a revert with this explanation?

Re: [PATCH net-next v3] net-loopback: set lo dev initial state to UP

[Petr Machata]

Jakub Kicinski <kuba@kernel.org> writes:

> Thanks for the report, could you send a revert with this explanation?

Sure.

Re: [PATCH net-next v3] net-loopback: set lo dev initial state to UP

[Mahesh Bandewar (महेश बंडेवार)]

On Tue, Feb 9, 2021 at 8:23 AM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Tue, 9 Feb 2021 12:54:59 +0100 Petr Machata wrote:
> > Jian Yang <jianyang.kernel@gmail.com> writes:
> >
> > > From: Jian Yang <jianyang@google.com>
> > >
> > > Traditionally loopback devices come up with initial state as DOWN for
> > > any new network-namespace. This would mean that anyone needing this
> > > device would have to bring this UP by issuing something like 'ip link
> > > set lo up'. This can be avoided if the initial state is set as UP.
> >
> > This will break user scripts, and it fact breaks kernel's very own
> > selftest. We currently have this internally:
> >
> >     diff --git a/tools/testing/selftests/net/fib_nexthops.sh b/tools/testing/selftests/net/fib_nexthops.sh
> >     index 4c7d33618437..bf8ed24ab3ba 100755
> >     --- a/tools/testing/selftests/net/fib_nexthops.sh
> >     +++ b/tools/testing/selftests/net/fib_nexthops.sh
> >     @@ -121,8 +121,6 @@ create_ns()
> >       set -e
> >       ip netns add ${n}
> >       ip netns set ${n} $((nsid++))
> >     - ip -netns ${n} addr add 127.0.0.1/8 dev lo
> >     - ip -netns ${n} link set lo up
> >
> >       ip netns exec ${n} sysctl -qw net.ipv4.ip_forward=1
> >       ip netns exec ${n} sysctl -qw net.ipv4.fib_multipath_use_neigh=1
> >
> > This now fails because the ip commands are run within a "set -e" block,
> > and kernel rejects addition of a duplicate address.
>
> Thanks for the report, could you send a revert with this explanation?
Rather than revert, shouldn't we just fix the self-test in that regard?

Re: [PATCH net-next v3] net-loopback: set lo dev initial state to UP

[Jakub Kicinski]

On Tue, 9 Feb 2021 10:49:23 -0800 Mahesh Bandewar (महेश बंडेवार) wrote:
> On Tue, Feb 9, 2021 at 8:23 AM Jakub Kicinski <kuba@kernel.org> wrote:
> > On Tue, 9 Feb 2021 12:54:59 +0100 Petr Machata wrote:  
> > > This will break user scripts, and it fact breaks kernel's very own
> > > selftest. We currently have this internally:
> > >
> > >     diff --git a/tools/testing/selftests/net/fib_nexthops.sh b/tools/testing/selftests/net/fib_nexthops.sh
> > >     index 4c7d33618437..bf8ed24ab3ba 100755
> > >     --- a/tools/testing/selftests/net/fib_nexthops.sh
> > >     +++ b/tools/testing/selftests/net/fib_nexthops.sh
> > >     @@ -121,8 +121,6 @@ create_ns()
> > >       set -e
> > >       ip netns add ${n}
> > >       ip netns set ${n} $((nsid++))
> > >     - ip -netns ${n} addr add 127.0.0.1/8 dev lo
> > >     - ip -netns ${n} link set lo up
> > >
> > >       ip netns exec ${n} sysctl -qw net.ipv4.ip_forward=1
> > >       ip netns exec ${n} sysctl -qw net.ipv4.fib_multipath_use_neigh=1
> > >
> > > This now fails because the ip commands are run within a "set -e" block,
> > > and kernel rejects addition of a duplicate address.  
> >
> > Thanks for the report, could you send a revert with this explanation?  
> Rather than revert, shouldn't we just fix the self-test in that regard?

The selftest is just a messenger. We all know Linus's stand on
regressions, IMO we can't make an honest argument that the change
does not break user space after it broke our own selftest. Maybe 
others disagree..

Re: [PATCH net-next v3] net-loopback: set lo dev initial state to UP

[Mahesh Bandewar (महेश बंडेवार)]

On Tue, Feb 9, 2021 at 11:04 AM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Tue, 9 Feb 2021 10:49:23 -0800 Mahesh Bandewar (महेश बंडेवार) wrote:
> > On Tue, Feb 9, 2021 at 8:23 AM Jakub Kicinski <kuba@kernel.org> wrote:
> > > On Tue, 9 Feb 2021 12:54:59 +0100 Petr Machata wrote:
> > > > This will break user scripts, and it fact breaks kernel's very own
> > > > selftest. We currently have this internally:
> > > >
> > > >     diff --git a/tools/testing/selftests/net/fib_nexthops.sh b/tools/testing/selftests/net/fib_nexthops.sh
> > > >     index 4c7d33618437..bf8ed24ab3ba 100755
> > > >     --- a/tools/testing/selftests/net/fib_nexthops.sh
> > > >     +++ b/tools/testing/selftests/net/fib_nexthops.sh
> > > >     @@ -121,8 +121,6 @@ create_ns()
> > > >       set -e
> > > >       ip netns add ${n}
> > > >       ip netns set ${n} $((nsid++))
> > > >     - ip -netns ${n} addr add 127.0.0.1/8 dev lo
> > > >     - ip -netns ${n} link set lo up
> > > >
> > > >       ip netns exec ${n} sysctl -qw net.ipv4.ip_forward=1
> > > >       ip netns exec ${n} sysctl -qw net.ipv4.fib_multipath_use_neigh=1
> > > >
> > > > This now fails because the ip commands are run within a "set -e" block,
> > > > and kernel rejects addition of a duplicate address.
> > >
> > > Thanks for the report, could you send a revert with this explanation?
> > Rather than revert, shouldn't we just fix the self-test in that regard?
>
> The selftest is just a messenger. We all know Linus's stand on
> regressions, IMO we can't make an honest argument that the change
> does not break user space after it broke our own selftest. Maybe
> others disagree..

Actually that was the reason behind encompassing this behavior change
with a sysctl.

Re: [PATCH net-next v3] net-loopback: set lo dev initial state to UP

[Jakub Kicinski]

On Tue, 9 Feb 2021 11:18:05 -0800 Mahesh Bandewar (महेश बंडेवार) wrote:
> On Tue, Feb 9, 2021 at 11:04 AM Jakub Kicinski <kuba@kernel.org> wrote:
> > On Tue, 9 Feb 2021 10:49:23 -0800 Mahesh Bandewar (महेश बंडेवार) wrote:  
> > > On Tue, Feb 9, 2021 at 8:23 AM Jakub Kicinski <kuba@kernel.org> wrote:  
> > > > On Tue, 9 Feb 2021 12:54:59 +0100 Petr Machata wrote:  
> > > > > This will break user scripts, and it fact breaks kernel's very own
> > > > > selftest. We currently have this internally:
> > > > >
> > > > >     diff --git a/tools/testing/selftests/net/fib_nexthops.sh b/tools/testing/selftests/net/fib_nexthops.sh
> > > > >     index 4c7d33618437..bf8ed24ab3ba 100755
> > > > >     --- a/tools/testing/selftests/net/fib_nexthops.sh
> > > > >     +++ b/tools/testing/selftests/net/fib_nexthops.sh
> > > > >     @@ -121,8 +121,6 @@ create_ns()
> > > > >       set -e
> > > > >       ip netns add ${n}
> > > > >       ip netns set ${n} $((nsid++))
> > > > >     - ip -netns ${n} addr add 127.0.0.1/8 dev lo
> > > > >     - ip -netns ${n} link set lo up
> > > > >
> > > > >       ip netns exec ${n} sysctl -qw net.ipv4.ip_forward=1
> > > > >       ip netns exec ${n} sysctl -qw net.ipv4.fib_multipath_use_neigh=1
> > > > >
> > > > > This now fails because the ip commands are run within a "set -e" block,
> > > > > and kernel rejects addition of a duplicate address.  
> > > >
> > > > Thanks for the report, could you send a revert with this explanation?  
> > > Rather than revert, shouldn't we just fix the self-test in that regard?  
> >
> > The selftest is just a messenger. We all know Linus's stand on
> > regressions, IMO we can't make an honest argument that the change
> > does not break user space after it broke our own selftest. Maybe
> > others disagree..  
> 
> Actually that was the reason behind encompassing this behavior change
> with a sysctl.

Which as I explained to you is pointless for portable applications.

Re: [PATCH net-next v3] net-loopback: set lo dev initial state to UP

[Ido Schimmel]

On Tue, Feb 09, 2021 at 10:49:23AM -0800, Mahesh Bandewar (महेश बंडेवार) wrote:
> On Tue, Feb 9, 2021 at 8:23 AM Jakub Kicinski <kuba@kernel.org> wrote:
> >
> > On Tue, 9 Feb 2021 12:54:59 +0100 Petr Machata wrote:
> > > Jian Yang <jianyang.kernel@gmail.com> writes:
> > >
> > > > From: Jian Yang <jianyang@google.com>
> > > >
> > > > Traditionally loopback devices come up with initial state as DOWN for
> > > > any new network-namespace. This would mean that anyone needing this
> > > > device would have to bring this UP by issuing something like 'ip link
> > > > set lo up'. This can be avoided if the initial state is set as UP.
> > >
> > > This will break user scripts, and it fact breaks kernel's very own
> > > selftest. We currently have this internally:
> > >
> > >     diff --git a/tools/testing/selftests/net/fib_nexthops.sh b/tools/testing/selftests/net/fib_nexthops.sh
> > >     index 4c7d33618437..bf8ed24ab3ba 100755
> > >     --- a/tools/testing/selftests/net/fib_nexthops.sh
> > >     +++ b/tools/testing/selftests/net/fib_nexthops.sh
> > >     @@ -121,8 +121,6 @@ create_ns()
> > >       set -e
> > >       ip netns add ${n}
> > >       ip netns set ${n} $((nsid++))
> > >     - ip -netns ${n} addr add 127.0.0.1/8 dev lo
> > >     - ip -netns ${n} link set lo up
> > >
> > >       ip netns exec ${n} sysctl -qw net.ipv4.ip_forward=1
> > >       ip netns exec ${n} sysctl -qw net.ipv4.fib_multipath_use_neigh=1
> > >
> > > This now fails because the ip commands are run within a "set -e" block,
> > > and kernel rejects addition of a duplicate address.
> >
> > Thanks for the report, could you send a revert with this explanation?
> Rather than revert, shouldn't we just fix the self-test in that regard?

I reviewed such a patch internally and asked Petr to report it as a
regression instead. At the time the new behavior was added under a
sysctl, but nobody had examples for behavior that will break, so the
sysctl was removed. Now we have such an example, so the revert / sysctl
are needed.

Re: [PATCH net-next v3] net-loopback: set lo dev initial state to UP

[Mahesh Bandewar (महेश बंडेवार)]

On Tue, Feb 9, 2021 at 11:06 AM Ido Schimmel <idosch@idosch.org> wrote:
>
> On Tue, Feb 09, 2021 at 10:49:23AM -0800, Mahesh Bandewar (महेश बंडेवार) wrote:
> > On Tue, Feb 9, 2021 at 8:23 AM Jakub Kicinski <kuba@kernel.org> wrote:
> > >
> > > On Tue, 9 Feb 2021 12:54:59 +0100 Petr Machata wrote:
> > > > Jian Yang <jianyang.kernel@gmail.com> writes:
> > > >
> > > > > From: Jian Yang <jianyang@google.com>
> > > > >
> > > > > Traditionally loopback devices come up with initial state as DOWN for
> > > > > any new network-namespace. This would mean that anyone needing this
> > > > > device would have to bring this UP by issuing something like 'ip link
> > > > > set lo up'. This can be avoided if the initial state is set as UP.
> > > >
> > > > This will break user scripts, and it fact breaks kernel's very own
> > > > selftest. We currently have this internally:
> > > >
> > > >     diff --git a/tools/testing/selftests/net/fib_nexthops.sh b/tools/testing/selftests/net/fib_nexthops.sh
> > > >     index 4c7d33618437..bf8ed24ab3ba 100755
> > > >     --- a/tools/testing/selftests/net/fib_nexthops.sh
> > > >     +++ b/tools/testing/selftests/net/fib_nexthops.sh
> > > >     @@ -121,8 +121,6 @@ create_ns()
> > > >       set -e
> > > >       ip netns add ${n}
> > > >       ip netns set ${n} $((nsid++))
> > > >     - ip -netns ${n} addr add 127.0.0.1/8 dev lo
> > > >     - ip -netns ${n} link set lo up
> > > >
> > > >       ip netns exec ${n} sysctl -qw net.ipv4.ip_forward=1
> > > >       ip netns exec ${n} sysctl -qw net.ipv4.fib_multipath_use_neigh=1
> > > >
> > > > This now fails because the ip commands are run within a "set -e" block,
> > > > and kernel rejects addition of a duplicate address.
> > >
> > > Thanks for the report, could you send a revert with this explanation?
> > Rather than revert, shouldn't we just fix the self-test in that regard?
>
> I reviewed such a patch internally and asked Petr to report it as a
> regression instead. At the time the new behavior was added under a
> sysctl, but nobody had examples for behavior that will break, so the
> sysctl was removed. Now we have such an example, so the revert / sysctl
> are needed.

OK, in that case I would prefer to send an incremental patch to
enclose the new behavior with the sysctl (proposed earlier) rather
than the revert. Would that help?