Re: [PATCH net] net: accept UFO datagrams from tuntap and packet
From: Jason Wang Date: Mon, 20 Nov 2017 11:03:54 +0800 > Looks good to me. The only concern is that whether or not stable can > accept this patch: Stable submission will not be a problem, please do not worry about it :-)
Re: [PATCH net] net: accept UFO datagrams from tuntap and packet
On 2017年11月18日 06:59, Willem de Bruijn wrote:
From: Willem de Bruijn
Tuntap and similar devices can inject GSO packets. Accept type
VIRTIO_NET_HDR_GSO_UDP, even though not generating UFO natively.
Processes are expected to use feature negotiation such as TUNSETOFFLOAD
to detect supported offload types and refrain from injecting other
packets. This process breaks down with live migration: guest kernels
do not renegotiate flags, so destination hosts need to expose all
features that the source host does.
Partially revert the UFO removal from 182e0b6b5846~1..d9d30adf5677.
This patch introduces nearly(*) no new code to simplify verification.
It brings back verbatim tuntap UFO negotiation, VIRTIO_NET_HDR_GSO_UDP
insertion and software UFO segmentation.
It does not reinstate protocol stack support, hardware offload
(NETIF_F_UFO), SKB_GSO_UDP tunneling in SKB_GSO_SOFTWARE or reception
of VIRTIO_NET_HDR_GSO_UDP packets in tuntap.
To support SKB_GSO_UDP reappearing in the stack, also reinstate
logic in act_csum and openvswitch. Achieve equivalence with v4.13 HEAD
by squashing in commit 939912216fa8 ("net: skb_needs_check() removes
CHECKSUM_UNNECESSARY check for tx.") and reverting commit 8d63bee643f1
("net: avoid skb_warn_bad_offload false positives on UFO").
(*) To avoid having to bring back skb_shinfo(skb)->ip6_frag_id,
ipv6_proxy_select_ident is changed to return a __be32, which is
assigned directly to the frag_hdr. Also, SKB_GSO_UDP is inserted
at the end of the enum to minimize code churn.
Link:http://lkml.kernel.org/r/
Fixes: fb652fdfe837 ("macvlan/macvtap: Remove NETIF_F_UFO advertisement.")
Reported-by: Michal Kubecek
Signed-off-by: Willem de Bruijn
Looks good to me. The only concern is that whether or not stable can
accept this patch:
stable-kernel-rules.rst said:
"
- It cannot be bigger than 100 lines, with context.
"
Otherwise we may consider the things like doing segmentation in
tun_get_user() mentioned in another thread by you.
Thanks
Re: [PATCH net] net: accept UFO datagrams from tuntap and packet
On Fri, Nov 17, 2017 at 8:42 PM, David Miller wrote:
> From: Willem de Bruijn
> Date: Fri, 17 Nov 2017 17:59:13 -0500
>
>> Tuntap and similar devices can inject GSO packets. Accept type
>> VIRTIO_NET_HDR_GSO_UDP, even though not generating UFO natively.
>>
>> Processes are expected to use feature negotiation such as TUNSETOFFLOAD
>> to detect supported offload types and refrain from injecting other
>> packets. This process breaks down with live migration: guest kernels
>> do not renegotiate flags, so destination hosts need to expose all
>> features that the source host does.
>>
>> Partially revert the UFO removal from 182e0b6b5846~1..d9d30adf5677.
>> This patch introduces nearly(*) no new code to simplify verification.
>> It brings back verbatim tuntap UFO negotiation, VIRTIO_NET_HDR_GSO_UDP
>> insertion and software UFO segmentation.
>
> This looks good, one minor nit:
>
>> @@ -2369,6 +2369,10 @@ static int set_offload(struct tun_struct *tun,
>> unsigned long arg)
>> features |= NETIF_F_TSO6;
>> arg &= ~(TUN_F_TSO4|TUN_F_TSO6);
>> }
>> +
>> + if (arg & TUN_F_UFO) {
>> + arg &= ~TUN_F_UFO;
>> + }
>
> This can be just simply "arg &= ~TUN_F_UFO;"? If anything the curly braces
> should be removed for a single statement basic block.
Okay, I'll fix that up in v2. At least for the initial version I wanted
to make as few changes as possible to the original code.
Re: [PATCH net] net: accept UFO datagrams from tuntap and packet
From: Willem de Bruijn
Date: Fri, 17 Nov 2017 17:59:13 -0500
> Tuntap and similar devices can inject GSO packets. Accept type
> VIRTIO_NET_HDR_GSO_UDP, even though not generating UFO natively.
>
> Processes are expected to use feature negotiation such as TUNSETOFFLOAD
> to detect supported offload types and refrain from injecting other
> packets. This process breaks down with live migration: guest kernels
> do not renegotiate flags, so destination hosts need to expose all
> features that the source host does.
>
> Partially revert the UFO removal from 182e0b6b5846~1..d9d30adf5677.
> This patch introduces nearly(*) no new code to simplify verification.
> It brings back verbatim tuntap UFO negotiation, VIRTIO_NET_HDR_GSO_UDP
> insertion and software UFO segmentation.
This looks good, one minor nit:
> @@ -2369,6 +2369,10 @@ static int set_offload(struct tun_struct *tun,
> unsigned long arg)
> features |= NETIF_F_TSO6;
> arg &= ~(TUN_F_TSO4|TUN_F_TSO6);
> }
> +
> + if (arg & TUN_F_UFO) {
> + arg &= ~TUN_F_UFO;
> + }
This can be just simply "arg &= ~TUN_F_UFO;"? If anything the curly braces
should be removed for a single statement basic block.
Thanks for working so hard on fixing this.
[PATCH net] net: accept UFO datagrams from tuntap and packet
From: Willem de Bruijn
Tuntap and similar devices can inject GSO packets. Accept type
VIRTIO_NET_HDR_GSO_UDP, even though not generating UFO natively.
Processes are expected to use feature negotiation such as TUNSETOFFLOAD
to detect supported offload types and refrain from injecting other
packets. This process breaks down with live migration: guest kernels
do not renegotiate flags, so destination hosts need to expose all
features that the source host does.
Partially revert the UFO removal from 182e0b6b5846~1..d9d30adf5677.
This patch introduces nearly(*) no new code to simplify verification.
It brings back verbatim tuntap UFO negotiation, VIRTIO_NET_HDR_GSO_UDP
insertion and software UFO segmentation.
It does not reinstate protocol stack support, hardware offload
(NETIF_F_UFO), SKB_GSO_UDP tunneling in SKB_GSO_SOFTWARE or reception
of VIRTIO_NET_HDR_GSO_UDP packets in tuntap.
To support SKB_GSO_UDP reappearing in the stack, also reinstate
logic in act_csum and openvswitch. Achieve equivalence with v4.13 HEAD
by squashing in commit 939912216fa8 ("net: skb_needs_check() removes
CHECKSUM_UNNECESSARY check for tx.") and reverting commit 8d63bee643f1
("net: avoid skb_warn_bad_offload false positives on UFO").
(*) To avoid having to bring back skb_shinfo(skb)->ip6_frag_id,
ipv6_proxy_select_ident is changed to return a __be32, which is
assigned directly to the frag_hdr. Also, SKB_GSO_UDP is inserted
at the end of the enum to minimize code churn.
Link:
http://lkml.kernel.org/r/
Fixes: fb652fdfe837 ("macvlan/macvtap: Remove NETIF_F_UFO advertisement.")
Reported-by: Michal Kubecek
Signed-off-by: Willem de Bruijn
---
drivers/net/tap.c | 2 +-
drivers/net/tun.c | 4 ++
include/linux/netdev_features.h | 4 +-
include/linux/netdevice.h | 1 +
include/linux/skbuff.h | 2 +
include/linux/virtio_net.h | 5 ++-
include/net/ipv6.h | 1 +
net/core/dev.c | 3 +-
net/ipv4/af_inet.c | 12 +-
net/ipv4/udp_offload.c | 49 ++--
net/ipv6/output_core.c | 31 +++
net/ipv6/udp_offload.c | 85 +++--
net/openvswitch/datapath.c | 14 +++
net/openvswitch/flow.c | 6 ++-
net/sched/act_csum.c| 6 +++
15 files changed, 211 insertions(+), 14 deletions(-)
diff --git a/drivers/net/tap.c b/drivers/net/tap.c
index b13890953ebb..e9489b88407c 100644
--- a/drivers/net/tap.c
+++ b/drivers/net/tap.c
@@ -1077,7 +1077,7 @@ static long tap_ioctl(struct file *file, unsigned int cmd,
case TUNSETOFFLOAD:
/* let the user check for future flags */
if (arg & ~(TUN_F_CSUM | TUN_F_TSO4 | TUN_F_TSO6 |
- TUN_F_TSO_ECN))
+ TUN_F_TSO_ECN | TUN_F_UFO))
return -EINVAL;
rtnl_lock();
diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index 6bb1e604aadd..a33385d8ac65 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -2369,6 +2369,10 @@ static int set_offload(struct tun_struct *tun, unsigned
long arg)
features |= NETIF_F_TSO6;
arg &= ~(TUN_F_TSO4|TUN_F_TSO6);
}
+
+ if (arg & TUN_F_UFO) {
+ arg &= ~TUN_F_UFO;
+ }
}
/* This gives the user a way to test for new features in future by
diff --git a/include/linux/netdev_features.h b/include/linux/netdev_features.h
index dc8b4896b77b..b1b0ca7ccb2b 100644
--- a/include/linux/netdev_features.h
+++ b/include/linux/netdev_features.h
@@ -54,8 +54,9 @@ enum {
NETIF_F_GSO_TUNNEL_REMCSUM_BIT, /* ... TUNNEL with TSO & REMCSUM */
NETIF_F_GSO_SCTP_BIT, /* ... SCTP fragmentation */
NETIF_F_GSO_ESP_BIT,/* ... ESP with TSO */
+ NETIF_F_GSO_UDP_BIT,/* ... UFO, deprecated except tuntap */
/**/NETIF_F_GSO_LAST = /* last bit, see GSO_MASK */
- NETIF_F_GSO_ESP_BIT,
+ NETIF_F_GSO_UDP_BIT,
NETIF_F_FCOE_CRC_BIT, /* FCoE CRC32 */
NETIF_F_SCTP_CRC_BIT, /* SCTP checksum offload */
@@ -132,6 +133,7 @@ enum {
#define NETIF_F_GSO_TUNNEL_REMCSUM __NETIF_F(GSO_TUNNEL_REMCSUM)
#define NETIF_F_GSO_SCTP __NETIF_F(GSO_SCTP)
#define NETIF_F_GSO_ESP__NETIF_F(GSO_ESP)
+#define NETIF_F_GSO_UDP__NETIF_F(GSO_UDP)
#define NETIF_F_HW_VLAN_STAG_FILTER __NETIF_F(HW_VLAN_STAG_FILTER)
#define NETIF_F_HW_VLAN_STAG_RX__NETIF_F(HW_VLAN_STAG_RX)
#define NETIF_F_HW_VLAN_STAG_TX__NETIF_F(HW_VLAN_STAG_TX)
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 6b274bfe489f..ef789e1d679e 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -4140,6 +4140,7 @@ static inline bool net_gso_ok(netdev_features_t features,
int gso_ty
