On 23.11.2016 05:45, Rohit Thapliyal wrote: > |>On 22.11.2016 07:27, Manjeet Pawar wrote: > >> From: Rohit Thapliyal <r.thapli...@samsung.com > <mailto:r.thapli...@samsung.com>> > >> > >> np checked for NULL and then dereferenced. It should be modified > >> for NULL case. > >> > >> Signed-off-by: Rohit Thapliyal <r.thapli...@samsung.com > <mailto:r.thapli...@samsung.com>>> > >> Signed-off-by: Manjeet Pawar <manjee...@samsung.com > <mailto:manjee...@samsung.com>>> > >> --- > >> net/ipv6/ip6_output.c | 9 +++++---- > >> 1 file changed, 5 insertions(+), 4 deletions(-) > >> > >> diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c > >> index 1dfc402..c2afa14 100644 > >> --- a/net/ipv6/ip6_output.c > >> +++ b/net/ipv6/ip6_output.c > >> @@ -205,14 +205,15 @@ int ip6_xmit(const struct sock *sk, struct sk_buff > *skb, struct flowi6 *fl6, > >> /* > >> * Fill in the IPv6 header > >> */ > >> - if (np) > >> + if (np) { > >> hlimit = np->hop_limit; > >> + ip6_flow_hdr( > >> + hdr, tclass, ip6_make_flowlabel( > >> + net, skb, fl6->flowlabel, > >> + np->autoflowlabel, fl6)); > >> + } > >> if (hlimit < 0) > >> hlimit = ip6_dst_hoplimit(dst); > >> > >> - ip6_flow_hdr(hdr, tclass, ip6_make_flowlabel(net, skb, fl6->flowlabel, > >> - np->autoflowlabel, fl6)); > >> - > >> hdr->payload_len = htons(seg_len); > >> hdr->nexthdr = proto; > >> hdr->hop_limit = hlimit; > >> > > > > > >We always should initialize hdr and not skip the ip6_flow_hdr call.| > > |if np becomes NULL, then anyways hdr won't be initialized due to NULL > pointer > dereference ip6_make_flowlabel.|
Which we would see as a crash. So far no crash has been reported in this code. Doing a quick code review on the paths leading to ip6_xmit, inet6_sk must always be set to actually reach up to this point. Otherwise we would have crashes on all code paths much earler. Anyway, I would be fine to keep the NULL check in this path, it looks better because of the inet6_sk you pointed out above but I would recommend to just use a "np ? np->autoflowlabel : ip6_default_np_autolabel(net) in the ip6_flow_hdr function. > |>Do you saw a bug or did you find this by code review? I wonder if np can > >actually be NULL at this point. Maybe we can just eliminate the NULL check.| > > | > > > I must admit that I found it just by code review, and so far didn't face any > crash whatsoever. > As we can see in inet6_sk, np could be NULL. Thus, the NULL check seems > justified. Thanks for looking at this! Bye, Hannes
