[PATCH net-next 0/3] udp gso fixes

[PATCH net-next 0/3] udp gso fixes

[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

[PATCH net-next 1/3] udp: exclude gso from xfrm paths

[Willem de Bruijn]

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

[PATCH net-next 2/3] gso: limit udp gso to egress-only virtual devices

[Willem de Bruijn]

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

Re: [PATCH net-next 2/3] gso: limit udp gso to egress-only virtual devices

[Willem de Bruijn]

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.

Re: [PATCH net-next 2/3] gso: limit udp gso to egress-only virtual devices

[kbuild test robot]

[-- 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 --]

[PATCH net-next 3/3] udp: only use paged allocation with scatter-gather

[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

Re: [PATCH net-next 3/3] udp: only use paged allocation with scatter-gather

[Eric Dumazet]

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.

Re: [PATCH net-next 3/3] udp: only use paged allocation with scatter-gather

[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.

Re: [PATCH net-next 3/3] udp: only use paged allocation with scatter-gather

[Eric Dumazet]

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.

Re: [PATCH net-next 3/3] udp: only use paged allocation with scatter-gather

[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.

Re: [PATCH net-next 3/3] udp: only use paged allocation with scatter-gather

[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.

Re: [PATCH net-next 3/3] udp: only use paged allocation with scatter-gather

[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;

Re: [PATCH net-next 3/3] udp: only use paged allocation with scatter-gather

[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.