Re: [PATCH] net: don't unnecessarily load kernel modules in dev_ioctl()

[Eric Dumazet]

On 03/08/2018 10:11 AM, David Miller wrote:


Yeah, understood.  At small scale the current behavior maybe made
sense, but these days it really doesn't.

No objections to the knob if you want to submit it.



Thanks David, I have rebased my patch and sent it.

New numbers on net-next are looking very nice, thanks to Kirill work.

Re: [PATCH] net: don't unnecessarily load kernel modules in dev_ioctl()

[David Miller]

From: Eric Dumazet 
Date: Thu, 8 Mar 2018 10:05:12 -0800

> Another problematic legacy behavior is the automatic creation of
> fallback tunnels, which hurts netns creation/deletion.
> 
> Some environments want to create a netns for every job/task, and they
> do not care if the init netns has these tunnels or not.
> 
> We have a local patch adding yet another knob to control this, since
> it saves a lot of cpu cycles (about 10ms per netns create/delete pair
> here)

Yeah, understood.  At small scale the current behavior maybe made
sense, but these days it really doesn't.

No objections to the knob if you want to submit it.

Re: [PATCH] net: don't unnecessarily load kernel modules in dev_ioctl()

[Eric Dumazet]

On 03/08/2018 09:34 AM, David Miller wrote:

From: Stephen Hemminger 
Date: Tue, 6 Mar 2018 15:59:20 -0800


Actually dev_load by ethernet name is really a legacy thing that
should just die,

It was kept around so that some very tunnel configuration using special names.

# ifconfig sit0

which probably several web pages still tell users to do...
We have much better control now with ip commands so that this is just
baggage.


As you say, some people use this stuff, so we really can't break
it at this point.


Another problematic legacy behavior is the automatic creation of 
fallback tunnels, which hurts netns creation/deletion.


Some environments want to create a netns for every job/task, and they do 
not care if the init netns has these tunnels or not.


We have a local patch adding yet another knob to control this, since it 
saves a lot of cpu cycles (about 10ms per netns create/delete pair here)


lpk43:~# echo 0 >/proc/sys/net/core/fb_tunnels_only_for_init_net
lpk43:~# time for i in {1..1000}; do unshare -n /bin/false;done

real0m14.939s
user0m0.152s
sys 0m1.496s

lpk43:~# unshare -n
lpk43:~# ip link
1: lo:  mtu 65536 qdisc noop state DOWN mode DEFAULT group 
default qlen 1000

link/loopback 00:00:00:00:00:00 brd 00:00:00:00:00:00
2: tunl0@NONE:  mtu 1480 qdisc noop state DOWN mode DEFAULT group 
default qlen 1000

link/ipip 0.0.0.0 brd 0.0.0.0
3: gre0@NONE:  mtu 1476 qdisc noop state DOWN mode DEFAULT group 
default qlen 1000

link/gre 0.0.0.0 brd 0.0.0.0
4: gretap0@NONE:  mtu 1462 qdisc noop state DOWN 
mode DEFAULT group default qlen 1000

link/ether 00:00:00:00:00:00 brd ff:ff:ff:ff:ff:ff
5: erspan0@NONE:  mtu 1450 qdisc noop state DOWN 
mode DEFAULT group default qlen 1000

link/ether 00:00:00:00:00:00 brd ff:ff:ff:ff:ff:ff
6: sit0@NONE:  mtu 1480 qdisc noop state DOWN mode DEFAULT group 
default qlen 1000

link/sit 0.0.0.0 brd 0.0.0.0
7: ip6tnl0@NONE:  mtu 1452 qdisc noop state DOWN mode DEFAULT 
group default qlen 1000

link/tunnel6 :: brd ::
8: ip6gre0@NONE:  mtu 1448 qdisc noop state DOWN mode DEFAULT 
group default qlen 1000
link/gre6 00:00:00:00:00:00:00:00:00:00:00:00:00:00:00:00 brd 
00:00:00:00:00:00:00:00:00:00:00:00:00:00:00:00

lpk43:~# exit
logout

lpk43:~# echo 1 >/proc/sys/net/core/fb_tunnels_only_for_init_net

lpk43:~# time for i in {1..1000}; do unshare -n /bin/false;done

real0m4.169s
user0m0.202s
sys 0m0.875s

lpk43:~# unshare -n

lpk43:~# ip link
1: lo:  mtu 65536 qdisc noop state DOWN mode DEFAULT group 
default qlen 1000

link/loopback 00:00:00:00:00:00 brd 00:00:00:00:00:00
lpk43:~#

Re: [PATCH] net: don't unnecessarily load kernel modules in dev_ioctl()

[David Miller]

From: Stephen Hemminger 
Date: Tue, 6 Mar 2018 15:59:20 -0800

> Actually dev_load by ethernet name is really a legacy thing that
> should just die,
> 
> It was kept around so that some very tunnel configuration using special names.
> 
>   # ifconfig sit0
> 
> which probably several web pages still tell users to do...
> We have much better control now with ip commands so that this is just
> baggage.

As you say, some people use this stuff, so we really can't break
it at this point.

Re: [PATCH] net: don't unnecessarily load kernel modules in dev_ioctl()

[David Miller]

From: Paul Moore 
Date: Tue, 6 Mar 2018 17:32:47 -0500

> On Tue, Mar 6, 2018 at 5:27 PM, Paul Moore  wrote:
>> From: Paul Moore 
>>
>> Starting with v4.16-rc1 we've been seeing a higher than usual number
>> of requests for the kernel to load networking modules, even on events
>> which shouldn't trigger a module load (e.g. ioctl(TCGETS)).  Stephen
>> Smalley suggested the problem may lie in commit 44c02a2c3dc5
>> ("dev_ioctl(): move copyin/copyout to callers") which moves changes
>> the network dev_ioctl() function to always call dev_load(),
>> regardless of the requested ioctl.
>>
>> This patch moves the dev_load() calls back into the individual ioctls
>> while preserving the rest of the original patch.
>>
>> Reported-by: Dominick Grift 
>> Suggested-by: Stephen Smalley 
>> Signed-off-by: Paul Moore 
>> ---
>>  net/core/dev_ioctl.c |7 +--
>>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> In the interest of full disclosure, I've compiled this code but I
> haven't booted it yet (test kernel building now).  I just wanted to
> post this sooner rather than later in case the networking folks, or
> Al, had a different solution they would prefer.

This is definitely the right fix, so patch applied.

Taking the dev_load() out of that switch statement definitely has
side effects.

Re: [PATCH] net: don't unnecessarily load kernel modules in dev_ioctl()

[Paul Moore]

On Tue, Mar 6, 2018 at 6:59 PM, Stephen Hemminger
 wrote:
> On Tue, 06 Mar 2018 17:27:44 -0500
> Paul Moore  wrote:
>> From: Paul Moore 
>>
>> Starting with v4.16-rc1 we've been seeing a higher than usual number
>> of requests for the kernel to load networking modules, even on events
>> which shouldn't trigger a module load (e.g. ioctl(TCGETS)).  Stephen
>> Smalley suggested the problem may lie in commit 44c02a2c3dc5
>> ("dev_ioctl(): move copyin/copyout to callers") which moves changes
>> the network dev_ioctl() function to always call dev_load(),
>> regardless of the requested ioctl.
>>
>> This patch moves the dev_load() calls back into the individual ioctls
>> while preserving the rest of the original patch.
>>
>> Reported-by: Dominick Grift 
>> Suggested-by: Stephen Smalley 
>> Signed-off-by: Paul Moore 
>> ---
>>  net/core/dev_ioctl.c |7 +--
>>  1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/net/core/dev_ioctl.c b/net/core/dev_ioctl.c
>> index 0ab1af04296c..a04e1e88bf3a 100644
>> --- a/net/core/dev_ioctl.c
>> +++ b/net/core/dev_ioctl.c
>> @@ -402,8 +402,6 @@ int dev_ioctl(struct net *net, unsigned int cmd, struct 
>> ifreq *ifr, bool *need_c
>>   if (colon)
>>   *colon = 0;
>>
>> - dev_load(net, ifr->ifr_name);
>
> Actually dev_load by ethernet name is really a legacy thing that should just 
> die,
>
> It was kept around so that some very tunnel configuration using special names.
>
> # ifconfig sit0
>
> which probably several web pages still tell users to do...
> We have much better control now with ip commands so that this is just
> baggage.

In an effort to get this regression fixed quickly, and not get tangled
up in a user education issue, can we at least restore the old ioctl()
behavior and worry about removing dev_load() later?

-- 
paul moore
www.paul-moore.com

Re: [PATCH] net: don't unnecessarily load kernel modules in dev_ioctl()

[Stephen Hemminger]

On Tue, 06 Mar 2018 17:27:44 -0500
Paul Moore  wrote:

> From: Paul Moore 
> 
> Starting with v4.16-rc1 we've been seeing a higher than usual number
> of requests for the kernel to load networking modules, even on events
> which shouldn't trigger a module load (e.g. ioctl(TCGETS)).  Stephen
> Smalley suggested the problem may lie in commit 44c02a2c3dc5
> ("dev_ioctl(): move copyin/copyout to callers") which moves changes
> the network dev_ioctl() function to always call dev_load(),
> regardless of the requested ioctl.
> 
> This patch moves the dev_load() calls back into the individual ioctls
> while preserving the rest of the original patch.
> 
> Reported-by: Dominick Grift 
> Suggested-by: Stephen Smalley 
> Signed-off-by: Paul Moore 
> ---
>  net/core/dev_ioctl.c |7 +--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/net/core/dev_ioctl.c b/net/core/dev_ioctl.c
> index 0ab1af04296c..a04e1e88bf3a 100644
> --- a/net/core/dev_ioctl.c
> +++ b/net/core/dev_ioctl.c
> @@ -402,8 +402,6 @@ int dev_ioctl(struct net *net, unsigned int cmd, struct 
> ifreq *ifr, bool *need_c
>   if (colon)
>   *colon = 0;
>  
> - dev_load(net, ifr->ifr_name);

Actually dev_load by ethernet name is really a legacy thing that should just 
die,

It was kept around so that some very tunnel configuration using special names.

# ifconfig sit0

which probably several web pages still tell users to do...
We have much better control now with ip commands so that this is just
baggage.

Re: [PATCH] net: don't unnecessarily load kernel modules in dev_ioctl()

[Paul Moore]

On Tue, Mar 6, 2018 at 5:27 PM, Paul Moore  wrote:
> From: Paul Moore 
>
> Starting with v4.16-rc1 we've been seeing a higher than usual number
> of requests for the kernel to load networking modules, even on events
> which shouldn't trigger a module load (e.g. ioctl(TCGETS)).  Stephen
> Smalley suggested the problem may lie in commit 44c02a2c3dc5
> ("dev_ioctl(): move copyin/copyout to callers") which moves changes
> the network dev_ioctl() function to always call dev_load(),
> regardless of the requested ioctl.
>
> This patch moves the dev_load() calls back into the individual ioctls
> while preserving the rest of the original patch.
>
> Reported-by: Dominick Grift 
> Suggested-by: Stephen Smalley 
> Signed-off-by: Paul Moore 
> ---
>  net/core/dev_ioctl.c |7 +--
>  1 file changed, 5 insertions(+), 2 deletions(-)

In the interest of full disclosure, I've compiled this code but I
haven't booted it yet (test kernel building now).  I just wanted to
post this sooner rather than later in case the networking folks, or
Al, had a different solution they would prefer.

> diff --git a/net/core/dev_ioctl.c b/net/core/dev_ioctl.c
> index 0ab1af04296c..a04e1e88bf3a 100644
> --- a/net/core/dev_ioctl.c
> +++ b/net/core/dev_ioctl.c
> @@ -402,8 +402,6 @@ int dev_ioctl(struct net *net, unsigned int cmd, struct 
> ifreq *ifr, bool *need_c
> if (colon)
> *colon = 0;
>
> -   dev_load(net, ifr->ifr_name);
> -
> /*
>  *  See which interface the caller is talking about.
>  */
> @@ -423,6 +421,7 @@ int dev_ioctl(struct net *net, unsigned int cmd, struct 
> ifreq *ifr, bool *need_c
> case SIOCGIFMAP:
> case SIOCGIFINDEX:
> case SIOCGIFTXQLEN:
> +   dev_load(net, ifr->ifr_name);
> rcu_read_lock();
> ret = dev_ifsioc_locked(net, ifr, cmd);
> rcu_read_unlock();
> @@ -431,6 +430,7 @@ int dev_ioctl(struct net *net, unsigned int cmd, struct 
> ifreq *ifr, bool *need_c
> return ret;
>
> case SIOCETHTOOL:
> +   dev_load(net, ifr->ifr_name);
> rtnl_lock();
> ret = dev_ethtool(net, ifr);
> rtnl_unlock();
> @@ -447,6 +447,7 @@ int dev_ioctl(struct net *net, unsigned int cmd, struct 
> ifreq *ifr, bool *need_c
> case SIOCGMIIPHY:
> case SIOCGMIIREG:
> case SIOCSIFNAME:
> +   dev_load(net, ifr->ifr_name);
> if (!ns_capable(net->user_ns, CAP_NET_ADMIN))
> return -EPERM;
> rtnl_lock();
> @@ -494,6 +495,7 @@ int dev_ioctl(struct net *net, unsigned int cmd, struct 
> ifreq *ifr, bool *need_c
> /* fall through */
> case SIOCBONDSLAVEINFOQUERY:
> case SIOCBONDINFOQUERY:
> +   dev_load(net, ifr->ifr_name);
> rtnl_lock();
> ret = dev_ifsioc(net, ifr, cmd);
> rtnl_unlock();
> @@ -518,6 +520,7 @@ int dev_ioctl(struct net *net, unsigned int cmd, struct 
> ifreq *ifr, bool *need_c
> cmd == SIOCGHWTSTAMP ||
> (cmd >= SIOCDEVPRIVATE &&
>  cmd <= SIOCDEVPRIVATE + 15)) {
> +   dev_load(net, ifr->ifr_name);
> rtnl_lock();
> ret = dev_ifsioc(net, ifr, cmd);
> rtnl_unlock();
>
> --
> To unsubscribe from this list: send the line "unsubscribe 
> linux-security-module" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
paul moore
www.paul-moore.com