Function 'ping_queue_rcv_skb' not always return success, which will
also return fail. If not check the wrong return value of it, lead
to function `ping_rcv` return success.
Signed-off-by: Zheng Yongjun <[email protected]>
---
v2:
- use rc as return value to make code look cleaner
v3:
- delete unnecessary braces {}
net/ipv4/ping.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/net/ipv4/ping.c b/net/ipv4/ping.c
index 1c9f71a37258..af9da2f7dc85 100644
--- a/net/ipv4/ping.c
+++ b/net/ipv4/ping.c
@@ -963,19 +963,19 @@ bool ping_rcv(struct sk_buff *skb)
/* Push ICMP header back */
skb_push(skb, skb->data - (u8 *)icmph);
+ bool rc = false;
sk = ping_lookup(net, skb, ntohs(icmph->un.echo.id));
if (sk) {
struct sk_buff *skb2 = skb_clone(skb, GFP_ATOMIC);
pr_debug("rcv on socket %p\n", sk);
- if (skb2)
- ping_queue_rcv_skb(sk, skb2);
+ if (skb2 && !ping_queue_rcv_skb(sk, skb2))
+ rc = true;
sock_put(sk);
- return true;
}
pr_debug("no socket, dropping\n");
- return false;
+ return rc;
}
EXPORT_SYMBOL_GPL(ping_rcv);
--
2.25.1
On Tue, Jun 08, 2021 at 10:08:53AM +0800, Zheng Yongjun wrote:
> Function 'ping_queue_rcv_skb' not always return success, which will
> also return fail. If not check the wrong return value of it, lead
> to function `ping_rcv` return success.
>
> Signed-off-by: Zheng Yongjun <[email protected]>
> ---
> v2:
> - use rc as return value to make code look cleaner
> v3:
> - delete unnecessary braces {}
> net/ipv4/ping.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/net/ipv4/ping.c b/net/ipv4/ping.c
> index 1c9f71a37258..af9da2f7dc85 100644
> --- a/net/ipv4/ping.c
> +++ b/net/ipv4/ping.c
> @@ -963,19 +963,19 @@ bool ping_rcv(struct sk_buff *skb)
> /* Push ICMP header back */
> skb_push(skb, skb->data - (u8 *)icmph);
>
> + bool rc = false;
Declaration of new variables in the middle of function is C++, while the
kernel is written in C. Please put variable declaration at the beginning
of function.
Thanks
> sk = ping_lookup(net, skb, ntohs(icmph->un.echo.id));
> if (sk) {
> struct sk_buff *skb2 = skb_clone(skb, GFP_ATOMIC);
>
> pr_debug("rcv on socket %p\n", sk);
> - if (skb2)
> - ping_queue_rcv_skb(sk, skb2);
> + if (skb2 && !ping_queue_rcv_skb(sk, skb2))
> + rc = true;
> sock_put(sk);
> - return true;
> }
> pr_debug("no socket, dropping\n");
>
> - return false;
> + return rc;
> }
> EXPORT_SYMBOL_GPL(ping_rcv);
>
> --
> 2.25.1
>
Thanks for your suggest, I send patch v4 now :)
-----?ʼ?ԭ??-----
??????: Leon Romanovsky [mailto:[email protected]]
????ʱ??: 2021??6??8?? 14:08
?ռ???: zhengyongjun <[email protected]>
????: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
????: Re: [PATCH v3] ping: Check return value of function 'ping_queue_rcv_skb'
On Tue, Jun 08, 2021 at 10:08:53AM +0800, Zheng Yongjun wrote:
> Function 'ping_queue_rcv_skb' not always return success, which will
> also return fail. If not check the wrong return value of it, lead to
> function `ping_rcv` return success.
>
> Signed-off-by: Zheng Yongjun <[email protected]>
> ---
> v2:
> - use rc as return value to make code look cleaner
> v3:
> - delete unnecessary braces {}
> net/ipv4/ping.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/net/ipv4/ping.c b/net/ipv4/ping.c index
> 1c9f71a37258..af9da2f7dc85 100644
> --- a/net/ipv4/ping.c
> +++ b/net/ipv4/ping.c
> @@ -963,19 +963,19 @@ bool ping_rcv(struct sk_buff *skb)
> /* Push ICMP header back */
> skb_push(skb, skb->data - (u8 *)icmph);
>
> + bool rc = false;
Declaration of new variables in the middle of function is C++, while the kernel is written in C. Please put variable declaration at the beginning of function.
Thanks
> sk = ping_lookup(net, skb, ntohs(icmph->un.echo.id));
> if (sk) {
> struct sk_buff *skb2 = skb_clone(skb, GFP_ATOMIC);
>
> pr_debug("rcv on socket %p\n", sk);
> - if (skb2)
> - ping_queue_rcv_skb(sk, skb2);
> + if (skb2 && !ping_queue_rcv_skb(sk, skb2))
> + rc = true;
> sock_put(sk);
> - return true;
> }
> pr_debug("no socket, dropping\n");
>
> - return false;
> + return rc;
> }
> EXPORT_SYMBOL_GPL(ping_rcv);
>
> --
> 2.25.1
>
Hi Zheng,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on net-next/master]
[also build test WARNING on net/master linus/master sparc-next/master v5.13-rc5 next-20210607]
[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]
url: https://github.com/0day-ci/linux/commits/Zheng-Yongjun/ping-Check-return-value-of-function-ping_queue_rcv_skb/20210608-095716
base: https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git dc8cf7550a703b8b9c94beed621c6c2474347eff
config: ia64-randconfig-r035-20210607 (attached as .config)
compiler: ia64-linux-gcc (GCC) 9.3.0
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
# https://github.com/0day-ci/linux/commit/bdcbf04f42e253fb727ce96bb2a680bd6c449468
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Zheng-Yongjun/ping-Check-return-value-of-function-ping_queue_rcv_skb/20210608-095716
git checkout bdcbf04f42e253fb727ce96bb2a680bd6c449468
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=ia64
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>
All warnings (new ones prefixed by >>):
In file included from arch/ia64/include/asm/pgtable.h:154,
from include/linux/pgtable.h:6,
from arch/ia64/include/asm/uaccess.h:40,
from include/linux/uaccess.h:11,
from net/ipv4/ping.c:18:
arch/ia64/include/asm/mmu_context.h: In function 'reload_context':
arch/ia64/include/asm/mmu_context.h:127:41: warning: variable 'old_rr4' set but not used [-Wunused-but-set-variable]
127 | unsigned long rr0, rr1, rr2, rr3, rr4, old_rr4;
| ^~~~~~~
net/ipv4/ping.c: In function 'ping_rcv':
>> net/ipv4/ping.c:966:2: warning: ISO C90 forbids mixed declarations and code [-Wdeclaration-after-statement]
966 | bool rc = false;
| ^~~~
vim +966 net/ipv4/ping.c
946
947
948 /*
949 * All we need to do is get the socket.
950 */
951
952 bool ping_rcv(struct sk_buff *skb)
953 {
954 struct sock *sk;
955 struct net *net = dev_net(skb->dev);
956 struct icmphdr *icmph = icmp_hdr(skb);
957
958 /* We assume the packet has already been checked by icmp_rcv */
959
960 pr_debug("ping_rcv(skb=%p,id=%04x,seq=%04x)\n",
961 skb, ntohs(icmph->un.echo.id), ntohs(icmph->un.echo.sequence));
962
963 /* Push ICMP header back */
964 skb_push(skb, skb->data - (u8 *)icmph);
965
> 966 bool rc = false;
967 sk = ping_lookup(net, skb, ntohs(icmph->un.echo.id));
968 if (sk) {
969 struct sk_buff *skb2 = skb_clone(skb, GFP_ATOMIC);
970
971 pr_debug("rcv on socket %p\n", sk);
972 if (skb2 && !ping_queue_rcv_skb(sk, skb2))
973 rc = true;
974 sock_put(sk);
975 }
976 pr_debug("no socket, dropping\n");
977
978 return rc;
979 }
980 EXPORT_SYMBOL_GPL(ping_rcv);
981
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]
Hi Zheng,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on net-next/master]
[also build test WARNING on net/master linus/master sparc-next/master v5.13-rc5 next-20210607]
[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]
url: https://github.com/0day-ci/linux/commits/Zheng-Yongjun/ping-Check-return-value-of-function-ping_queue_rcv_skb/20210608-095716
base: https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git dc8cf7550a703b8b9c94beed621c6c2474347eff
config: x86_64-randconfig-r033-20210607 (attached as .config)
compiler: clang version 13.0.0 (https://github.com/llvm/llvm-project d32cc150feb72f315a5bbd34f92e7beca21a50da)
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 x86_64 cross compiling tool for clang build
# apt-get install binutils-x86-64-linux-gnu
# https://github.com/0day-ci/linux/commit/bdcbf04f42e253fb727ce96bb2a680bd6c449468
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Zheng-Yongjun/ping-Check-return-value-of-function-ping_queue_rcv_skb/20210608-095716
git checkout bdcbf04f42e253fb727ce96bb2a680bd6c449468
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>
All warnings (new ones prefixed by >>):
>> net/ipv4/ping.c:966:7: warning: ISO C90 forbids mixing declarations and code [-Wdeclaration-after-statement]
bool rc = false;
^
1 warning generated.
vim +966 net/ipv4/ping.c
946
947
948 /*
949 * All we need to do is get the socket.
950 */
951
952 bool ping_rcv(struct sk_buff *skb)
953 {
954 struct sock *sk;
955 struct net *net = dev_net(skb->dev);
956 struct icmphdr *icmph = icmp_hdr(skb);
957
958 /* We assume the packet has already been checked by icmp_rcv */
959
960 pr_debug("ping_rcv(skb=%p,id=%04x,seq=%04x)\n",
961 skb, ntohs(icmph->un.echo.id), ntohs(icmph->un.echo.sequence));
962
963 /* Push ICMP header back */
964 skb_push(skb, skb->data - (u8 *)icmph);
965
> 966 bool rc = false;
967 sk = ping_lookup(net, skb, ntohs(icmph->un.echo.id));
968 if (sk) {
969 struct sk_buff *skb2 = skb_clone(skb, GFP_ATOMIC);
970
971 pr_debug("rcv on socket %p\n", sk);
972 if (skb2 && !ping_queue_rcv_skb(sk, skb2))
973 rc = true;
974 sock_put(sk);
975 }
976 pr_debug("no socket, dropping\n");
977
978 return rc;
979 }
980 EXPORT_SYMBOL_GPL(ping_rcv);
981
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]