| Message ID | 861947c2d2d087db82af93c21920ce8147d15490.1611074818.git.pabeni@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2,net-next] net: fix GSO for SG-enabled devices. | expand |
On Wed, Jan 20, 2021 at 12:57 AM Paolo Abeni <pabeni@redhat.com> wrote: > > The commit dbd50f238dec ("net: move the hsize check to the else > block in skb_segment") introduced a data corruption for devices > supporting scatter-gather. > > The problem boils down to signed/unsigned comparison given > unexpected results: if signed 'hsize' is negative, it will be > considered greater than a positive 'len', which is unsigned. > > This commit addresses resorting to the old checks order, so that > 'hsize' never has a negative value when compared with 'len'. > > v1 -> v2: > - reorder hsize checks instead of explicit cast (Alex) > > Bisected-by: Matthieu Baerts <matthieu.baerts@tessares.net> > Fixes: dbd50f238dec ("net: move the hsize check to the else block in skb_segment") > Signed-off-by: Paolo Abeni <pabeni@redhat.com> Reviewed-by: Xin Long <lucien.xin@gmail.com> > --- > net/core/skbuff.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/net/core/skbuff.c b/net/core/skbuff.c > index e835193cabcc3..cf2c4dcf42579 100644 > --- a/net/core/skbuff.c > +++ b/net/core/skbuff.c > @@ -3938,10 +3938,10 @@ struct sk_buff *skb_segment(struct sk_buff *head_skb, > skb_release_head_state(nskb); > __skb_push(nskb, doffset); > } else { > + if (hsize < 0) > + hsize = 0; > if (hsize > len || !sg) > hsize = len; > - else if (hsize < 0) > - hsize = 0; > > nskb = __alloc_skb(hsize + doffset + headroom, > GFP_ATOMIC, skb_alloc_rx_flag(head_skb), > -- > 2.26.2 > Reviewed-by: Xin Long <lucien.xin@gmail.com>
On Wed, 20 Jan 2021 10:17:44 +0800 Xin Long wrote: > On Wed, Jan 20, 2021 at 12:57 AM Paolo Abeni <pabeni@redhat.com> wrote: > > > > The commit dbd50f238dec ("net: move the hsize check to the else > > block in skb_segment") introduced a data corruption for devices > > supporting scatter-gather. > > > > The problem boils down to signed/unsigned comparison given > > unexpected results: if signed 'hsize' is negative, it will be > > considered greater than a positive 'len', which is unsigned. > > > > This commit addresses resorting to the old checks order, so that > > 'hsize' never has a negative value when compared with 'len'. > > > > v1 -> v2: > > - reorder hsize checks instead of explicit cast (Alex) > > > > Bisected-by: Matthieu Baerts <matthieu.baerts@tessares.net> > > Fixes: dbd50f238dec ("net: move the hsize check to the else block in skb_segment") > > Signed-off-by: Paolo Abeni <pabeni@redhat.com> > Reviewed-by: Xin Long <lucien.xin@gmail.com> I'm hitting this as well, so applied, thanks! > > --- > Reviewed-by: Xin Long <lucien.xin@gmail.com> One review tag is enough ;) apparently patchwork doesn't know to dedup them :S
diff --git a/net/core/skbuff.c b/net/core/skbuff.c index e835193cabcc3..cf2c4dcf42579 100644 --- a/net/core/skbuff.c +++ b/net/core/skbuff.c @@ -3938,10 +3938,10 @@ struct sk_buff *skb_segment(struct sk_buff *head_skb, skb_release_head_state(nskb); __skb_push(nskb, doffset); } else { + if (hsize < 0) + hsize = 0; if (hsize > len || !sg) hsize = len; - else if (hsize < 0) - hsize = 0; nskb = __alloc_skb(hsize + doffset + headroom, GFP_ATOMIC, skb_alloc_rx_flag(head_skb),
The commit dbd50f238dec ("net: move the hsize check to the else block in skb_segment") introduced a data corruption for devices supporting scatter-gather. The problem boils down to signed/unsigned comparison given unexpected results: if signed 'hsize' is negative, it will be considered greater than a positive 'len', which is unsigned. This commit addresses resorting to the old checks order, so that 'hsize' never has a negative value when compared with 'len'. v1 -> v2: - reorder hsize checks instead of explicit cast (Alex) Bisected-by: Matthieu Baerts <matthieu.baerts@tessares.net> Fixes: dbd50f238dec ("net: move the hsize check to the else block in skb_segment") Signed-off-by: Paolo Abeni <pabeni@redhat.com> --- net/core/skbuff.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)