| Message ID | 20250211071922.2311873-3-junnan01.wu@samsung.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | vsock suspend/resume fix | expand |
On Tue, Feb 11, 2025 at 03:19:22PM +0800, Junnan Wu wrote: >Function virtio_vsock_vqs_del will be invoked in 2 cases >virtio_vsock_remove() and virtio_vsock_freeze(). > >And when driver freeze, the connected socket will be set to TCP_CLOSE >and it will cause that socket can not be unusable after resume. > >Refactor function virtio_vsock_vqs_del to differentiate the 2 use cases. > >Co-developed-by: Ying Gao <ying01.gao@samsung.com> >Signed-off-by: Ying Gao <ying01.gao@samsung.com> >Signed-off-by: Junnan Wu <junnan01.wu@samsung.com> >--- > net/vmw_vsock/virtio_transport.c | 18 ++++++++++++------ > 1 file changed, 12 insertions(+), 6 deletions(-) We are still discussing this in v1: https://lore.kernel.org/virtualization/iv6oalr6yuwsfkoxnorp4t77fdjheteyojauwf2phshucdxatf@ominy3hfcpxb/T/#u Please stop sending new versions before reaching an agreement! BTW I still think this is wrong, so: Nacked-by: Stefano Garzarella <sgarzare@redhat.com> > >diff --git a/net/vmw_vsock/virtio_transport.c b/net/vmw_vsock/virtio_transport.c >index f0e48e6911fc..909048c9069b 100644 >--- a/net/vmw_vsock/virtio_transport.c >+++ b/net/vmw_vsock/virtio_transport.c >@@ -716,14 +716,20 @@ static void virtio_vsock_vqs_start(struct virtio_vsock *vsock) > queue_work(virtio_vsock_workqueue, &vsock->send_pkt_work); > } > >-static void virtio_vsock_vqs_del(struct virtio_vsock *vsock) >+static void virtio_vsock_vqs_del(struct virtio_vsock *vsock, bool vsock_reset) > { > struct virtio_device *vdev = vsock->vdev; > struct sk_buff *skb; > >- /* Reset all connected sockets when the VQs disappear */ >- vsock_for_each_connected_socket(&virtio_transport.transport, >- virtio_vsock_reset_sock); >+ /* When driver is removed, reset all connected >+ * sockets because the VQs disappear. You wrote it here too, you have to reset them because the VQs are going to disappear, isn't it the same after the freeze? >+ * When driver is just freezed, don't do that because the connected >+ * socket still need to use after restore. >+ */ >+ if (vsock_reset) { >+ vsock_for_each_connected_socket(&virtio_transport.transport, >+ virtio_vsock_reset_sock); >+ } > > /* Stop all work handlers to make sure no one is accessing the device, > * so we can safely call virtio_reset_device(). >@@ -831,7 +837,7 @@ static void virtio_vsock_remove(struct virtio_device *vdev) > rcu_assign_pointer(the_virtio_vsock, NULL); > synchronize_rcu(); > >- virtio_vsock_vqs_del(vsock); >+ virtio_vsock_vqs_del(vsock, true); > > /* Other works can be queued before 'config->del_vqs()', so we flush > * all works before to free the vsock object to avoid use after free. >@@ -856,7 +862,7 @@ static int virtio_vsock_freeze(struct virtio_device *vdev) > rcu_assign_pointer(the_virtio_vsock, NULL); > synchronize_rcu(); > >- virtio_vsock_vqs_del(vsock); >+ virtio_vsock_vqs_del(vsock, false); > > mutex_unlock(&the_virtio_vsock_mutex); > >-- >2.34.1 >
…
> and it will cause that socket can not be unusable after resume.
…
I find such a wording confusing.
Can the change description become clearer?
Regards,
Markus
diff --git a/net/vmw_vsock/virtio_transport.c b/net/vmw_vsock/virtio_transport.c index f0e48e6911fc..909048c9069b 100644 --- a/net/vmw_vsock/virtio_transport.c +++ b/net/vmw_vsock/virtio_transport.c @@ -716,14 +716,20 @@ static void virtio_vsock_vqs_start(struct virtio_vsock *vsock) queue_work(virtio_vsock_workqueue, &vsock->send_pkt_work); } -static void virtio_vsock_vqs_del(struct virtio_vsock *vsock) +static void virtio_vsock_vqs_del(struct virtio_vsock *vsock, bool vsock_reset) { struct virtio_device *vdev = vsock->vdev; struct sk_buff *skb; - /* Reset all connected sockets when the VQs disappear */ - vsock_for_each_connected_socket(&virtio_transport.transport, - virtio_vsock_reset_sock); + /* When driver is removed, reset all connected + * sockets because the VQs disappear. + * When driver is just freezed, don't do that because the connected + * socket still need to use after restore. + */ + if (vsock_reset) { + vsock_for_each_connected_socket(&virtio_transport.transport, + virtio_vsock_reset_sock); + } /* Stop all work handlers to make sure no one is accessing the device, * so we can safely call virtio_reset_device(). @@ -831,7 +837,7 @@ static void virtio_vsock_remove(struct virtio_device *vdev) rcu_assign_pointer(the_virtio_vsock, NULL); synchronize_rcu(); - virtio_vsock_vqs_del(vsock); + virtio_vsock_vqs_del(vsock, true); /* Other works can be queued before 'config->del_vqs()', so we flush * all works before to free the vsock object to avoid use after free. @@ -856,7 +862,7 @@ static int virtio_vsock_freeze(struct virtio_device *vdev) rcu_assign_pointer(the_virtio_vsock, NULL); synchronize_rcu(); - virtio_vsock_vqs_del(vsock); + virtio_vsock_vqs_del(vsock, false); mutex_unlock(&the_virtio_vsock_mutex);