| Message ID | 1479796024-39418-1-git-send-email-manjeet.p@samsung.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
On 22.11.2016 07:27, Manjeet Pawar wrote: > From: Rohit Thapliyal <r.thapliyal@samsung.com> > > np checked for NULL and then dereferenced. It should be modified > for NULL case. > > Signed-off-by: Rohit Thapliyal <r.thapliyal@samsung.com> > Signed-off-by: Manjeet Pawar <manjeet.p@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. 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. Thanks, Hannes
From: Hannes Frederic Sowa <hannes@stressinduktion.org> Date: Tue, 22 Nov 2016 13:26:45 +0100 > On 22.11.2016 07:27, Manjeet Pawar wrote: >> From: Rohit Thapliyal <r.thapliyal@samsung.com> >> >> np checked for NULL and then dereferenced. It should be modified >> for NULL case. >> >> Signed-off-by: Rohit Thapliyal <r.thapliyal@samsung.com> >> Signed-off-by: Manjeet Pawar <manjeet.p@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. > > 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. Also the indentation is really off.
On 23.11.2016 05:45, Rohit Thapliyal wrote: > |>On 22.11.2016 07:27, Manjeet Pawar wrote: > >> From: Rohit Thapliyal <r.thapliyal@samsung.com <mailto:r.thapliyal@samsung.com>> > >> > >> np checked for NULL and then dereferenced. It should be modified > >> for NULL case. > >> > >> Signed-off-by: Rohit Thapliyal <r.thapliyal@samsung.com > <mailto:r.thapliyal@samsung.com>>> > >> Signed-off-by: Manjeet Pawar <manjeet.p@samsung.com > <mailto:manjeet.p@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
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;