Re: [PATCH] ip6_offload: check segs for NULL in ipv6_gso_segment.
From: Artem SavkovDate: Thu, 1 Dec 2016 14:06:04 +0100 > segs needs to be checked for being NULL in ipv6_gso_segment() before calling > skb_shinfo(segs), otherwise kernel can run into a NULL-pointer dereference: ... > Signed-off-by: Artem Savkov Applied and queued up for -stable.
Re: [PATCH] ip6_offload: check segs for NULL in ipv6_gso_segment.
From: Artem Savkov Date: Thu, 1 Dec 2016 14:06:04 +0100 > segs needs to be checked for being NULL in ipv6_gso_segment() before calling > skb_shinfo(segs), otherwise kernel can run into a NULL-pointer dereference: ... > Signed-off-by: Artem Savkov Applied and queued up for -stable.
Re: [PATCH] ip6_offload: check segs for NULL in ipv6_gso_segment.
On Thu, Dec 01, 2016 at 09:47:17PM -0500, Don Bowman wrote: > I have been having this problem (4.9rc6). > I applied this patch. > I still have the problem. > > My stack trace (attached as image, i did not get it in text) is: > > ipv6_gso_segment > skb_mac_gso_segment > __skb_gso_segment > validate_xmit_skb > sch_direct_xmit > __dev_queue_xmit > macvlan_start > > .. > > I think the macvlan is important (it does not seem to occur on my VM > that are not macvtap). I don't see any way that the same problem may get reproduced in ipv6_gso_segment with that patch applied, so this must be a different issue. -- Regards, Artem
Re: [PATCH] ip6_offload: check segs for NULL in ipv6_gso_segment.
On Thu, Dec 01, 2016 at 09:47:17PM -0500, Don Bowman wrote: > I have been having this problem (4.9rc6). > I applied this patch. > I still have the problem. > > My stack trace (attached as image, i did not get it in text) is: > > ipv6_gso_segment > skb_mac_gso_segment > __skb_gso_segment > validate_xmit_skb > sch_direct_xmit > __dev_queue_xmit > macvlan_start > > .. > > I think the macvlan is important (it does not seem to occur on my VM > that are not macvtap). I don't see any way that the same problem may get reproduced in ipv6_gso_segment with that patch applied, so this must be a different issue. -- Regards, Artem
Re: [PATCH] ip6_offload: check segs for NULL in ipv6_gso_segment.
On Thu, 2016-12-01 at 16:07 +0100, Artem Savkov wrote: > I am not, but this would have the same behavior as pre-07b26c9 code and > IS_ERR_OR_NULL is used in ipv4's inet_gso_segment(). My concern might have been that IS_ERR_OR_NULL() considers the !ptr to be unlikely. But in this code path, we really can not tell. segs == NULL can be quite likely in TUN case, because of DODGY bit Commit 50c3a487d50756 replaced the perfectly fine : if (!segs || IS_ERR(segs)) into dubious if (IS_ERR_OR_NULL(segs)) segs = NULL is not an error, but use of IS_ERR_OR_NULL() might mislead programmers trying to understand this code.
Re: [PATCH] ip6_offload: check segs for NULL in ipv6_gso_segment.
On Thu, 2016-12-01 at 16:07 +0100, Artem Savkov wrote: > I am not, but this would have the same behavior as pre-07b26c9 code and > IS_ERR_OR_NULL is used in ipv4's inet_gso_segment(). My concern might have been that IS_ERR_OR_NULL() considers the !ptr to be unlikely. But in this code path, we really can not tell. segs == NULL can be quite likely in TUN case, because of DODGY bit Commit 50c3a487d50756 replaced the perfectly fine : if (!segs || IS_ERR(segs)) into dubious if (IS_ERR_OR_NULL(segs)) segs = NULL is not an error, but use of IS_ERR_OR_NULL() might mislead programmers trying to understand this code.
Re: [PATCH] ip6_offload: check segs for NULL in ipv6_gso_segment.
On Thu, 2016-12-01 at 06:34 -0800, Eric Dumazet wrote: > On Thu, 2016-12-01 at 14:06 +0100, Artem Savkov wrote: > > segs needs to be checked for being NULL in ipv6_gso_segment() before calling > > skb_shinfo(segs), otherwise kernel can run into a NULL-pointer dereference: > > > > Signed-off-by: Artem Savkov> > --- > > > > > diff --git a/net/ipv6/ip6_offload.c b/net/ipv6/ip6_offload.c > > index 1fcf61f..89c59e6 100644 > > --- a/net/ipv6/ip6_offload.c > > +++ b/net/ipv6/ip6_offload.c > > @@ -99,7 +99,7 @@ static struct sk_buff *ipv6_gso_segment(struct sk_buff > > *skb, > > segs = ops->callbacks.gso_segment(skb, features); > > } > > > > - if (IS_ERR(segs)) > > + if (IS_ERR_OR_NULL(segs)) > > goto out; > > > > gso_partial = !!(skb_shinfo(segs)->gso_type & SKB_GSO_PARTIAL); > > Do you know when was this bug added ? > > Are you sure this is the right fix ? > > Which gso_segment() is returning NULL exactly ? Oh never mind. This is the same fix than 576a30eb64534 but applied to IPv6. Thanks ! Acked-by: Eric Dumazet
Re: [PATCH] ip6_offload: check segs for NULL in ipv6_gso_segment.
On Thu, 2016-12-01 at 06:34 -0800, Eric Dumazet wrote: > On Thu, 2016-12-01 at 14:06 +0100, Artem Savkov wrote: > > segs needs to be checked for being NULL in ipv6_gso_segment() before calling > > skb_shinfo(segs), otherwise kernel can run into a NULL-pointer dereference: > > > > Signed-off-by: Artem Savkov > > --- > > > > > diff --git a/net/ipv6/ip6_offload.c b/net/ipv6/ip6_offload.c > > index 1fcf61f..89c59e6 100644 > > --- a/net/ipv6/ip6_offload.c > > +++ b/net/ipv6/ip6_offload.c > > @@ -99,7 +99,7 @@ static struct sk_buff *ipv6_gso_segment(struct sk_buff > > *skb, > > segs = ops->callbacks.gso_segment(skb, features); > > } > > > > - if (IS_ERR(segs)) > > + if (IS_ERR_OR_NULL(segs)) > > goto out; > > > > gso_partial = !!(skb_shinfo(segs)->gso_type & SKB_GSO_PARTIAL); > > Do you know when was this bug added ? > > Are you sure this is the right fix ? > > Which gso_segment() is returning NULL exactly ? Oh never mind. This is the same fix than 576a30eb64534 but applied to IPv6. Thanks ! Acked-by: Eric Dumazet
Re: [PATCH] ip6_offload: check segs for NULL in ipv6_gso_segment.
On Thu, Dec 01, 2016 at 06:34:07AM -0800, Eric Dumazet wrote: > On Thu, 2016-12-01 at 14:06 +0100, Artem Savkov wrote: > > segs needs to be checked for being NULL in ipv6_gso_segment() before calling > > skb_shinfo(segs), otherwise kernel can run into a NULL-pointer dereference: > > > > Signed-off-by: Artem Savkov> > --- > > > > > diff --git a/net/ipv6/ip6_offload.c b/net/ipv6/ip6_offload.c > > index 1fcf61f..89c59e6 100644 > > --- a/net/ipv6/ip6_offload.c > > +++ b/net/ipv6/ip6_offload.c > > @@ -99,7 +99,7 @@ static struct sk_buff *ipv6_gso_segment(struct sk_buff > > *skb, > > segs = ops->callbacks.gso_segment(skb, features); > > } > > > > - if (IS_ERR(segs)) > > + if (IS_ERR_OR_NULL(segs)) > > goto out; > > > > gso_partial = !!(skb_shinfo(segs)->gso_type & SKB_GSO_PARTIAL); > > Do you know when was this bug added ? It started to show up with 4.9-rc4, from what I see the culprit is 07b26c9 gso: Support partial splitting at the frag_list pointer > Are you sure this is the right fix ? I am not, but this would have the same behavior as pre-07b26c9 code and IS_ERR_OR_NULL is used in ipv4's inet_gso_segment(). > Which gso_segment() is returning NULL exactly ? Unfortunatelly I don't know that and I don't have a good reproducer, the only way to reproduce this that I currently have is calling virt-install. -- Regards, Artem
Re: [PATCH] ip6_offload: check segs for NULL in ipv6_gso_segment.
On Thu, Dec 01, 2016 at 06:34:07AM -0800, Eric Dumazet wrote: > On Thu, 2016-12-01 at 14:06 +0100, Artem Savkov wrote: > > segs needs to be checked for being NULL in ipv6_gso_segment() before calling > > skb_shinfo(segs), otherwise kernel can run into a NULL-pointer dereference: > > > > Signed-off-by: Artem Savkov > > --- > > > > > diff --git a/net/ipv6/ip6_offload.c b/net/ipv6/ip6_offload.c > > index 1fcf61f..89c59e6 100644 > > --- a/net/ipv6/ip6_offload.c > > +++ b/net/ipv6/ip6_offload.c > > @@ -99,7 +99,7 @@ static struct sk_buff *ipv6_gso_segment(struct sk_buff > > *skb, > > segs = ops->callbacks.gso_segment(skb, features); > > } > > > > - if (IS_ERR(segs)) > > + if (IS_ERR_OR_NULL(segs)) > > goto out; > > > > gso_partial = !!(skb_shinfo(segs)->gso_type & SKB_GSO_PARTIAL); > > Do you know when was this bug added ? It started to show up with 4.9-rc4, from what I see the culprit is 07b26c9 gso: Support partial splitting at the frag_list pointer > Are you sure this is the right fix ? I am not, but this would have the same behavior as pre-07b26c9 code and IS_ERR_OR_NULL is used in ipv4's inet_gso_segment(). > Which gso_segment() is returning NULL exactly ? Unfortunatelly I don't know that and I don't have a good reproducer, the only way to reproduce this that I currently have is calling virt-install. -- Regards, Artem
Re: [PATCH] ip6_offload: check segs for NULL in ipv6_gso_segment.
On Thu, 2016-12-01 at 14:06 +0100, Artem Savkov wrote: > segs needs to be checked for being NULL in ipv6_gso_segment() before calling > skb_shinfo(segs), otherwise kernel can run into a NULL-pointer dereference: > Signed-off-by: Artem Savkov> --- > > diff --git a/net/ipv6/ip6_offload.c b/net/ipv6/ip6_offload.c > index 1fcf61f..89c59e6 100644 > --- a/net/ipv6/ip6_offload.c > +++ b/net/ipv6/ip6_offload.c > @@ -99,7 +99,7 @@ static struct sk_buff *ipv6_gso_segment(struct sk_buff *skb, > segs = ops->callbacks.gso_segment(skb, features); > } > > - if (IS_ERR(segs)) > + if (IS_ERR_OR_NULL(segs)) > goto out; > > gso_partial = !!(skb_shinfo(segs)->gso_type & SKB_GSO_PARTIAL); Do you know when was this bug added ? Are you sure this is the right fix ? Which gso_segment() is returning NULL exactly ? Thanks.
Re: [PATCH] ip6_offload: check segs for NULL in ipv6_gso_segment.
On Thu, 2016-12-01 at 14:06 +0100, Artem Savkov wrote: > segs needs to be checked for being NULL in ipv6_gso_segment() before calling > skb_shinfo(segs), otherwise kernel can run into a NULL-pointer dereference: > Signed-off-by: Artem Savkov > --- > > diff --git a/net/ipv6/ip6_offload.c b/net/ipv6/ip6_offload.c > index 1fcf61f..89c59e6 100644 > --- a/net/ipv6/ip6_offload.c > +++ b/net/ipv6/ip6_offload.c > @@ -99,7 +99,7 @@ static struct sk_buff *ipv6_gso_segment(struct sk_buff *skb, > segs = ops->callbacks.gso_segment(skb, features); > } > > - if (IS_ERR(segs)) > + if (IS_ERR_OR_NULL(segs)) > goto out; > > gso_partial = !!(skb_shinfo(segs)->gso_type & SKB_GSO_PARTIAL); Do you know when was this bug added ? Are you sure this is the right fix ? Which gso_segment() is returning NULL exactly ? Thanks.