From: Markus Elfring <[email protected]>
Date: Sun, 31 Dec 2023 18:26:38 +0100
A few update suggestions were taken into account
from static source code analysis.
Markus Elfring (4):
Improve exception handling
Move an assignment for the variable “sk”
Delete an unnecessary variable initialisation
Move an assignment for the variable “cb_mutex”
net/netlink/af_netlink.c | 11 ++++++-----
1 file changed, 6 insertions(+), 5 deletions(-)
--
2.43.0
From: Markus Elfring <[email protected]>
Date: Sun, 31 Dec 2023 17:26:41 +0100
The kfree() function was called in one case by
the __netlink_kernel_create() function during error handling
even if the passed variable contained a null pointer.
This issue was detected by using the Coccinelle software.
Thus use another label.
Signed-off-by: Markus Elfring <[email protected]>
---
net/netlink/af_netlink.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
index 4ed8ffd58ff3..c3f88015cacf 100644
--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -2042,7 +2042,7 @@ __netlink_kernel_create(struct net *net, int unit, struct module *module,
listeners = kzalloc(sizeof(*listeners) + NLGRPSZ(groups), GFP_KERNEL);
if (!listeners)
- goto out_sock_release;
+ goto out_netlink_release_sock;
sk->sk_data_ready = netlink_data_ready;
if (cfg && cfg->input)
@@ -2076,6 +2076,7 @@ __netlink_kernel_create(struct net *net, int unit, struct module *module,
out_sock_release:
kfree(listeners);
+out_netlink_release_sock:
netlink_kernel_release(sk);
return NULL;
--
2.43.0
From: Markus Elfring <[email protected]>
Date: Sun, 31 Dec 2023 17:36:50 +0100
Move one assignment for the variable “sk” closer to the place
where this pointer is used.
Signed-off-by: Markus Elfring <[email protected]>
---
net/netlink/af_netlink.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
index c3f88015cacf..b71d9c21d15b 100644
--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -2033,8 +2033,6 @@ __netlink_kernel_create(struct net *net, int unit, struct module *module,
if (__netlink_create(net, sock, cb_mutex, unit, 1) < 0)
goto out_sock_release_nosk;
- sk = sock->sk;
-
if (!cfg || cfg->groups < 32)
groups = 32;
else
@@ -2044,6 +2042,7 @@ __netlink_kernel_create(struct net *net, int unit, struct module *module,
if (!listeners)
goto out_netlink_release_sock;
+ sk = sock->sk;
sk->sk_data_ready = netlink_data_ready;
if (cfg && cfg->input)
nlk_sk(sk)->netlink_rcv = cfg->input;
--
2.43.0
From: Markus Elfring <[email protected]>
Date: Sun, 31 Dec 2023 17:45:00 +0100
The variable “listeners” will eventually be set to an appropriate pointer
a bit later. Thus omit the explicit initialisation at the beginning.
Signed-off-by: Markus Elfring <[email protected]>
---
net/netlink/af_netlink.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
index b71d9c21d15b..cfddc9c6a376 100644
--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -2018,7 +2018,7 @@ __netlink_kernel_create(struct net *net, int unit, struct module *module,
struct socket *sock;
struct sock *sk;
struct netlink_sock *nlk;
- struct listeners *listeners = NULL;
+ struct listeners *listeners;
struct mutex *cb_mutex = cfg ? cfg->cb_mutex : NULL;
unsigned int groups;
--
2.43.0
From: Markus Elfring <[email protected]>
Date: Sun, 31 Dec 2023 18:16:26 +0100
Move one assignment for the variable “cb_mutex” closer to the place
where this pointer is used.
Signed-off-by: Markus Elfring <[email protected]>
---
net/netlink/af_netlink.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
index cfddc9c6a376..12a0b6f8be19 100644
--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -2019,7 +2019,7 @@ __netlink_kernel_create(struct net *net, int unit, struct module *module,
struct sock *sk;
struct netlink_sock *nlk;
struct listeners *listeners;
- struct mutex *cb_mutex = cfg ? cfg->cb_mutex : NULL;
+ struct mutex *cb_mutex; /* Serialize data processing with callbacks */
unsigned int groups;
BUG_ON(!nl_table);
@@ -2030,6 +2030,7 @@ __netlink_kernel_create(struct net *net, int unit, struct module *module,
if (sock_create_lite(PF_NETLINK, SOCK_DGRAM, unit, &sock))
return NULL;
+ cb_mutex = (cfg ? cfg->cb_mutex : NULL);
if (__netlink_create(net, sock, cb_mutex, unit, 1) < 0)
goto out_sock_release_nosk;
--
2.43.0
Hi Markus,
kernel test robot noticed the following build warnings:
[auto build test WARNING on net/main]
[also build test WARNING on net-next/main linus/master horms-ipvs/master v6.7-rc8 next-20231222]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Markus-Elfring/netlink-Improve-exception-handling-in-__netlink_kernel_create/20240101-015025
base: net/main
patch link: https://lore.kernel.org/r/223a61e9-f826-4f37-b514-ca6ed53b1269%40web.de
patch subject: [PATCH 2/4] netlink: Move an assignment for the variable “sk” in __netlink_kernel_create()
config: arm-randconfig-001-20240101 (https://download.01.org/0day-ci/archive/20240101/[email protected]/config)
compiler: clang version 18.0.0git (https://github.com/llvm/llvm-project 8a4266a626914765c0c69839e8a51be383013c1a)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240101/[email protected]/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All warnings (new ones prefixed by >>):
>> net/netlink/af_netlink.c:2044:6: warning: variable 'sk' is used uninitialized whenever 'if' condition is true [-Wsometimes-uninitialized]
2044 | if (!listeners)
| ^~~~~~~~~~
net/netlink/af_netlink.c:2081:25: note: uninitialized use occurs here
2081 | netlink_kernel_release(sk);
| ^~
net/netlink/af_netlink.c:2044:2: note: remove the 'if' if its condition is always false
2044 | if (!listeners)
| ^~~~~~~~~~~~~~~
2045 | goto out_netlink_release_sock;
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
net/netlink/af_netlink.c:2021:17: note: initialize the variable 'sk' to silence this warning
2021 | struct sock *sk;
| ^
| = NULL
1 warning generated.
vim +2044 net/netlink/af_netlink.c
^1da177e4c3f41 Linus Torvalds 2005-04-16 2009
^1da177e4c3f41 Linus Torvalds 2005-04-16 2010 /*
^1da177e4c3f41 Linus Torvalds 2005-04-16 2011 * We export these functions to other modules. They provide a
^1da177e4c3f41 Linus Torvalds 2005-04-16 2012 * complete set of kernel non-blocking support for message
^1da177e4c3f41 Linus Torvalds 2005-04-16 2013 * queueing.
^1da177e4c3f41 Linus Torvalds 2005-04-16 2014 */
^1da177e4c3f41 Linus Torvalds 2005-04-16 2015
^1da177e4c3f41 Linus Torvalds 2005-04-16 2016 struct sock *
9f00d9776bc5be Pablo Neira Ayuso 2012-09-08 2017 __netlink_kernel_create(struct net *net, int unit, struct module *module,
a31f2d17b331db Pablo Neira Ayuso 2012-06-29 2018 struct netlink_kernel_cfg *cfg)
^1da177e4c3f41 Linus Torvalds 2005-04-16 2019 {
^1da177e4c3f41 Linus Torvalds 2005-04-16 2020 struct socket *sock;
^1da177e4c3f41 Linus Torvalds 2005-04-16 2021 struct sock *sk;
77247bbb309424 Patrick McHardy 2005-08-14 2022 struct netlink_sock *nlk;
5c398dc8f5a58b Eric Dumazet 2010-10-24 2023 struct listeners *listeners = NULL;
a31f2d17b331db Pablo Neira Ayuso 2012-06-29 2024 struct mutex *cb_mutex = cfg ? cfg->cb_mutex : NULL;
a31f2d17b331db Pablo Neira Ayuso 2012-06-29 2025 unsigned int groups;
^1da177e4c3f41 Linus Torvalds 2005-04-16 2026
fab2caf62ed03d Akinobu Mita 2006-08-29 2027 BUG_ON(!nl_table);
^1da177e4c3f41 Linus Torvalds 2005-04-16 2028
^1da177e4c3f41 Linus Torvalds 2005-04-16 2029 if (unit < 0 || unit >= MAX_LINKS)
^1da177e4c3f41 Linus Torvalds 2005-04-16 2030 return NULL;
^1da177e4c3f41 Linus Torvalds 2005-04-16 2031
^1da177e4c3f41 Linus Torvalds 2005-04-16 2032 if (sock_create_lite(PF_NETLINK, SOCK_DGRAM, unit, &sock))
^1da177e4c3f41 Linus Torvalds 2005-04-16 2033 return NULL;
13d3078e22f565 Eric W. Biederman 2015-05-08 2034
13d3078e22f565 Eric W. Biederman 2015-05-08 2035 if (__netlink_create(net, sock, cb_mutex, unit, 1) < 0)
23fe18669e7fda Pavel Emelyanov 2008-01-30 2036 goto out_sock_release_nosk;
23fe18669e7fda Pavel Emelyanov 2008-01-30 2037
a31f2d17b331db Pablo Neira Ayuso 2012-06-29 2038 if (!cfg || cfg->groups < 32)
4277a083ecd2c8 Patrick McHardy 2006-03-20 2039 groups = 32;
a31f2d17b331db Pablo Neira Ayuso 2012-06-29 2040 else
a31f2d17b331db Pablo Neira Ayuso 2012-06-29 2041 groups = cfg->groups;
4277a083ecd2c8 Patrick McHardy 2006-03-20 2042
5c398dc8f5a58b Eric Dumazet 2010-10-24 2043 listeners = kzalloc(sizeof(*listeners) + NLGRPSZ(groups), GFP_KERNEL);
4277a083ecd2c8 Patrick McHardy 2006-03-20 @2044 if (!listeners)
90ae404765d263 Markus Elfring 2023-12-31 2045 goto out_netlink_release_sock;
4277a083ecd2c8 Patrick McHardy 2006-03-20 2046
e547df909ec09d Markus Elfring 2023-12-31 2047 sk = sock->sk;
^1da177e4c3f41 Linus Torvalds 2005-04-16 2048 sk->sk_data_ready = netlink_data_ready;
a31f2d17b331db Pablo Neira Ayuso 2012-06-29 2049 if (cfg && cfg->input)
a31f2d17b331db Pablo Neira Ayuso 2012-06-29 2050 nlk_sk(sk)->netlink_rcv = cfg->input;
^1da177e4c3f41 Linus Torvalds 2005-04-16 2051
8ea65f4a2dfaaf Herbert Xu 2015-01-26 2052 if (netlink_insert(sk, 0))
77247bbb309424 Patrick McHardy 2005-08-14 2053 goto out_sock_release;
4fdb3bb723db46 Harald Welte 2005-08-09 2054
77247bbb309424 Patrick McHardy 2005-08-14 2055 nlk = nlk_sk(sk);
8fe08d70a2b61b Eric Dumazet 2023-08-11 2056 set_bit(NETLINK_F_KERNEL_SOCKET, &nlk->flags);
4fdb3bb723db46 Harald Welte 2005-08-09 2057
4fdb3bb723db46 Harald Welte 2005-08-09 2058 netlink_table_grab();
b4b510290b056b Eric W. Biederman 2007-09-12 2059 if (!nl_table[unit].registered) {
4277a083ecd2c8 Patrick McHardy 2006-03-20 2060 nl_table[unit].groups = groups;
5c398dc8f5a58b Eric Dumazet 2010-10-24 2061 rcu_assign_pointer(nl_table[unit].listeners, listeners);
af65bdfce98d79 Patrick McHardy 2007-04-20 2062 nl_table[unit].cb_mutex = cb_mutex;
77247bbb309424 Patrick McHardy 2005-08-14 2063 nl_table[unit].module = module;
9785e10aedfa0f Pablo Neira Ayuso 2012-09-08 2064 if (cfg) {
9785e10aedfa0f Pablo Neira Ayuso 2012-09-08 2065 nl_table[unit].bind = cfg->bind;
6251edd932ce3f Hiroaki SHIMODA 2014-11-13 2066 nl_table[unit].unbind = cfg->unbind;
a4c9a56e6a2cde Anjali Kulkarni 2023-07-19 2067 nl_table[unit].release = cfg->release;
9785e10aedfa0f Pablo Neira Ayuso 2012-09-08 2068 nl_table[unit].flags = cfg->flags;
9785e10aedfa0f Pablo Neira Ayuso 2012-09-08 2069 }
ab33a1711cf60b Patrick McHardy 2005-08-14 2070 nl_table[unit].registered = 1;
f937f1f46b6d2f Jesper Juhl 2007-10-15 2071 } else {
f937f1f46b6d2f Jesper Juhl 2007-10-15 2072 kfree(listeners);
869e58f87094b1 Denis V. Lunev 2008-01-18 2073 nl_table[unit].registered++;
b4b510290b056b Eric W. Biederman 2007-09-12 2074 }
4fdb3bb723db46 Harald Welte 2005-08-09 2075 netlink_table_ungrab();
77247bbb309424 Patrick McHardy 2005-08-14 2076 return sk;
77247bbb309424 Patrick McHardy 2005-08-14 2077
4fdb3bb723db46 Harald Welte 2005-08-09 2078 out_sock_release:
4277a083ecd2c8 Patrick McHardy 2006-03-20 2079 kfree(listeners);
90ae404765d263 Markus Elfring 2023-12-31 2080 out_netlink_release_sock:
9dfbec1fb2bedf Denis V. Lunev 2008-02-29 2081 netlink_kernel_release(sk);
23fe18669e7fda Pavel Emelyanov 2008-01-30 2082 return NULL;
23fe18669e7fda Pavel Emelyanov 2008-01-30 2083
23fe18669e7fda Pavel Emelyanov 2008-01-30 2084 out_sock_release_nosk:
4fdb3bb723db46 Harald Welte 2005-08-09 2085 sock_release(sock);
77247bbb309424 Patrick McHardy 2005-08-14 2086 return NULL;
^1da177e4c3f41 Linus Torvalds 2005-04-16 2087 }
9f00d9776bc5be Pablo Neira Ayuso 2012-09-08 2088 EXPORT_SYMBOL(__netlink_kernel_create);
b7c6ba6eb1234e Denis V. Lunev 2008-01-28 2089
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
…
> url: https://github.com/intel-lab-lkp/linux/commits/Markus-Elfring/netlink-Improve-exception-handling-in-__netlink_kernel_create/20240101-015025
…
> patch link: https://lore.kernel.org/r/223a61e9-f826-4f37-b514-ca6ed53b1269%40web.de
…
> All warnings (new ones prefixed by >>):
>
>>> net/netlink/af_netlink.c:2044:6: warning: variable 'sk' is used uninitialized whenever 'if' condition is true [-Wsometimes-uninitialized]
…
>
…
> ^1da177e4c3f41 Linus Torvalds 2005-04-16 2016 struct sock *
> 9f00d9776bc5be Pablo Neira Ayuso 2012-09-08 2017 __netlink_kernel_create(struct net *net, int unit, struct module *module,
> a31f2d17b331db Pablo Neira Ayuso 2012-06-29 2018 struct netlink_kernel_cfg *cfg)
> ^1da177e4c3f41 Linus Torvalds 2005-04-16 2019 {
> ^1da177e4c3f41 Linus Torvalds 2005-04-16 2020 struct socket *sock;
> ^1da177e4c3f41 Linus Torvalds 2005-04-16 2021 struct sock *sk;
…
> 5c398dc8f5a58b Eric Dumazet 2010-10-24 2023 struct listeners *listeners = NULL;
…
> ^1da177e4c3f41 Linus Torvalds 2005-04-16 2032 if (sock_create_lite(PF_NETLINK, SOCK_DGRAM, unit, &sock))
> ^1da177e4c3f41 Linus Torvalds 2005-04-16 2033 return NULL;
…
> 5c398dc8f5a58b Eric Dumazet 2010-10-24 2043 listeners = kzalloc(sizeof(*listeners) + NLGRPSZ(groups), GFP_KERNEL);
> 4277a083ecd2c8 Patrick McHardy 2006-03-20 @2044 if (!listeners)
> 90ae404765d263 Markus Elfring 2023-12-31 2045 goto out_netlink_release_sock;
> 4277a083ecd2c8 Patrick McHardy 2006-03-20 2046
> e547df909ec09d Markus Elfring 2023-12-31 2047 sk = sock->sk;
> ^1da177e4c3f41 Linus Torvalds 2005-04-16 2048 sk->sk_data_ready = netlink_data_ready;
…
> 4fdb3bb723db46 Harald Welte 2005-08-09 2075 netlink_table_ungrab();
> 77247bbb309424 Patrick McHardy 2005-08-14 2076 return sk;
> 77247bbb309424 Patrick McHardy 2005-08-14 2077
> 4fdb3bb723db46 Harald Welte 2005-08-09 2078 out_sock_release:
> 4277a083ecd2c8 Patrick McHardy 2006-03-20 2079 kfree(listeners);
> 90ae404765d263 Markus Elfring 2023-12-31 2080 out_netlink_release_sock:
> 9dfbec1fb2bedf Denis V. Lunev 2008-02-29 2081 netlink_kernel_release(sk);
> 23fe18669e7fda Pavel Emelyanov 2008-01-30 2082 return NULL;
How do you think about to use the function call “netlink_kernel_release(sock->sk)”?
Regards,
Markus
On Sun, 31 Dec 2023 18:42:30 +0100
Markus Elfring <[email protected]> wrote:
> From: Markus Elfring <[email protected]>
> Date: Sun, 31 Dec 2023 17:26:41 +0100
>
> The kfree() function was called in one case by
> the __netlink_kernel_create() function during error handling
> even if the passed variable contained a null pointer.
> This issue was detected by using the Coccinelle software.
>
> Thus use another label.
>
> Signed-off-by: Markus Elfring <[email protected]>
NAK
Please look at something else, calling kfree(NULL) is correct
and the preferred solution.
On Sun, 31 Dec 2023 18:44:13 +0100
Markus Elfring <[email protected]> wrote:
> From: Markus Elfring <[email protected]>
> Date: Sun, 31 Dec 2023 17:36:50 +0100
>
> Move one assignment for the variable “sk” closer to the place
> where this pointer is used.
>
> Signed-off-by: Markus Elfring <[email protected]>
NAK
Useless churn of source code.
If compiler will do this kind of optimization itself.
On Sun, 31 Dec 2023 18:46:09 +0100
Markus Elfring <[email protected]> wrote:
> From: Markus Elfring <[email protected]>
> Date: Sun, 31 Dec 2023 17:45:00 +0100
>
> The variable “listeners” will eventually be set to an appropriate pointer
> a bit later. Thus omit the explicit initialisation at the beginning.
>
> Signed-off-by: Markus Elfring <[email protected]>
This looks OK, and the compiler would catch if it was a problem.
Not really necessary though.
Acked-by: Stephen Hemminger <[email protected]>
On Sun, 31 Dec 2023 18:48:11 +0100
Markus Elfring <[email protected]> wrote:
> From: Markus Elfring <[email protected]>
> Date: Sun, 31 Dec 2023 18:16:26 +0100
>
> Move one assignment for the variable “cb_mutex” closer to the place
> where this pointer is used.
>
> Signed-off-by: Markus Elfring <[email protected]>
NAK
Useless code churn, no improvement in performance or readablilty.
Compiler optimizer will do this already if it wants.