regression: UFO removal breaks kvm live migration

regression: UFO removal breaks kvm live migration

[Michal Kubecek]

Hello,

I just received this bug report:

  https://bugzilla.suse.com/show_bug.cgi?id=1066757

The reporter runs a live migration of a kvm guest from a host with
kernel supporting UFO (openSUSE 42.2 or 42.3, based on 4.4) to a host
with kernel with UFO support removed (SLE15 or openSUSE 15.0 pre-release
which is based on 4.12 but has the UFO removal series backported).

The migration fails with

  kvm: virtio-net: saved image requires TUN_F_UFO support

because the guest image has a virtio_net device with UFO enabled which
requires TUN_F_UFO on the corresponding host tun device but that is no
longer available on the target host.

This kind of problem already happened once:

  https://www.spinics.net/lists/netdev/msg443821.html

At that time, commit 3d0ad09412ff ("drivers/net: Disable UFO through
virtio") was reverted once the issue it worked around was resolved in
a different way.

I didn't have time to think it through yet but perhaps we could allow
setting TUN_F_UFO and ignore its value.

This is not time critical for SLE15 / openSUSE 15.0 which are still at
early beta stage but 4.14 final is close and once it's out, more users
are going to hit this.

Michal Kubecek

Re: regression: UFO removal breaks kvm live migration

[Willem de Bruijn]

On Tue, Nov 7, 2017 at 5:02 PM, Michal Kubecek <mkubecek@suse.cz> wrote:
> Hello,
>
> I just received this bug report:
>
>   https://bugzilla.suse.com/show_bug.cgi?id=1066757
>
> The reporter runs a live migration of a kvm guest from a host with
> kernel supporting UFO (openSUSE 42.2 or 42.3, based on 4.4) to a host
> with kernel with UFO support removed (SLE15 or openSUSE 15.0 pre-release
> which is based on 4.12 but has the UFO removal series backported).
>
> The migration fails with
>
>   kvm: virtio-net: saved image requires TUN_F_UFO support
>
> because the guest image has a virtio_net device with UFO enabled which
> requires TUN_F_UFO on the corresponding host tun device but that is no
> longer available on the target host.
>
> This kind of problem already happened once:
>
>   https://www.spinics.net/lists/netdev/msg443821.html
>
> At that time, commit 3d0ad09412ff ("drivers/net: Disable UFO through
> virtio") was reverted once the issue it worked around was resolved in
> a different way.
>
> I didn't have time to think it through yet but perhaps we could allow
> setting TUN_F_UFO and ignore its value.

If the feature is enabled guests may try to send UFO packets, which
the host is no longer able to fragment.

virtio_net_hdr_to_skb will drop the packets immediately based on
gso_type and tun_get_user will return EINVAL.

Still, perhaps that's preferable as migration will succeed and most
guests won't ever try to send those packets in the first place.

> This is not time critical for SLE15 / openSUSE 15.0 which are still at
> early beta stage but 4.14 final is close and once it's out, more users
> are going to hit this.
>
> Michal Kubecek
>

Re: regression: UFO removal breaks kvm live migration

[David Miller]

From: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
Date: Wed, 8 Nov 2017 12:36:26 +0900

> On Tue, Nov 7, 2017 at 5:02 PM, Michal Kubecek <mkubecek@suse.cz> wrote:
>> I didn't have time to think it through yet but perhaps we could allow
>> setting TUN_F_UFO and ignore its value.
> 
> If the feature is enabled guests may try to send UFO packets, which
> the host is no longer able to fragment.
> 
> virtio_net_hdr_to_skb will drop the packets immediately based on
> gso_type and tun_get_user will return EINVAL.
> 
> Still, perhaps that's preferable as migration will succeed and most
> guests won't ever try to send those packets in the first place.

However, this would create the situation where there is no way
to properly probe for the actual presence of UFO support.

Re: regression: UFO removal breaks kvm live migration

[Jason Wang]

On 2017年11月08日 15:26, David Miller wrote:
> From: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
> Date: Wed, 8 Nov 2017 12:36:26 +0900
>
>> On Tue, Nov 7, 2017 at 5:02 PM, Michal Kubecek <mkubecek@suse.cz> wrote:
>>> I didn't have time to think it through yet but perhaps we could allow
>>> setting TUN_F_UFO and ignore its value.
>> If the feature is enabled guests may try to send UFO packets, which
>> the host is no longer able to fragment.
>>
>> virtio_net_hdr_to_skb will drop the packets immediately based on
>> gso_type and tun_get_user will return EINVAL.
>>
>> Still, perhaps that's preferable as migration will succeed and most
>> guests won't ever try to send those packets in the first place.
> However, this would create the situation where there is no way
> to properly probe for the actual presence of UFO support.

I think we should not have any assumption on how guest will use the 
feature. So I could not come a better than bring it back partially for 
TAP, looks like we only need segment them in tun_get_user().

Thanks

Re: regression: UFO removal breaks kvm live migration

[Willem de Bruijn]

On Wed, Nov 8, 2017 at 4:49 PM, Jason Wang <jasowang@redhat.com> wrote:
>
>
> On 2017年11月08日 15:26, David Miller wrote:
>>
>> From: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
>> Date: Wed, 8 Nov 2017 12:36:26 +0900
>>
>>> On Tue, Nov 7, 2017 at 5:02 PM, Michal Kubecek <mkubecek@suse.cz> wrote:
>>>>
>>>> I didn't have time to think it through yet but perhaps we could allow
>>>> setting TUN_F_UFO and ignore its value.
>>>
>>> If the feature is enabled guests may try to send UFO packets, which
>>> the host is no longer able to fragment.
>>>
>>> virtio_net_hdr_to_skb will drop the packets immediately based on
>>> gso_type and tun_get_user will return EINVAL.
>>>
>>> Still, perhaps that's preferable as migration will succeed and most
>>> guests won't ever try to send those packets in the first place.
>>
>> However, this would create the situation where there is no way
>> to properly probe for the actual presence of UFO support.
>
>
> I think we should not have any assumption on how guest will use the feature.
> So I could not come a better than bring it back partially for TAP, looks
> like we only need segment them in tun_get_user().

Live migration essentially expects that features can never be removed [1],
as feature bits are not renegotiated after migration. In the short term we'll
have to work around that, but in the long term that does not seem practical.

There already exist interfaces to renegotiate guest/host state at runtime,
including for offloads [2][3]. For newer guests, we should support a trigger
from the host to renegotiate offloads.

That won't help in the short term. I'm still reading up to see if there are
any other options besides reimplement or advertise-but-drop, such as
an implicit trigger that would make the guest renegotiate. It's unlikely, but
worth a look..

[1] https://lists.linuxfoundation.org/pipermail/virtualization/2014-November/028126.html
[2] https://lists.linuxfoundation.org/pipermail/virtualization/2013-April/023818.html
[3] https://patchwork.kernel.org/patch/9850785/

Re: regression: UFO removal breaks kvm live migration

[Jason Wang]

On 2017年11月08日 17:08, Willem de Bruijn wrote:
> On Wed, Nov 8, 2017 at 4:49 PM, Jason Wang <jasowang@redhat.com> wrote:
>>
>> On 2017年11月08日 15:26, David Miller wrote:
>>> From: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
>>> Date: Wed, 8 Nov 2017 12:36:26 +0900
>>>
>>>> On Tue, Nov 7, 2017 at 5:02 PM, Michal Kubecek <mkubecek@suse.cz> wrote:
>>>>> I didn't have time to think it through yet but perhaps we could allow
>>>>> setting TUN_F_UFO and ignore its value.
>>>> If the feature is enabled guests may try to send UFO packets, which
>>>> the host is no longer able to fragment.
>>>>
>>>> virtio_net_hdr_to_skb will drop the packets immediately based on
>>>> gso_type and tun_get_user will return EINVAL.
>>>>
>>>> Still, perhaps that's preferable as migration will succeed and most
>>>> guests won't ever try to send those packets in the first place.
>>> However, this would create the situation where there is no way
>>> to properly probe for the actual presence of UFO support.
>>
>> I think we should not have any assumption on how guest will use the feature.
>> So I could not come a better than bring it back partially for TAP, looks
>> like we only need segment them in tun_get_user().
> Live migration essentially expects that features can never be removed [1],
> as feature bits are not renegotiated after migration. In the short term we'll
> have to work around that, but in the long term that does not seem practical.
>
> There already exist interfaces to renegotiate guest/host state at runtime,
> including for offloads [2][3]. For newer guests, we should support a trigger
> from the host to renegotiate offloads.

I could not think of a real use case for this other than trying to 
workaround a bug.

>
> That won't help in the short term. I'm still reading up to see if there are
> any other options besides reimplement or advertise-but-drop, such as
> an implicit trigger that would make the guest renegotiate. It's unlikely, but
> worth a look..

Yes, this looks hard. And even if we can manage to do this, it looks an 
overkill since it will impact all guest after migration.

Thanks

>
> [1] https://lists.linuxfoundation.org/pipermail/virtualization/2014-November/028126.html
> [2] https://lists.linuxfoundation.org/pipermail/virtualization/2013-April/023818.html
> [3] https://patchwork.kernel.org/patch/9850785/

Re: regression: UFO removal breaks kvm live migration

[David Miller]

From: Jason Wang <jasowang@redhat.com>
Date: Wed, 8 Nov 2017 17:25:48 +0900

> On 2017年11月08日 17:08, Willem de Bruijn wrote:
>> That won't help in the short term. I'm still reading up to see if
>> there are
>> any other options besides reimplement or advertise-but-drop, such as
>> an implicit trigger that would make the guest renegotiate. It's
>> unlikely, but
>> worth a look..
> 
> Yes, this looks hard. And even if we can manage to do this, it looks
> an overkill since it will impact all guest after migration.

Like Willem I would much prefer "advertise-but-drop" if it works.

In the long term feature renegotiation triggers are a must.

There is no way for us to remove features otherwise.  In my opinion
this will even make migrations more powerful.

Re: regression: UFO removal breaks kvm live migration

[Jason Wang]

On 2017年11月08日 20:32, David Miller wrote:
> From: Jason Wang <jasowang@redhat.com>
> Date: Wed, 8 Nov 2017 17:25:48 +0900
>
>> On 2017年11月08日 17:08, Willem de Bruijn wrote:
>>> That won't help in the short term. I'm still reading up to see if
>>> there are
>>> any other options besides reimplement or advertise-but-drop, such as
>>> an implicit trigger that would make the guest renegotiate. It's
>>> unlikely, but
>>> worth a look..
>> Yes, this looks hard. And even if we can manage to do this, it looks
>> an overkill since it will impact all guest after migration.
> Like Willem I would much prefer "advertise-but-drop" if it works.

This makes migration work but all guest UFO traffic will stall.

>
> In the long term feature renegotiation triggers are a must.
>
> There is no way for us to remove features otherwise.

We can remove if we don't break userspace(guest).

> In my opinion
> this will even make migrations more powerful.

But this does not help for guest running old version of kernel which  
still think UFO work.

Thanks

Re: regression: UFO removal breaks kvm live migration

[David Miller]

From: Jason Wang <jasowang@redhat.com>
Date: Wed, 8 Nov 2017 21:53:27 +0900

> On 2017年11月08日 20:32, David Miller wrote:
>> From: Jason Wang <jasowang@redhat.com>
>> Date: Wed, 8 Nov 2017 17:25:48 +0900
>>
>>> On 2017年11月08日 17:08, Willem de Bruijn wrote:
>>>> That won't help in the short term. I'm still reading up to see if
>>>> there are
>>>> any other options besides reimplement or advertise-but-drop, such as
>>>> an implicit trigger that would make the guest renegotiate. It's
>>>> unlikely, but
>>>> worth a look..
>>> Yes, this looks hard. And even if we can manage to do this, it looks
>>> an overkill since it will impact all guest after migration.
>> Like Willem I would much prefer "advertise-but-drop" if it works.
> 
> This makes migration work but all guest UFO traffic will stall.

The idea is that the sender will resend and it will be smaller and
thus non-UFO.

Re: regression: UFO removal breaks kvm live migration

[Michael S. Tsirkin]

On Wed, Nov 08, 2017 at 08:32:31PM +0900, David Miller wrote:
> From: Jason Wang <jasowang@redhat.com>
> Date: Wed, 8 Nov 2017 17:25:48 +0900
> 
> > On 2017年11月08日 17:08, Willem de Bruijn wrote:
> >> That won't help in the short term. I'm still reading up to see if
> >> there are
> >> any other options besides reimplement or advertise-but-drop, such as
> >> an implicit trigger that would make the guest renegotiate. It's
> >> unlikely, but
> >> worth a look..
> > 
> > Yes, this looks hard. And even if we can manage to do this, it looks
> > an overkill since it will impact all guest after migration.
> 
> Like Willem I would much prefer "advertise-but-drop" if it works.
> 
> In the long term feature renegotiation triggers are a must.
> 
> There is no way for us to remove features otherwise.

Isn't this like most userspace ABI issues? Once you add them it's very hard
to remove them, even with negotiation - too much userspace just does
if (ret)
	exit();

> In my opinion this will even make migrations more powerful.

I agree. Not sure how to avoid packet drops when doing this though.
And dropping a ton of TX packets isn't nice at all since
TX packets is what teaches the network about the new location
of the VM.

-- 
MST

Re: regression: UFO removal breaks kvm live migration

[Willem de Bruijn]

On Wed, Nov 8, 2017 at 9:53 PM, Jason Wang <jasowang@redhat.com> wrote:
>
>
> On 2017年11月08日 20:32, David Miller wrote:
>>
>> From: Jason Wang <jasowang@redhat.com>
>> Date: Wed, 8 Nov 2017 17:25:48 +0900
>>
>>> On 2017年11月08日 17:08, Willem de Bruijn wrote:
>>>>
>>>> That won't help in the short term. I'm still reading up to see if
>>>> there are
>>>> any other options besides reimplement or advertise-but-drop, such as
>>>> an implicit trigger that would make the guest renegotiate. It's
>>>> unlikely, but
>>>> worth a look..
>>>
>>> Yes, this looks hard. And even if we can manage to do this, it looks
>>> an overkill since it will impact all guest after migration.
>>
>> Like Willem I would much prefer "advertise-but-drop" if it works.
>
>
> This makes migration work but all guest UFO traffic will stall.
>
>>
>> In the long term feature renegotiation triggers are a must.
>>
>> There is no way for us to remove features otherwise.
>
>
> We can remove if we don't break userspace(guest).
>
>> In my opinion
>> this will even make migrations more powerful.
>
>
> But this does not help for guest running old version of kernel which still
> think UFO work.

Indeed, if we have to support live migration of arbitrary old guests
without any expectations on hypervisor version either, features can
simply never be reverted, even if a negotiation interface exists.

At least for upcoming features and devices, guest code should not
have this expectation, but from the start allow renegation such as
CTRL_GUEST_OFFLOADS [1] based on a host trigger. But for
tuntap TUNSETOFFLOAD it seems that ship has sailed.

Okay, I will send a patch to reinstate UFO for this use case (only). There
is some related work in tap_handle_frame and packet_direct_xmit to
segment directly in the device. I will be traveling the next few days, so
it won't be in time for 4.14 (but can go in stable later, of course).

[1] https://patchwork.kernel.org/patch/9850785/

Re: regression: UFO removal breaks kvm live migration

[David Miller]

From: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
Date: Fri, 10 Nov 2017 14:32:29 +0900

> Okay, I will send a patch to reinstate UFO for this use case (only).

Thank you.

Re: regression: UFO removal breaks kvm live migration

[Willem de Bruijn]

On Fri, Nov 10, 2017 at 12:32 AM, Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
> On Wed, Nov 8, 2017 at 9:53 PM, Jason Wang <jasowang@redhat.com> wrote:
>>
>>
>> On 2017年11月08日 20:32, David Miller wrote:
>>>
>>> From: Jason Wang <jasowang@redhat.com>
>>> Date: Wed, 8 Nov 2017 17:25:48 +0900
>>>
>>>> On 2017年11月08日 17:08, Willem de Bruijn wrote:
>>>>>
>>>>> That won't help in the short term. I'm still reading up to see if
>>>>> there are
>>>>> any other options besides reimplement or advertise-but-drop, such as
>>>>> an implicit trigger that would make the guest renegotiate. It's
>>>>> unlikely, but
>>>>> worth a look..
>>>>
>>>> Yes, this looks hard. And even if we can manage to do this, it looks
>>>> an overkill since it will impact all guest after migration.
>>>
>>> Like Willem I would much prefer "advertise-but-drop" if it works.
>>
>>
>> This makes migration work but all guest UFO traffic will stall.
>>
>>>
>>> In the long term feature renegotiation triggers are a must.
>>>
>>> There is no way for us to remove features otherwise.
>>
>>
>> We can remove if we don't break userspace(guest).
>>
>>> In my opinion
>>> this will even make migrations more powerful.
>>
>>
>> But this does not help for guest running old version of kernel which still
>> think UFO work.
>
> Indeed, if we have to support live migration of arbitrary old guests
> without any expectations on hypervisor version either, features can
> simply never be reverted, even if a negotiation interface exists.
>
> At least for upcoming features and devices, guest code should not
> have this expectation, but from the start allow renegation such as
> CTRL_GUEST_OFFLOADS [1] based on a host trigger. But for
> tuntap TUNSETOFFLOAD it seems that ship has sailed.
>
> Okay, I will send a patch to reinstate UFO for this use case (only). There
> is some related work in tap_handle_frame and packet_direct_xmit to
> segment directly in the device. I will be traveling the next few days, so
> it won't be in time for 4.14 (but can go in stable later, of course).

I'm finishing up and running some tests. The majority of the patch is a
straightforward partial revert of the patchset, so while fairly large for a
patch to net (~150 lines, esp. in udp[46]_ufo_fragment), that is all
thoroughly tested code. Notably absent are the protocol layer and
hardware support (NETIF_F_UFO) portions.

The only open issue is whether to rely on existing skb_gso_segment
processing in the transmit path from validate_xmit_skb or to add new
skb_gso_segment calls directly to tun_get_user, tap_get_user and
pf_packet. Tun has to loop around four different ways of injecting
packets into the device. Something like the below snippet.

More conservative is to introduce no completely new code and rely on
validate_xmit_skb, but that means having to protect the entire stack
against skbs with SKB_GSO_UDP, so also bringing back some
checksum and fragment handling snippets in gre_gso_segment,
__skb_udp_tunnel_segment, act_csum and openvswitch.

A third option is to send the conservative approach to net, then
in net-next follow up with a patch to plug the SKB_GSO_UDP
directly in the devices and revert the tunnel/act/openvswitch stanzas
I'm leaning towards that approach.

@@ -1380,7 +1380,7 @@ static ssize_t tun_get_user(struct tun_struct
*tun, struct tun_file *tfile,
                            int noblock, bool more)
 {
        struct tun_pi pi = { 0, cpu_to_be16(ETH_P_IP) };
-       struct sk_buff *skb;
+       struct sk_buff *skb, *segs = NULL;
        size_t total_len = iov_iter_count(from);
        size_t len = total_len, align = tun->align, linear;
        struct virtio_net_hdr gso = { 0 };
@@ -1552,12 +1552,33 @@ static ssize_t tun_get_user(struct tun_struct
*tun, struct tun_file *tfile,
        }

        rxhash = __skb_get_hash_symmetric(skb);
+
+       if (skb_shinfo(skb)->gso_type & SKB_GSO_UDP) {
+               skb_push(skb, ETH_HLEN);
+               segs = __skb_gso_segment(skb, netif_skb_features(skb), false);
+
+               if (IS_ERR(segs)) {
+                       kfree_skb(skb);
+                       return PTR_ERR(segs);
+               }
+
+               if (segs) {
+                       consume_skb(skb);
+                       skb = segs;
+               }
+again:
+               skb_pull(skb, ETH_HLEN);
+               segs = skb->next;
+               skb->next = NULL;
+       }
+
 #ifndef CONFIG_4KSTACKS
-        tun_rx_batched(tun, tfile, skb, more);
+       tun_rx_batched(tun, tfile, skb, more || segs);
 #else
        netif_rx_ni(skb);
 #endif

+       if (segs) {
+               skb = segs;
+               goto again;
+       }

Re: regression: UFO removal breaks kvm live migration

[Willem de Bruijn]

>> Okay, I will send a patch to reinstate UFO for this use case (only). There
>> is some related work in tap_handle_frame and packet_direct_xmit to
>> segment directly in the device. I will be traveling the next few days, so
>> it won't be in time for 4.14 (but can go in stable later, of course).
>
> I'm finishing up and running some tests. The majority of the patch is a
> straightforward partial revert of the patchset, so while fairly large for a
> patch to net (~150 lines, esp. in udp[46]_ufo_fragment), that is all
> thoroughly tested code. Notably absent are the protocol layer and
> hardware support (NETIF_F_UFO) portions.
>
> The only open issue is whether to rely on existing skb_gso_segment
> processing in the transmit path from validate_xmit_skb or to add new
> skb_gso_segment calls directly to tun_get_user, tap_get_user and
> pf_packet. Tun has to loop around four different ways of injecting
> packets into the device. Something like the below snippet.
>
> More conservative is to introduce no completely new code and rely on
> validate_xmit_skb, but that means having to protect the entire stack
> against skbs with SKB_GSO_UDP, so also bringing back some
> checksum and fragment handling snippets in gre_gso_segment,
> __skb_udp_tunnel_segment, act_csum and openvswitch.

Come to think of it, as this patch does not bring back NETIF_F_UFO
support to NETIF_F_GSO_SOFTWARE, the tunnel cases can be
excluded.

Then this is probably the simpler and more obviously correct approach.

Re: regression: UFO removal breaks kvm live migration

[Willem de Bruijn]

On Fri, Nov 17, 2017 at 9:48 AM, Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
>>> Okay, I will send a patch to reinstate UFO for this use case (only). There
>>> is some related work in tap_handle_frame and packet_direct_xmit to
>>> segment directly in the device. I will be traveling the next few days, so
>>> it won't be in time for 4.14 (but can go in stable later, of course).
>>
>> I'm finishing up and running some tests. The majority of the patch is a
>> straightforward partial revert of the patchset, so while fairly large for a
>> patch to net (~150 lines, esp. in udp[46]_ufo_fragment), that is all
>> thoroughly tested code. Notably absent are the protocol layer and
>> hardware support (NETIF_F_UFO) portions.
>>
>> The only open issue is whether to rely on existing skb_gso_segment
>> processing in the transmit path from validate_xmit_skb or to add new
>> skb_gso_segment calls directly to tun_get_user, tap_get_user and
>> pf_packet. Tun has to loop around four different ways of injecting
>> packets into the device. Something like the below snippet.
>>
>> More conservative is to introduce no completely new code and rely on
>> validate_xmit_skb, but that means having to protect the entire stack
>> against skbs with SKB_GSO_UDP, so also bringing back some
>> checksum and fragment handling snippets in gre_gso_segment,
>> __skb_udp_tunnel_segment, act_csum and openvswitch.
>
> Come to think of it, as this patch does not bring back NETIF_F_UFO
> support to NETIF_F_GSO_SOFTWARE, the tunnel cases can be
> excluded.
>
> Then this is probably the simpler and more obviously correct approach.

Sent: http://patchwork.ozlabs.org/patch/839168/