Re: [PATCH v2 net 13/17] net: Fix data-races around sysctl_fb_tunnels_only_for_init_net.

Re: [PATCH v2 net 13/17] net: Fix data-races around sysctl_fb_tunnels_only_for_init_net.

[kernel test robot]

Hi Kuniyuki,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on net/master]

url:    https://github.com/intel-lab-lkp/linux/commits/Kuniyuki-Iwashima/net-sysctl-Fix-data-races-around-net-core-XXX/20220818-115941
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net.git fc4aaf9fb3c99bcb326d52f9d320ed5680bd1cee
config: riscv-randconfig-r032-20220818 (https://download.01.org/0day-ci/archive/20220818/202208181615.Lu9xjiEv-lkp@intel.com/config)
compiler: clang version 16.0.0 (https://github.com/llvm/llvm-project aed5e3bea138ce581d682158eb61c27b3cfdd6ec)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install riscv cross compiling tool for clang build
        # apt-get install binutils-riscv64-linux-gnu
        # https://github.com/intel-lab-lkp/linux/commit/6bc3dfb3dc4862f4e00ba93c45cd5c0251b85d5b
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Kuniyuki-Iwashima/net-sysctl-Fix-data-races-around-net-core-XXX/20220818-115941
        git checkout 6bc3dfb3dc4862f4e00ba93c45cd5c0251b85d5b
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=riscv SHELL=/bin/bash

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> ld.lld: error: undefined symbol: sysctl_fb_tunnels_only_for_init_net
   >>> referenced by ip_tunnel.c
   >>>               ipv4/ip_tunnel.o:(ip_tunnel_init_net) in archive net/built-in.a
   >>> referenced by ip_tunnel.c
   >>>               ipv4/ip_tunnel.o:(ip_tunnel_init_net) in archive net/built-in.a

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

Re: [PATCH v2 net 13/17] net: Fix data-races around sysctl_fb_tunnels_only_for_init_net.

[Kuniyuki Iwashima]

From:   kernel test robot <lkp@intel.com>
Date:   Thu, 18 Aug 2022 16:51:37 +0800
> Hi Kuniyuki,
> 
> Thank you for the patch! Yet something to improve:
> 
> [auto build test ERROR on net/master]
> 
> url:    https://github.com/intel-lab-lkp/linux/commits/Kuniyuki-Iwashima/net-sysctl-Fix-data-races-around-net-core-XXX/20220818-115941
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net.git fc4aaf9fb3c99bcb326d52f9d320ed5680bd1cee
> config: riscv-randconfig-r032-20220818 (https://download.01.org/0day-ci/archive/20220818/202208181615.Lu9xjiEv-lkp@intel.com/config)
> compiler: clang version 16.0.0 (https://github.com/llvm/llvm-project aed5e3bea138ce581d682158eb61c27b3cfdd6ec)
> reproduce (this is a W=1 build):
>         wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
>         chmod +x ~/bin/make.cross
>         # install riscv cross compiling tool for clang build
>         # apt-get install binutils-riscv64-linux-gnu
>         # https://github.com/intel-lab-lkp/linux/commit/6bc3dfb3dc4862f4e00ba93c45cd5c0251b85d5b
>         git remote add linux-review https://github.com/intel-lab-lkp/linux
>         git fetch --no-tags linux-review Kuniyuki-Iwashima/net-sysctl-Fix-data-races-around-net-core-XXX/20220818-115941
>         git checkout 6bc3dfb3dc4862f4e00ba93c45cd5c0251b85d5b
>         # save the config file
>         mkdir build_dir && cp config build_dir/.config
>         COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=riscv SHELL=/bin/bash
> 
> If you fix the issue, kindly add following tag where applicable
> Reported-by: kernel test robot <lkp@intel.com>
> 
> All errors (new ones prefixed by >>):
> 
> >> ld.lld: error: undefined symbol: sysctl_fb_tunnels_only_for_init_net
>    >>> referenced by ip_tunnel.c
>    >>>               ipv4/ip_tunnel.o:(ip_tunnel_init_net) in archive net/built-in.a
>    >>> referenced by ip_tunnel.c
>    >>>               ipv4/ip_tunnel.o:(ip_tunnel_init_net) in archive net/built-in.a

Hmm... I tested allmodconfig with x86_64 but it seems not enough...

I don't think just using READ_ONCE() causes regression.
Is this really regression or always-broken stuff in some arch,
or ... clang 16?

Anyway, I'll take a look.

Re: [PATCH v2 net 13/17] net: Fix data-races around sysctl_fb_tunnels_only_for_init_net.

[Nathan Chancellor]

On Thu, Aug 18, 2022 at 08:01:54AM -0700, Kuniyuki Iwashima wrote:
> From:   kernel test robot <lkp@intel.com>
> Date:   Thu, 18 Aug 2022 16:51:37 +0800
> > Hi Kuniyuki,
> > 
> > Thank you for the patch! Yet something to improve:
> > 
> > [auto build test ERROR on net/master]
> > 
> > url:    https://github.com/intel-lab-lkp/linux/commits/Kuniyuki-Iwashima/net-sysctl-Fix-data-races-around-net-core-XXX/20220818-115941
> > base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net.git fc4aaf9fb3c99bcb326d52f9d320ed5680bd1cee
> > config: riscv-randconfig-r032-20220818 (https://download.01.org/0day-ci/archive/20220818/202208181615.Lu9xjiEv-lkp@intel.com/config)
> > compiler: clang version 16.0.0 (https://github.com/llvm/llvm-project aed5e3bea138ce581d682158eb61c27b3cfdd6ec)
> > reproduce (this is a W=1 build):
> >         wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
> >         chmod +x ~/bin/make.cross
> >         # install riscv cross compiling tool for clang build
> >         # apt-get install binutils-riscv64-linux-gnu
> >         # https://github.com/intel-lab-lkp/linux/commit/6bc3dfb3dc4862f4e00ba93c45cd5c0251b85d5b
> >         git remote add linux-review https://github.com/intel-lab-lkp/linux
> >         git fetch --no-tags linux-review Kuniyuki-Iwashima/net-sysctl-Fix-data-races-around-net-core-XXX/20220818-115941
> >         git checkout 6bc3dfb3dc4862f4e00ba93c45cd5c0251b85d5b
> >         # save the config file
> >         mkdir build_dir && cp config build_dir/.config
> >         COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=riscv SHELL=/bin/bash
> > 
> > If you fix the issue, kindly add following tag where applicable
> > Reported-by: kernel test robot <lkp@intel.com>
> > 
> > All errors (new ones prefixed by >>):
> > 
> > >> ld.lld: error: undefined symbol: sysctl_fb_tunnels_only_for_init_net
> >    >>> referenced by ip_tunnel.c
> >    >>>               ipv4/ip_tunnel.o:(ip_tunnel_init_net) in archive net/built-in.a
> >    >>> referenced by ip_tunnel.c
> >    >>>               ipv4/ip_tunnel.o:(ip_tunnel_init_net) in archive net/built-in.a
> 
> Hmm... I tested allmodconfig with x86_64 but it seems not enough...
> 
> I don't think just using READ_ONCE() causes regression.
> Is this really regression or always-broken stuff in some arch,
> or ... clang 16?
> 
> Anyway, I'll take a look.

You'll see the same error with the same configuration and GCC:

riscv64-linux-gnu-ld: net/ipv4/ip_tunnel.o: in function `.L536':
ip_tunnel.c:(.text+0x1d4a): undefined reference to `sysctl_fb_tunnels_only_for_init_net'
riscv64-linux-gnu-ld: ip_tunnel.c:(.text+0x1d4e): undefined reference to `sysctl_fb_tunnels_only_for_init_net'

$ scripts/config --file build/riscv/.config -s SYSCTL
undef

Prior to your change, the '!IS_ENABLED(CONFIG_SYSCTL)' would cause
net_has_fallback_tunnels() to unconditionally 'return 1' in the
CONFIG_SYSCTL=n case, meaning 'fb_tunnels_only_for_init_net' was never
emitted in the final assembly so the linker would not complain about it
never being defined (the kernel relies on this trick a lot, which is why
you cannot build the kernel with -O0).

After your change, sysctl_fb_tunnels_only_for_init_net is
unconditionally used but it is only defined in sysctl_net_core.c, which
is only built when CONFIG_SYSCTL=y, hence the link error.

I suspect hoisting '!IS_ENABLED(CONFIG_SYSCTL)' out of the return into
its own conditional would fix the error:

  static inline bool net_has_fallback_tunnels(const struct net *net)
  {
      int fb_tunnels_only_for_init_net;

      if (!IS_ENABLED(CONFIG_SYSCTL))
          return true;

      fb_tunnels_only_for_init_net = READ_ONCE(sysctl_fb_tunnels_only_for_init_net);

      return !fb_tunnels_only_for_init_net ||
             (net == &init_net && fb_tunnels_only_for_init_net == 1)
  }

Cheers,
Nathan

Re: [PATCH v2 net 13/17] net: Fix data-races around sysctl_fb_tunnels_only_for_init_net.

[Kuniyuki Iwashima]

From:   Nathan Chancellor <nathan@kernel.org>
Date:   Thu, 18 Aug 2022 09:23:19 -0700
> On Thu, Aug 18, 2022 at 08:01:54AM -0700, Kuniyuki Iwashima wrote:
> > From:   kernel test robot <lkp@intel.com>
> > Date:   Thu, 18 Aug 2022 16:51:37 +0800
> > > Hi Kuniyuki,
> > > 
> > > Thank you for the patch! Yet something to improve:
> > > 
> > > [auto build test ERROR on net/master]
> > > 
> > > url:    https://github.com/intel-lab-lkp/linux/commits/Kuniyuki-Iwashima/net-sysctl-Fix-data-races-around-net-core-XXX/20220818-115941
> > > base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net.git fc4aaf9fb3c99bcb326d52f9d320ed5680bd1cee
> > > config: riscv-randconfig-r032-20220818 (https://download.01.org/0day-ci/archive/20220818/202208181615.Lu9xjiEv-lkp@intel.com/config)
> > > compiler: clang version 16.0.0 (https://github.com/llvm/llvm-project aed5e3bea138ce581d682158eb61c27b3cfdd6ec)
> > > reproduce (this is a W=1 build):
> > >         wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
> > >         chmod +x ~/bin/make.cross
> > >         # install riscv cross compiling tool for clang build
> > >         # apt-get install binutils-riscv64-linux-gnu
> > >         # https://github.com/intel-lab-lkp/linux/commit/6bc3dfb3dc4862f4e00ba93c45cd5c0251b85d5b
> > >         git remote add linux-review https://github.com/intel-lab-lkp/linux
> > >         git fetch --no-tags linux-review Kuniyuki-Iwashima/net-sysctl-Fix-data-races-around-net-core-XXX/20220818-115941
> > >         git checkout 6bc3dfb3dc4862f4e00ba93c45cd5c0251b85d5b
> > >         # save the config file
> > >         mkdir build_dir && cp config build_dir/.config
> > >         COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=riscv SHELL=/bin/bash
> > > 
> > > If you fix the issue, kindly add following tag where applicable
> > > Reported-by: kernel test robot <lkp@intel.com>
> > > 
> > > All errors (new ones prefixed by >>):
> > > 
> > > >> ld.lld: error: undefined symbol: sysctl_fb_tunnels_only_for_init_net
> > >    >>> referenced by ip_tunnel.c
> > >    >>>               ipv4/ip_tunnel.o:(ip_tunnel_init_net) in archive net/built-in.a
> > >    >>> referenced by ip_tunnel.c
> > >    >>>               ipv4/ip_tunnel.o:(ip_tunnel_init_net) in archive net/built-in.a
> > 
> > Hmm... I tested allmodconfig with x86_64 but it seems not enough...
> > 
> > I don't think just using READ_ONCE() causes regression.
> > Is this really regression or always-broken stuff in some arch,
> > or ... clang 16?
> > 
> > Anyway, I'll take a look.
> 
> You'll see the same error with the same configuration and GCC:
> 
> riscv64-linux-gnu-ld: net/ipv4/ip_tunnel.o: in function `.L536':
> ip_tunnel.c:(.text+0x1d4a): undefined reference to `sysctl_fb_tunnels_only_for_init_net'
> riscv64-linux-gnu-ld: ip_tunnel.c:(.text+0x1d4e): undefined reference to `sysctl_fb_tunnels_only_for_init_net'
> 
> $ scripts/config --file build/riscv/.config -s SYSCTL
> undef
> 
> Prior to your change, the '!IS_ENABLED(CONFIG_SYSCTL)' would cause
> net_has_fallback_tunnels() to unconditionally 'return 1' in the
> CONFIG_SYSCTL=n case,

Yes, you are right.
I've just noticed it and this fixed the error.
Also, I did the same mistake in the 14th patch...
Thank you, Nathan!

---8<---
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 2563d30736e9..78dd63a5c7c8 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -640,9 +640,14 @@ extern int sysctl_devconf_inherit_init_net;
  */
 static inline bool net_has_fallback_tunnels(const struct net *net)
 {
-	return !IS_ENABLED(CONFIG_SYSCTL) ||
-	       !sysctl_fb_tunnels_only_for_init_net ||
-	       (net == &init_net && sysctl_fb_tunnels_only_for_init_net == 1);
+#if IS_ENABLED(CONFIG_SYSCTL)
+	int fb_tunnels_only_for_init_net = READ_ONCE(sysctl_fb_tunnels_only_for_init_net);
+
+	return !fb_tunnels_only_for_init_net ||
+		(net_eq(net, &init_net) && fb_tunnels_only_for_init_net == 1);
+#else
+	return true;
+#endif
 }
 
 static inline int netdev_queue_numa_node_read(const struct netdev_queue *q)
---8<---


> meaning 'fb_tunnels_only_for_init_net' was never
> emitted in the final assembly so the linker would not complain about it
> never being defined (the kernel relies on this trick a lot, which is why
> you cannot build the kernel with -O0).
> 
> After your change, sysctl_fb_tunnels_only_for_init_net is
> unconditionally used but it is only defined in sysctl_net_core.c, which
> is only built when CONFIG_SYSCTL=y, hence the link error.
> 
> I suspect hoisting '!IS_ENABLED(CONFIG_SYSCTL)' out of the return into
> its own conditional would fix the error:
> 
>   static inline bool net_has_fallback_tunnels(const struct net *net)
>   {
>       int fb_tunnels_only_for_init_net;
> 
>       if (!IS_ENABLED(CONFIG_SYSCTL))
>           return true;
> 
>       fb_tunnels_only_for_init_net = READ_ONCE(sysctl_fb_tunnels_only_for_init_net);
> 
>       return !fb_tunnels_only_for_init_net ||
>              (net == &init_net && fb_tunnels_only_for_init_net == 1)
>   }
> 
> Cheers,
> Nathan