| Message ID | 61306401471dcfc6219d5c001580769c2c67377a.1611059420.git.pabeni@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] net: fix GSO for SG-enabled devices. | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | warning | 9 maintainers not CCed: dseok.yi@samsung.com elver@google.com willemb@google.com kyk.segfault@gmail.com linmiaohe@huawei.com steffen.klassert@secunet.com jonathan.lemon@gmail.com gnault@redhat.com viro@zeniv.linux.org.uk |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 1 this patch: 1 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | warning | WARNING: Non-standard signature: Bisected-by: |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 1 this patch: 1 |
| netdev/header_inline | success | Link |
| netdev/stable | success | Stable not CCed |
> -----Original Message----- > From: Paolo Abeni <pabeni@redhat.com> > Sent: Tuesday, January 19, 2021 4:31 AM > To: netdev@vger.kernel.org > Cc: David S . Miller <davem@davemloft.net>; Jakub Kicinski > <kuba@kernel.org>; Alexander Duyck <alexanderduyck@fb.com>; Xin Long > <lucien.xin@gmail.com> > Subject: [PATCH net-next] net: fix GSO for SG-enabled devices. > > 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 the issue explicitly casting 'len' to a signed integer, so > that the comparison gives the correct result. > > 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> > --- > note: a possible more readable alternative would be moving the > if (hsize < 0) > > before 'if (hsize > len)', but that was explicitly discouraged in a previous > iteration of the blamed commit to save a comparison, so I opted to preserve > that optimization. I would say it is probably better to just moving the "hsize < 0" check. What I had suggested was a minor optimization and it hadn't occurred to me that this is a signed vs unsigned comparison. > --- > net/core/skbuff.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/net/core/skbuff.c b/net/core/skbuff.c index > e835193cabcc3..27f69c0bd8393 100644 > --- a/net/core/skbuff.c > +++ b/net/core/skbuff.c > @@ -3938,7 +3938,7 @@ struct sk_buff *skb_segment(struct sk_buff > *head_skb, > skb_release_head_state(nskb); > __skb_push(nskb, doffset); > } else { > - if (hsize > len || !sg) > + if (hsize > (int)len || !sg) > hsize = len; > else if (hsize < 0) > hsize = 0;
diff --git a/net/core/skbuff.c b/net/core/skbuff.c index e835193cabcc3..27f69c0bd8393 100644 --- a/net/core/skbuff.c +++ b/net/core/skbuff.c @@ -3938,7 +3938,7 @@ struct sk_buff *skb_segment(struct sk_buff *head_skb, skb_release_head_state(nskb); __skb_push(nskb, doffset); } else { - if (hsize > len || !sg) + if (hsize > (int)len || !sg) hsize = len; else if (hsize < 0) hsize = 0;
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 the issue explicitly casting 'len' to a signed integer, so that the comparison gives the correct result. 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> --- note: a possible more readable alternative would be moving the if (hsize < 0) before 'if (hsize > len)', but that was explicitly discouraged in a previous iteration of the blamed commit to save a comparison, so I opted to preserve that optimization. --- net/core/skbuff.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)