* [PATCH net-next 0/3] udp gso fixes
@ 2018-05-14 23:07 Willem de Bruijn
2018-05-14 23:07 ` [PATCH net-next 1/3] udp: exclude gso from xfrm paths Willem de Bruijn
` (2 more replies)
0 siblings, 3 replies; 13+ messages in thread
From: Willem de Bruijn @ 2018-05-14 23:07 UTC (permalink / raw)
To: netdev; +Cc: davem, Willem de Bruijn
From: Willem de Bruijn <willemb@google.com>
A few small fixes:
- disallow segmentation with XFRM
- do not leak gso packets into the ingress path
- fix a panic if scatter-gather is disabled
Willem de Bruijn (3):
udp: exclude gso from xfrm paths
gso: limit udp gso to egress-only virtual devices
udp: only use paged allocation with scatter-gather
drivers/net/bonding/bond_main.c | 5 +++--
drivers/net/team/team.c | 5 +++--
include/linux/netdev_features.h | 1 -
net/ipv4/ip_output.c | 2 +-
net/ipv4/udp.c | 3 ++-
net/ipv6/ip6_output.c | 2 +-
net/ipv6/udp.c | 3 ++-
7 files changed, 12 insertions(+), 9 deletions(-)
--
2.17.0.441.gb46fe60e1d-goog
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH net-next 1/3] udp: exclude gso from xfrm paths
2018-05-14 23:07 [PATCH net-next 0/3] udp gso fixes Willem de Bruijn
@ 2018-05-14 23:07 ` Willem de Bruijn
2018-05-14 23:07 ` [PATCH net-next 2/3] gso: limit udp gso to egress-only virtual devices Willem de Bruijn
2018-05-14 23:07 ` [PATCH net-next 3/3] udp: only use paged allocation with scatter-gather Willem de Bruijn
2 siblings, 0 replies; 13+ messages in thread
From: Willem de Bruijn @ 2018-05-14 23:07 UTC (permalink / raw)
To: netdev; +Cc: davem, Willem de Bruijn, Michal Kubecek
From: Willem de Bruijn <willemb@google.com>
UDP GSO conflicts with transformations in the XFRM layer.
Return an error if GSO is attempted.
Fixes: bec1f6f69736 ("udp: generate gso with UDP_SEGMENT")
CC: Michal Kubecek <mkubecek@suse.cz>
Signed-off-by: Willem de Bruijn <willemb@google.com>
---
net/ipv4/udp.c | 3 ++-
net/ipv6/udp.c | 3 ++-
2 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
index ff4d4ba67735..d71f1f3e1155 100644
--- a/net/ipv4/udp.c
+++ b/net/ipv4/udp.c
@@ -788,7 +788,8 @@ static int udp_send_skb(struct sk_buff *skb, struct flowi4 *fl4,
return -EINVAL;
if (sk->sk_no_check_tx)
return -EINVAL;
- if (skb->ip_summed != CHECKSUM_PARTIAL || is_udplite)
+ if (skb->ip_summed != CHECKSUM_PARTIAL || is_udplite ||
+ dst_xfrm(skb_dst(skb)))
return -EIO;
skb_shinfo(skb)->gso_size = cork->gso_size;
diff --git a/net/ipv6/udp.c b/net/ipv6/udp.c
index 2839c1bd1e58..426c9d2b418d 100644
--- a/net/ipv6/udp.c
+++ b/net/ipv6/udp.c
@@ -1053,7 +1053,8 @@ static int udp_v6_send_skb(struct sk_buff *skb, struct flowi6 *fl6,
return -EINVAL;
if (udp_sk(sk)->no_check6_tx)
return -EINVAL;
- if (skb->ip_summed != CHECKSUM_PARTIAL || is_udplite)
+ if (skb->ip_summed != CHECKSUM_PARTIAL || is_udplite ||
+ dst_xfrm(skb_dst(skb)))
return -EIO;
skb_shinfo(skb)->gso_size = cork->gso_size;
--
2.17.0.441.gb46fe60e1d-goog
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH net-next 2/3] gso: limit udp gso to egress-only virtual devices
2018-05-14 23:07 [PATCH net-next 0/3] udp gso fixes Willem de Bruijn
2018-05-14 23:07 ` [PATCH net-next 1/3] udp: exclude gso from xfrm paths Willem de Bruijn
@ 2018-05-14 23:07 ` Willem de Bruijn
2018-05-14 23:12 ` Willem de Bruijn
2018-05-15 6:34 ` kbuild test robot
2018-05-14 23:07 ` [PATCH net-next 3/3] udp: only use paged allocation with scatter-gather Willem de Bruijn
2 siblings, 2 replies; 13+ messages in thread
From: Willem de Bruijn @ 2018-05-14 23:07 UTC (permalink / raw)
To: netdev; +Cc: davem, Willem de Bruijn, Alexander Duyck
From: Willem de Bruijn <willemb@google.com>
Until the udp receive stack supports large packets (UDP GRO), GSO
packets must not loop from the egress to the ingress path.
Revert the change that added NETIF_F_GSO_UDP_L4 to various virtual
devices through NETIF_F_GSO_ENCAP_ALL as this included devices that
may loop packets, such as veth and macvlan.
Instead add it to specific devices that forward to another device's
egress path: bonding and team.
Fixes: 83aa025f535f ("udp: add gso support to virtual devices")
CC: Alexander Duyck <alexander.duyck@gmail.com>
Signed-off-by: Willem de Bruijn <willemb@google.com>
---
drivers/net/bonding/bond_main.c | 5 +++--
drivers/net/team/team.c | 5 +++--
include/linux/netdev_features.h | 1 -
3 files changed, 6 insertions(+), 5 deletions(-)
diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 4176e1d95f47..d7b58370ae77 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -1107,7 +1107,8 @@ static void bond_compute_features(struct bonding *bond)
done:
bond_dev->vlan_features = vlan_features;
- bond_dev->hw_enc_features = enc_features | NETIF_F_GSO_ENCAP_ALL;
+ bond_dev->hw_enc_features = enc_features | NETIF_F_GSO_ENCAP_ALL |
+ NETIF_F_GSO_UDP_L4;
bond_dev->gso_max_segs = gso_max_segs;
netif_set_gso_max_size(bond_dev, gso_max_size);
@@ -4263,7 +4264,7 @@ void bond_setup(struct net_device *bond_dev)
NETIF_F_HW_VLAN_CTAG_RX |
NETIF_F_HW_VLAN_CTAG_FILTER;
- bond_dev->hw_features |= NETIF_F_GSO_ENCAP_ALL;
+ bond_dev->hw_features |= NETIF_F_GSO_ENCAP_ALL | NETIF_F_GSO_UDP_L4;
bond_dev->features |= bond_dev->hw_features;
}
diff --git a/drivers/net/team/team.c b/drivers/net/team/team.c
index 9dbd390ace34..c6a9f0cafea2 100644
--- a/drivers/net/team/team.c
+++ b/drivers/net/team/team.c
@@ -1026,7 +1026,8 @@ static void __team_compute_features(struct team *team)
}
team->dev->vlan_features = vlan_features;
- team->dev->hw_enc_features = enc_features | NETIF_F_GSO_ENCAP_ALL;
+ team->dev->hw_enc_features = enc_features | NETIF_F_GSO_ENCAP_ALL |
+ NETIF_GSO_UDP_L4;
team->dev->hard_header_len = max_hard_header_len;
team->dev->priv_flags &= ~IFF_XMIT_DST_RELEASE;
@@ -2117,7 +2118,7 @@ static void team_setup(struct net_device *dev)
NETIF_F_HW_VLAN_CTAG_RX |
NETIF_F_HW_VLAN_CTAG_FILTER;
- dev->hw_features |= NETIF_F_GSO_ENCAP_ALL;
+ dev->hw_features |= NETIF_F_GSO_ENCAP_ALL | NETIF_F_GSO_UDP_L4;
dev->features |= dev->hw_features;
}
diff --git a/include/linux/netdev_features.h b/include/linux/netdev_features.h
index c87c3a3453c1..623bb8ced060 100644
--- a/include/linux/netdev_features.h
+++ b/include/linux/netdev_features.h
@@ -220,7 +220,6 @@ enum {
NETIF_F_GSO_GRE_CSUM | \
NETIF_F_GSO_IPXIP4 | \
NETIF_F_GSO_IPXIP6 | \
- NETIF_F_GSO_UDP_L4 | \
NETIF_F_GSO_UDP_TUNNEL | \
NETIF_F_GSO_UDP_TUNNEL_CSUM)
--
2.17.0.441.gb46fe60e1d-goog
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH net-next 3/3] udp: only use paged allocation with scatter-gather
2018-05-14 23:07 [PATCH net-next 0/3] udp gso fixes Willem de Bruijn
2018-05-14 23:07 ` [PATCH net-next 1/3] udp: exclude gso from xfrm paths Willem de Bruijn
2018-05-14 23:07 ` [PATCH net-next 2/3] gso: limit udp gso to egress-only virtual devices Willem de Bruijn
@ 2018-05-14 23:07 ` Willem de Bruijn
2018-05-14 23:12 ` Eric Dumazet
2 siblings, 1 reply; 13+ messages in thread
From: Willem de Bruijn @ 2018-05-14 23:07 UTC (permalink / raw)
To: netdev; +Cc: davem, Willem de Bruijn
From: Willem de Bruijn <willemb@google.com>
Paged allocation stores most payload in skb frags. This helps udp gso
by avoiding copying from the gso skb to segment skb in skb_segment.
But without scatter-gather, data must be linear, so do not use paged
mode unless NETIF_F_SG.
Fixes: 15e36f5b8e98 ("udp: paged allocation with gso")
Reported-by: Sean Tranchetti <stranche@codeaurora.org>
Signed-off-by: Willem de Bruijn <willemb@google.com>
---
net/ipv4/ip_output.c | 2 +-
net/ipv6/ip6_output.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/net/ipv4/ip_output.c b/net/ipv4/ip_output.c
index b5e21eb198d8..b38731d8a44f 100644
--- a/net/ipv4/ip_output.c
+++ b/net/ipv4/ip_output.c
@@ -884,7 +884,7 @@ static int __ip_append_data(struct sock *sk,
exthdrlen = !skb ? rt->dst.header_len : 0;
mtu = cork->gso_size ? IP_MAX_MTU : cork->fragsize;
- paged = !!cork->gso_size;
+ paged = cork->gso_size && (rt->dst.dev->features & NETIF_F_SG);
if (cork->tx_flags & SKBTX_ANY_SW_TSTAMP &&
sk->sk_tsflags & SOF_TIMESTAMPING_OPT_ID)
diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c
index 7f4493080df6..35a940b9f208 100644
--- a/net/ipv6/ip6_output.c
+++ b/net/ipv6/ip6_output.c
@@ -1262,7 +1262,7 @@ static int __ip6_append_data(struct sock *sk,
dst_exthdrlen = rt->dst.header_len - rt->rt6i_nfheader_len;
}
- paged = !!cork->gso_size;
+ paged = cork->gso_size && (rt->dst.dev->features & NETIF_F_SG);
mtu = cork->gso_size ? IP6_MAX_MTU : cork->fragsize;
orig_mtu = mtu;
--
2.17.0.441.gb46fe60e1d-goog
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH net-next 3/3] udp: only use paged allocation with scatter-gather
2018-05-14 23:07 ` [PATCH net-next 3/3] udp: only use paged allocation with scatter-gather Willem de Bruijn
@ 2018-05-14 23:12 ` Eric Dumazet
2018-05-14 23:30 ` Willem de Bruijn
0 siblings, 1 reply; 13+ messages in thread
From: Eric Dumazet @ 2018-05-14 23:12 UTC (permalink / raw)
To: Willem de Bruijn, netdev; +Cc: davem, Willem de Bruijn
On 05/14/2018 04:07 PM, Willem de Bruijn wrote:
> From: Willem de Bruijn <willemb@google.com>
>
> Paged allocation stores most payload in skb frags. This helps udp gso
> by avoiding copying from the gso skb to segment skb in skb_segment.
>
> But without scatter-gather, data must be linear, so do not use paged
> mode unless NETIF_F_SG.
>
> Fixes: 15e36f5b8e98 ("udp: paged allocation with gso")
> Reported-by: Sean Tranchetti <stranche@codeaurora.org>
> Signed-off-by: Willem de Bruijn <willemb@google.com>
> ---
> net/ipv4/ip_output.c | 2 +-
> net/ipv6/ip6_output.c | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/net/ipv4/ip_output.c b/net/ipv4/ip_output.c
> index b5e21eb198d8..b38731d8a44f 100644
> --- a/net/ipv4/ip_output.c
> +++ b/net/ipv4/ip_output.c
> @@ -884,7 +884,7 @@ static int __ip_append_data(struct sock *sk,
>
> exthdrlen = !skb ? rt->dst.header_len : 0;
> mtu = cork->gso_size ? IP_MAX_MTU : cork->fragsize;
> - paged = !!cork->gso_size;
> + paged = cork->gso_size && (rt->dst.dev->features & NETIF_F_SG);
>
> if (cork->tx_flags & SKBTX_ANY_SW_TSTAMP &&
> sk->sk_tsflags & SOF_TIMESTAMPING_OPT_ID)
> diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c
> index 7f4493080df6..35a940b9f208 100644
> --- a/net/ipv6/ip6_output.c
> +++ b/net/ipv6/ip6_output.c
> @@ -1262,7 +1262,7 @@ static int __ip6_append_data(struct sock *sk,
> dst_exthdrlen = rt->dst.header_len - rt->rt6i_nfheader_len;
> }
>
> - paged = !!cork->gso_size;
> + paged = cork->gso_size && (rt->dst.dev->features & NETIF_F_SG);
> mtu = cork->gso_size ? IP6_MAX_MTU : cork->fragsize;
> orig_mtu = mtu;
>
>
As I said, this wont help for stacked device
bonding might advertise NETIF_F_SG, but one slave might not.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH net-next 2/3] gso: limit udp gso to egress-only virtual devices
2018-05-14 23:07 ` [PATCH net-next 2/3] gso: limit udp gso to egress-only virtual devices Willem de Bruijn
@ 2018-05-14 23:12 ` Willem de Bruijn
2018-05-15 6:34 ` kbuild test robot
1 sibling, 0 replies; 13+ messages in thread
From: Willem de Bruijn @ 2018-05-14 23:12 UTC (permalink / raw)
To: Network Development; +Cc: David Miller, Willem de Bruijn, Alexander Duyck
On Mon, May 14, 2018 at 7:07 PM, Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
> From: Willem de Bruijn <willemb@google.com>
>
> Until the udp receive stack supports large packets (UDP GRO), GSO
> packets must not loop from the egress to the ingress path.
>
> Revert the change that added NETIF_F_GSO_UDP_L4 to various virtual
> devices through NETIF_F_GSO_ENCAP_ALL as this included devices that
> may loop packets, such as veth and macvlan.
>
> Instead add it to specific devices that forward to another device's
> egress path: bonding and team.
>
> Fixes: 83aa025f535f ("udp: add gso support to virtual devices")
> CC: Alexander Duyck <alexander.duyck@gmail.com>
> Signed-off-by: Willem de Bruijn <willemb@google.com>
> ---
> diff --git a/drivers/net/team/team.c b/drivers/net/team/team.c
> index 9dbd390ace34..c6a9f0cafea2 100644
> --- a/drivers/net/team/team.c
> +++ b/drivers/net/team/team.c
> @@ -1026,7 +1026,8 @@ static void __team_compute_features(struct team *team)
> }
>
> team->dev->vlan_features = vlan_features;
> - team->dev->hw_enc_features = enc_features | NETIF_F_GSO_ENCAP_ALL;
> + team->dev->hw_enc_features = enc_features | NETIF_F_GSO_ENCAP_ALL |
> + NETIF_GSO_UDP_L4;
This has a typo. team.ko did not build automatically for me and caught it
with a full compile just too late.
Need to send a v2, sorry.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH net-next 3/3] udp: only use paged allocation with scatter-gather
2018-05-14 23:12 ` Eric Dumazet
@ 2018-05-14 23:30 ` Willem de Bruijn
2018-05-14 23:45 ` Eric Dumazet
0 siblings, 1 reply; 13+ messages in thread
From: Willem de Bruijn @ 2018-05-14 23:30 UTC (permalink / raw)
To: Eric Dumazet; +Cc: Network Development, David Miller, Willem de Bruijn
On Mon, May 14, 2018 at 7:12 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>
>
> On 05/14/2018 04:07 PM, Willem de Bruijn wrote:
>> From: Willem de Bruijn <willemb@google.com>
>>
>> Paged allocation stores most payload in skb frags. This helps udp gso
>> by avoiding copying from the gso skb to segment skb in skb_segment.
>>
>> But without scatter-gather, data must be linear, so do not use paged
>> mode unless NETIF_F_SG.
>>
>> Fixes: 15e36f5b8e98 ("udp: paged allocation with gso")
>> Reported-by: Sean Tranchetti <stranche@codeaurora.org>
>> Signed-off-by: Willem de Bruijn <willemb@google.com>
>> ---
>> net/ipv4/ip_output.c | 2 +-
>> net/ipv6/ip6_output.c | 2 +-
>> 2 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/net/ipv4/ip_output.c b/net/ipv4/ip_output.c
>> index b5e21eb198d8..b38731d8a44f 100644
>> --- a/net/ipv4/ip_output.c
>> +++ b/net/ipv4/ip_output.c
>> @@ -884,7 +884,7 @@ static int __ip_append_data(struct sock *sk,
>>
>> exthdrlen = !skb ? rt->dst.header_len : 0;
>> mtu = cork->gso_size ? IP_MAX_MTU : cork->fragsize;
>> - paged = !!cork->gso_size;
>> + paged = cork->gso_size && (rt->dst.dev->features & NETIF_F_SG);
>>
>> if (cork->tx_flags & SKBTX_ANY_SW_TSTAMP &&
>> sk->sk_tsflags & SOF_TIMESTAMPING_OPT_ID)
>> diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c
>> index 7f4493080df6..35a940b9f208 100644
>> --- a/net/ipv6/ip6_output.c
>> +++ b/net/ipv6/ip6_output.c
>> @@ -1262,7 +1262,7 @@ static int __ip6_append_data(struct sock *sk,
>> dst_exthdrlen = rt->dst.header_len - rt->rt6i_nfheader_len;
>> }
>>
>> - paged = !!cork->gso_size;
>> + paged = cork->gso_size && (rt->dst.dev->features & NETIF_F_SG);
>> mtu = cork->gso_size ? IP6_MAX_MTU : cork->fragsize;
>> orig_mtu = mtu;
>>
>>
>
> As I said, this wont help for stacked device
>
> bonding might advertise NETIF_F_SG, but one slave might not.
I don't quite follow. The reported crash happens in the protocol layer,
because of this check. With pagedlen we have not allocated
sufficient space for the skb_put.
if (!(rt->dst.dev->features&NETIF_F_SG)) {
unsigned int off;
off = skb->len;
if (getfrag(from, skb_put(skb, copy),
offset, copy, off, skb) < 0) {
__skb_trim(skb, off);
err = -EFAULT;
goto error;
}
} else {
int i = skb_shinfo(skb)->nr_frags;
Are you referring to a separate potential issue in the gso layer?
If a bonding device advertises SG, but a slave does not, then
skb_segment on the slave should build linear segs? I have not
tested that.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH net-next 3/3] udp: only use paged allocation with scatter-gather
2018-05-14 23:30 ` Willem de Bruijn
@ 2018-05-14 23:45 ` Eric Dumazet
2018-05-15 14:14 ` Willem de Bruijn
0 siblings, 1 reply; 13+ messages in thread
From: Eric Dumazet @ 2018-05-14 23:45 UTC (permalink / raw)
To: Willem de Bruijn, Eric Dumazet
Cc: Network Development, David Miller, Willem de Bruijn
On 05/14/2018 04:30 PM, Willem de Bruijn wrote:
> I don't quite follow. The reported crash happens in the protocol layer,
> because of this check. With pagedlen we have not allocated
> sufficient space for the skb_put.
>
> if (!(rt->dst.dev->features&NETIF_F_SG)) {
> unsigned int off;
>
> off = skb->len;
> if (getfrag(from, skb_put(skb, copy),
> offset, copy, off, skb) < 0) {
> __skb_trim(skb, off);
> err = -EFAULT;
> goto error;
> }
> } else {
> int i = skb_shinfo(skb)->nr_frags;
>
> Are you referring to a separate potential issue in the gso layer?
> If a bonding device advertises SG, but a slave does not, then
> skb_segment on the slave should build linear segs? I have not
> tested that.
Given that the device attribute could change under us, we need to not
crash, even if initially we thought NETIF_F_SG was available.
Unless you want to hold RTNL in UDP xmit :)
Ideally, GSO should be always on, as we did for TCP.
Otherwise, I can guarantee syzkaller will hit again.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH net-next 2/3] gso: limit udp gso to egress-only virtual devices
2018-05-14 23:07 ` [PATCH net-next 2/3] gso: limit udp gso to egress-only virtual devices Willem de Bruijn
2018-05-14 23:12 ` Willem de Bruijn
@ 2018-05-15 6:34 ` kbuild test robot
1 sibling, 0 replies; 13+ messages in thread
From: kbuild test robot @ 2018-05-15 6:34 UTC (permalink / raw)
To: Willem de Bruijn
Cc: kbuild-all, netdev, davem, Willem de Bruijn, Alexander Duyck
[-- Attachment #1: Type: text/plain, Size: 2893 bytes --]
Hi Willem,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on net-next/master]
url: https://github.com/0day-ci/linux/commits/Willem-de-Bruijn/udp-gso-fixes/20180515-120246
config: xtensa-allmodconfig (attached as .config)
compiler: xtensa-linux-gcc (GCC) 7.2.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=xtensa
All errors (new ones prefixed by >>):
drivers/net//team/team.c: In function '__team_compute_features':
>> drivers/net//team/team.c:1030:10: error: 'NETIF_GSO_UDP_L4' undeclared (first use in this function); did you mean 'NETIF_F_GSO_UDP_L4'?
NETIF_GSO_UDP_L4;
^~~~~~~~~~~~~~~~
NETIF_F_GSO_UDP_L4
drivers/net//team/team.c:1030:10: note: each undeclared identifier is reported only once for each function it appears in
vim +1030 drivers/net//team/team.c
996
997 #define TEAM_VLAN_FEATURES (NETIF_F_HW_CSUM | NETIF_F_SG | \
998 NETIF_F_FRAGLIST | NETIF_F_ALL_TSO | \
999 NETIF_F_HIGHDMA | NETIF_F_LRO)
1000
1001 #define TEAM_ENC_FEATURES (NETIF_F_HW_CSUM | NETIF_F_SG | \
1002 NETIF_F_RXCSUM | NETIF_F_ALL_TSO)
1003
1004 static void __team_compute_features(struct team *team)
1005 {
1006 struct team_port *port;
1007 u32 vlan_features = TEAM_VLAN_FEATURES & NETIF_F_ALL_FOR_ALL;
1008 netdev_features_t enc_features = TEAM_ENC_FEATURES;
1009 unsigned short max_hard_header_len = ETH_HLEN;
1010 unsigned int dst_release_flag = IFF_XMIT_DST_RELEASE |
1011 IFF_XMIT_DST_RELEASE_PERM;
1012
1013 list_for_each_entry(port, &team->port_list, list) {
1014 vlan_features = netdev_increment_features(vlan_features,
1015 port->dev->vlan_features,
1016 TEAM_VLAN_FEATURES);
1017 enc_features =
1018 netdev_increment_features(enc_features,
1019 port->dev->hw_enc_features,
1020 TEAM_ENC_FEATURES);
1021
1022
1023 dst_release_flag &= port->dev->priv_flags;
1024 if (port->dev->hard_header_len > max_hard_header_len)
1025 max_hard_header_len = port->dev->hard_header_len;
1026 }
1027
1028 team->dev->vlan_features = vlan_features;
1029 team->dev->hw_enc_features = enc_features | NETIF_F_GSO_ENCAP_ALL |
> 1030 NETIF_GSO_UDP_L4;
1031 team->dev->hard_header_len = max_hard_header_len;
1032
1033 team->dev->priv_flags &= ~IFF_XMIT_DST_RELEASE;
1034 if (dst_release_flag == (IFF_XMIT_DST_RELEASE | IFF_XMIT_DST_RELEASE_PERM))
1035 team->dev->priv_flags |= IFF_XMIT_DST_RELEASE;
1036 }
1037
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 52976 bytes --]
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH net-next 3/3] udp: only use paged allocation with scatter-gather
2018-05-14 23:45 ` Eric Dumazet
@ 2018-05-15 14:14 ` Willem de Bruijn
2018-05-15 20:04 ` Willem de Bruijn
0 siblings, 1 reply; 13+ messages in thread
From: Willem de Bruijn @ 2018-05-15 14:14 UTC (permalink / raw)
To: Eric Dumazet; +Cc: Network Development, David Miller, Willem de Bruijn
On Mon, May 14, 2018 at 7:45 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>
>
> On 05/14/2018 04:30 PM, Willem de Bruijn wrote:
>
>> I don't quite follow. The reported crash happens in the protocol layer,
>> because of this check. With pagedlen we have not allocated
>> sufficient space for the skb_put.
>>
>> if (!(rt->dst.dev->features&NETIF_F_SG)) {
>> unsigned int off;
>>
>> off = skb->len;
>> if (getfrag(from, skb_put(skb, copy),
>> offset, copy, off, skb) < 0) {
>> __skb_trim(skb, off);
>> err = -EFAULT;
>> goto error;
>> }
>> } else {
>> int i = skb_shinfo(skb)->nr_frags;
>>
>> Are you referring to a separate potential issue in the gso layer?
>> If a bonding device advertises SG, but a slave does not, then
>> skb_segment on the slave should build linear segs? I have not
>> tested that.
>
> Given that the device attribute could change under us, we need to not
> crash, even if initially we thought NETIF_F_SG was available.
>
> Unless you want to hold RTNL in UDP xmit :)
>
> Ideally, GSO should be always on, as we did for TCP.
>
> Otherwise, I can guarantee syzkaller will hit again.
Ah, right. Thanks, Eric!
I'll read that feature bit only once.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH net-next 3/3] udp: only use paged allocation with scatter-gather
2018-05-15 14:14 ` Willem de Bruijn
@ 2018-05-15 20:04 ` Willem de Bruijn
2018-05-15 23:57 ` Willem de Bruijn
0 siblings, 1 reply; 13+ messages in thread
From: Willem de Bruijn @ 2018-05-15 20:04 UTC (permalink / raw)
To: Eric Dumazet; +Cc: Network Development, David Miller, Willem de Bruijn
On Tue, May 15, 2018 at 10:14 AM, Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
> On Mon, May 14, 2018 at 7:45 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>>
>>
>> On 05/14/2018 04:30 PM, Willem de Bruijn wrote:
>>
>>> I don't quite follow. The reported crash happens in the protocol layer,
>>> because of this check. With pagedlen we have not allocated
>>> sufficient space for the skb_put.
>>>
>>> if (!(rt->dst.dev->features&NETIF_F_SG)) {
>>> unsigned int off;
>>>
>>> off = skb->len;
>>> if (getfrag(from, skb_put(skb, copy),
>>> offset, copy, off, skb) < 0) {
>>> __skb_trim(skb, off);
>>> err = -EFAULT;
>>> goto error;
>>> }
>>> } else {
>>> int i = skb_shinfo(skb)->nr_frags;
>>>
>>> Are you referring to a separate potential issue in the gso layer?
>>> If a bonding device advertises SG, but a slave does not, then
>>> skb_segment on the slave should build linear segs? I have not
>>> tested that.
>>
>> Given that the device attribute could change under us, we need to not
>> crash, even if initially we thought NETIF_F_SG was available.
>>
>> Unless you want to hold RTNL in UDP xmit :)
>>
>> Ideally, GSO should be always on, as we did for TCP.
>>
>> Otherwise, I can guarantee syzkaller will hit again.
>
> Ah, right. Thanks, Eric!
>
> I'll read that feature bit only once.
This issue is actually deeper and not specific to gso.
With corking it is trivial to turn off sg in between calls.
I'll need to send a separate fix for that.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH net-next 3/3] udp: only use paged allocation with scatter-gather
2018-05-15 20:04 ` Willem de Bruijn
@ 2018-05-15 23:57 ` Willem de Bruijn
2018-05-16 20:10 ` Willem de Bruijn
0 siblings, 1 reply; 13+ messages in thread
From: Willem de Bruijn @ 2018-05-15 23:57 UTC (permalink / raw)
To: Eric Dumazet; +Cc: Network Development, David Miller, Willem de Bruijn
On Tue, May 15, 2018 at 4:04 PM, Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
> On Tue, May 15, 2018 at 10:14 AM, Willem de Bruijn
> <willemdebruijn.kernel@gmail.com> wrote:
>> On Mon, May 14, 2018 at 7:45 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>>>
>>>
>>> On 05/14/2018 04:30 PM, Willem de Bruijn wrote:
>>>
>>>> I don't quite follow. The reported crash happens in the protocol layer,
>>>> because of this check. With pagedlen we have not allocated
>>>> sufficient space for the skb_put.
>>>>
>>>> if (!(rt->dst.dev->features&NETIF_F_SG)) {
>>>> unsigned int off;
>>>>
>>>> off = skb->len;
>>>> if (getfrag(from, skb_put(skb, copy),
>>>> offset, copy, off, skb) < 0) {
>>>> __skb_trim(skb, off);
>>>> err = -EFAULT;
>>>> goto error;
>>>> }
>>>> } else {
>>>> int i = skb_shinfo(skb)->nr_frags;
>>>>
>>>> Are you referring to a separate potential issue in the gso layer?
>>>> If a bonding device advertises SG, but a slave does not, then
>>>> skb_segment on the slave should build linear segs? I have not
>>>> tested that.
>>>
>>> Given that the device attribute could change under us, we need to not
>>> crash, even if initially we thought NETIF_F_SG was available.
>>>
>>> Unless you want to hold RTNL in UDP xmit :)
>>>
>>> Ideally, GSO should be always on, as we did for TCP.
>>>
>>> Otherwise, I can guarantee syzkaller will hit again.
>>
>> Ah, right. Thanks, Eric!
>>
>> I'll read that feature bit only once.
>
> This issue is actually deeper and not specific to gso.
> With corking it is trivial to turn off sg in between calls.
>
> I'll need to send a separate fix for that.
This would do it. The extra branch is unfortunate, but I see no easy
way around it for the corking case.
It will obviously not build a linear skb, but validate_xmit_skb will clean
that up for such edge cases.
diff --git a/net/ipv4/ip_output.c b/net/ipv4/ip_output.c
index 66340ab750e6..e7daec7c7421 100644
--- a/net/ipv4/ip_output.c
+++ b/net/ipv4/ip_output.c
@@ -1040,7 +1040,8 @@ static int __ip_append_data(struct sock *sk,
if (copy > length)
copy = length;
- if (!(rt->dst.dev->features&NETIF_F_SG)) {
+ if (!(rt->dst.dev->features&NETIF_F_SG) &&
+ skb_tailroom(skb) >= copy) {
unsigned int off;
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH net-next 3/3] udp: only use paged allocation with scatter-gather
2018-05-15 23:57 ` Willem de Bruijn
@ 2018-05-16 20:10 ` Willem de Bruijn
0 siblings, 0 replies; 13+ messages in thread
From: Willem de Bruijn @ 2018-05-16 20:10 UTC (permalink / raw)
To: Eric Dumazet; +Cc: Network Development, David Miller, Willem de Bruijn
On Tue, May 15, 2018 at 7:57 PM, Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
> On Tue, May 15, 2018 at 4:04 PM, Willem de Bruijn
> <willemdebruijn.kernel@gmail.com> wrote:
>> On Tue, May 15, 2018 at 10:14 AM, Willem de Bruijn
>> <willemdebruijn.kernel@gmail.com> wrote:
>>> On Mon, May 14, 2018 at 7:45 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>>>>
>>>>
>>>> On 05/14/2018 04:30 PM, Willem de Bruijn wrote:
>>>>
>>>>> I don't quite follow. The reported crash happens in the protocol layer,
>>>>> because of this check. With pagedlen we have not allocated
>>>>> sufficient space for the skb_put.
>>>>>
>>>>> if (!(rt->dst.dev->features&NETIF_F_SG)) {
>>>>> unsigned int off;
>>>>>
>>>>> off = skb->len;
>>>>> if (getfrag(from, skb_put(skb, copy),
>>>>> offset, copy, off, skb) < 0) {
>>>>> __skb_trim(skb, off);
>>>>> err = -EFAULT;
>>>>> goto error;
>>>>> }
>>>>> } else {
>>>>> int i = skb_shinfo(skb)->nr_frags;
>>>>>
>>>>> Are you referring to a separate potential issue in the gso layer?
>>>>> If a bonding device advertises SG, but a slave does not, then
>>>>> skb_segment on the slave should build linear segs? I have not
>>>>> tested that.
>>>>
>>>> Given that the device attribute could change under us, we need to not
>>>> crash, even if initially we thought NETIF_F_SG was available.
>>>>
>>>> Unless you want to hold RTNL in UDP xmit :)
>>>>
>>>> Ideally, GSO should be always on, as we did for TCP.
>>>>
>>>> Otherwise, I can guarantee syzkaller will hit again.
>>>
>>> Ah, right. Thanks, Eric!
>>>
>>> I'll read that feature bit only once.
>>
>> This issue is actually deeper and not specific to gso.
>> With corking it is trivial to turn off sg in between calls.
>>
>> I'll need to send a separate fix for that.
>
> This would do it. The extra branch is unfortunate, but I see no easy
> way around it for the corking case.
>
> It will obviously not build a linear skb, but validate_xmit_skb will clean
> that up for such edge cases.
>
> diff --git a/net/ipv4/ip_output.c b/net/ipv4/ip_output.c
> index 66340ab750e6..e7daec7c7421 100644
> --- a/net/ipv4/ip_output.c
> +++ b/net/ipv4/ip_output.c
> @@ -1040,7 +1040,8 @@ static int __ip_append_data(struct sock *sk,
> if (copy > length)
> copy = length;
>
> - if (!(rt->dst.dev->features&NETIF_F_SG)) {
> + if (!(rt->dst.dev->features&NETIF_F_SG) &&
> + skb_tailroom(skb) >= copy) {
> unsigned int off;
Reminder that this is a separate draft patch to net unrelated to gso.
A simpler branch
> - if (!(rt->dst.dev->features&NETIF_F_SG)) {
> + if (skb_tailroom(skb) >= copy) {
is probably sufficient, but might have subtle side-effects when SG is
off, where allocation padding allows data to fit that would currently is
added as frag. Risky for a stable patch with no significant benefit.
On the other extreme, I can define
bool sg = rt->dst.dev->features & NETIF_F_SG;
and refer to that in both current sites that test the flag. But this
will not help the corking case where the function is entered twice
for the same skb. I'll add that in the net-next gso fix where the flag
is tested three times.
But intend to send this snippet (also for v6) as is.
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2018-05-16 20:11 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-05-14 23:07 [PATCH net-next 0/3] udp gso fixes Willem de Bruijn
2018-05-14 23:07 ` [PATCH net-next 1/3] udp: exclude gso from xfrm paths Willem de Bruijn
2018-05-14 23:07 ` [PATCH net-next 2/3] gso: limit udp gso to egress-only virtual devices Willem de Bruijn
2018-05-14 23:12 ` Willem de Bruijn
2018-05-15 6:34 ` kbuild test robot
2018-05-14 23:07 ` [PATCH net-next 3/3] udp: only use paged allocation with scatter-gather Willem de Bruijn
2018-05-14 23:12 ` Eric Dumazet
2018-05-14 23:30 ` Willem de Bruijn
2018-05-14 23:45 ` Eric Dumazet
2018-05-15 14:14 ` Willem de Bruijn
2018-05-15 20:04 ` Willem de Bruijn
2018-05-15 23:57 ` Willem de Bruijn
2018-05-16 20:10 ` Willem de Bruijn
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.