ipv6_pinfo is initlialized by inet6_sk() which returns NULL.
Hence it can cause segmentation fault. Fix this by adding a
NULL check.
Signed-off-by: Gaurav Singh <[email protected]>
---
net/ipv6/ip6_output.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c
index 8a8c2d0cfcc8..7c077a6847e4 100644
--- a/net/ipv6/ip6_output.c
+++ b/net/ipv6/ip6_output.c
@@ -181,10 +181,10 @@ int ip6_output(struct net *net, struct sock *sk, struct sk_buff *skb)
bool ip6_autoflowlabel(struct net *net, const struct ipv6_pinfo *np)
{
- if (!np->autoflowlabel_set)
- return ip6_default_np_autolabel(net);
- else
+ if (np && np->autoflowlabel_set)
return np->autoflowlabel;
+ else
+ ip6_default_np_autolabel(net);
}
/*
--
2.17.1
Hi Gaurav,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on sparc-next/master]
[also build test WARNING on ipvs/master linus/master v5.8-rc7 next-20200724]
[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/Gaurav-Singh/ip6_output-Add-ipv6_pinfo-null-check/20200727-113949
base: https://git.kernel.org/pub/scm/linux/kernel/git/davem/sparc-next.git master
config: csky-defconfig (attached as .config)
compiler: csky-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
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=csky
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/ipv6/ip6_output.c: In function 'ip6_autoflowlabel':
>> net/ipv6/ip6_output.c:188:1: warning: control reaches end of non-void function [-Wreturn-type]
188 | }
| ^
vim +188 net/ipv6/ip6_output.c
^1da177e4c3f41 Linus Torvalds 2005-04-16 181
e9191ffb65d8e1 Ben Hutchings 2018-01-22 182 bool ip6_autoflowlabel(struct net *net, const struct ipv6_pinfo *np)
513674b5a2c9c7 Shaohua Li 2017-12-20 183 {
5bdc1ea8a7d229 Gaurav Singh 2020-07-26 184 if (np && np->autoflowlabel_set)
513674b5a2c9c7 Shaohua Li 2017-12-20 185 return np->autoflowlabel;
5bdc1ea8a7d229 Gaurav Singh 2020-07-26 186 else
5bdc1ea8a7d229 Gaurav Singh 2020-07-26 187 ip6_default_np_autolabel(net);
513674b5a2c9c7 Shaohua Li 2017-12-20 @188 }
513674b5a2c9c7 Shaohua Li 2017-12-20 189
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]
Hi Gaurav,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on sparc-next/master]
[also build test WARNING on ipvs/master linus/master v5.8-rc7 next-20200724]
[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/Gaurav-Singh/ip6_output-Add-ipv6_pinfo-null-check/20200727-113949
base: https://git.kernel.org/pub/scm/linux/kernel/git/davem/sparc-next.git master
config: x86_64-randconfig-r011-20200727 (attached as .config)
compiler: clang version 12.0.0 (https://github.com/llvm/llvm-project 8dc820393219c7ee440b4ec86c9a201301943276)
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
# 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/ipv6/ip6_output.c:188:1: warning: non-void function does not return a value in all control paths [-Wreturn-type]
}
^
1 warning generated.
vim +188 net/ipv6/ip6_output.c
^1da177e4c3f41 Linus Torvalds 2005-04-16 181
e9191ffb65d8e1 Ben Hutchings 2018-01-22 182 bool ip6_autoflowlabel(struct net *net, const struct ipv6_pinfo *np)
513674b5a2c9c7 Shaohua Li 2017-12-20 183 {
5bdc1ea8a7d229 Gaurav Singh 2020-07-26 184 if (np && np->autoflowlabel_set)
513674b5a2c9c7 Shaohua Li 2017-12-20 185 return np->autoflowlabel;
5bdc1ea8a7d229 Gaurav Singh 2020-07-26 186 else
5bdc1ea8a7d229 Gaurav Singh 2020-07-26 187 ip6_default_np_autolabel(net);
513674b5a2c9c7 Shaohua Li 2017-12-20 @188 }
513674b5a2c9c7 Shaohua Li 2017-12-20 189
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]
From: Gaurav Singh <[email protected]>
Date: Sun, 26 Jul 2020 23:38:10 -0400
> ipv6_pinfo is initlialized by inet6_sk() which returns NULL.
> Hence it can cause segmentation fault. Fix this by adding a
> NULL check.
Please take your time with such changes and actually look at the
compiler output, it will warn that you are adding a new problem.
Specifically, the function now fails to return a value at all.
But even more important, how is the situation you are fixing
possible? Do you have a sample crash? Can you please describe
the code paths and conditions that lead to the problem?
You must also provide a valid and properly formatted Fixes: tag
for bug fixes such as this.
Thank you.
On Sun, Jul 26, 2020 at 8:39 PM Gaurav Singh <[email protected]> wrote:
>
> ipv6_pinfo is initlialized by inet6_sk() which returns NULL.
Why? It only returns NULL for timewait or request sock, but
I don't see how ip6_autoflowlabel() could be called on these
sockets. So please explain.
> Hence it can cause segmentation fault. Fix this by adding a
> NULL check.
Which exact call path? Do you have a full stack trace?
Thanks.
Add return to fix build issue. Haven't reproduced this issue at
my end.
My hypothesis is this: In function: ip6_xmit(), we have
const struct ipv6_pinfo *np = inet6_sk(sk); which returns NULL.
Further down the function, there's a check:
if (np) hlimit = hp->htop_limit
Further, we have a call
ip6_flow_hdr(hdr, tclass, ip6_make_flowlabel(net, skb, fl6->flowlabel,
ip6_autoflowlabel(net, np), fl6)); .
Hence np = NULL gets passed in
the function ip6_autoflowlabel() which accesses np-> without check which
may cause a segment violation.
Signed-off-by: Gaurav Singh <[email protected]>
---
net/ipv6/ip6_output.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c
index 8a8c2d0cfcc8..94a07c9bd925 100644
--- a/net/ipv6/ip6_output.c
+++ b/net/ipv6/ip6_output.c
@@ -181,10 +181,10 @@ int ip6_output(struct net *net, struct sock *sk, struct sk_buff *skb)
bool ip6_autoflowlabel(struct net *net, const struct ipv6_pinfo *np)
{
- if (!np->autoflowlabel_set)
- return ip6_default_np_autolabel(net);
- else
+ if (np && np->autoflowlabel_set)
return np->autoflowlabel;
+ else
+ return ip6_default_np_autolabel(net);
}
/*
--
2.17.1
On Mon, Jul 27, 2020 at 7:14 PM Gaurav Singh <[email protected]> wrote:
>
> Add return to fix build issue. Haven't reproduced this issue at
> my end.
>
> My hypothesis is this: In function: ip6_xmit(), we have
> const struct ipv6_pinfo *np = inet6_sk(sk); which returns NULL.
>
> Further down the function, there's a check:
> if (np) hlimit = hp->htop_limit
This check exists before git history, at that time 'sk' could be NULL,
hence 'np', so it does not mean it is still necessary now.
I looked at all callers of ip6_xmit(), I don't see how it is called with
a non-full socket, neither 'sk' could be NULL after
commit b30bd282cbf5c46247a279a2e8d2aae027d9f1bf
("[IPV6]: ip6_xmit: remove unnecessary NULL ptr check").
Thanks.
On 7/27/20 8:12 PM, Cong Wang wrote:
> On Mon, Jul 27, 2020 at 7:14 PM Gaurav Singh <[email protected]> wrote:
>>
>> Add return to fix build issue. Haven't reproduced this issue at
>> my end.
>>
>> My hypothesis is this: In function: ip6_xmit(), we have
>> const struct ipv6_pinfo *np = inet6_sk(sk); which returns NULL.
>>
>> Further down the function, there's a check:
>> if (np) hlimit = hp->htop_limit
>
> This check exists before git history, at that time 'sk' could be NULL,
> hence 'np', so it does not mean it is still necessary now.
>
> I looked at all callers of ip6_xmit(), I don't see how it is called with
> a non-full socket, neither 'sk' could be NULL after
> commit b30bd282cbf5c46247a279a2e8d2aae027d9f1bf
> ("[IPV6]: ip6_xmit: remove unnecessary NULL ptr check").
>
> Thanks.
>
Agreed.
And again, fact that this patch lacks a Fixes: tag speaks for itself.
This means the author expects all reviewers to make a deep analysis.
Please bear with us, and add a Fixes: tag so that we can fully understand what was
the bug origin and why a fix is valid.
This PR fixes a possible segmentation violation.
In function: ip6_xmit(), we have
const struct ipv6_pinfo *np = inet6_sk(sk); which returns NULL
unconditionally (regardless sk being NULL or not).
In include/linux/ipv6.h:
static inline struct ipv6_pinfo * inet6_sk(const struct sock *__sk)
{
return NULL;
}
Further down the function, there's a check:
if (np) hlimit = hp->htop_limit
Thereafter, we have a call
ip6_flow_hdr(hdr, tclass, ip6_make_flowlabel(net, skb, fl6->flowlabel,
ip6_autoflowlabel(net, np), fl6));
Hence np = NULL gets passed in
the function ip6_autoflowlabel() that accesses np without check which
may cause a segment violation.
Fixes: 513674b5a2c9c ("net: reevalulate autoflowlabel setting after sysctl setting")
Signed-off-by: Gaurav Singh <[email protected]>
---
net/ipv6/ip6_output.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c
index 8a8c2d0cfcc8..94a07c9bd925 100644
--- a/net/ipv6/ip6_output.c
+++ b/net/ipv6/ip6_output.c
@@ -181,10 +181,10 @@ int ip6_output(struct net *net, struct sock *sk, struct sk_buff *skb)
bool ip6_autoflowlabel(struct net *net, const struct ipv6_pinfo *np)
{
- if (!np->autoflowlabel_set)
- return ip6_default_np_autolabel(net);
- else
+ if (np && np->autoflowlabel_set)
return np->autoflowlabel;
+ else
+ return ip6_default_np_autolabel(net);
}
/*
--
2.17.1
On Sat, Aug 8, 2020 at 10:07 AM Gaurav Singh <[email protected]> wrote:
>
> This PR fixes a possible segmentation violation.
>
> In function: ip6_xmit(), we have
> const struct ipv6_pinfo *np = inet6_sk(sk); which returns NULL
> unconditionally (regardless sk being NULL or not).
>
> In include/linux/ipv6.h:
>
> static inline struct ipv6_pinfo * inet6_sk(const struct sock *__sk)
> {
> return NULL;
> }
>
Tell us who will use ip6_autoflowlabel() when CONFIG_IPV6
is disabled. :)