| Message ID | 20200728021348.4116-1-gaurav1086@gmail.com |
|---|---|
| State | Rejected |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net/ipv6] ip6_output: Add ipv6_pinfo null check | expand |
On Mon, Jul 27, 2020 at 7:14 PM Gaurav Singh <gaurav1086@gmail.com> 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 <gaurav1086@gmail.com> 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.
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); } /*
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 <gaurav1086@gmail.com> --- net/ipv6/ip6_output.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)