* Re: [PATCH] vsock/virtio: use GFP_ATOMIC under RCU read lock
2024-10-02 13:41 [PATCH] vsock/virtio: use GFP_ATOMIC under RCU read lock Michael S. Tsirkin
@ 2024-10-02 14:02 ` Stefano Garzarella
2024-10-02 16:42 ` Stefano Garzarella
2024-10-03 1:33 ` Luigi Leonardi
` (3 subsequent siblings)
4 siblings, 1 reply; 8+ messages in thread
From: Stefano Garzarella @ 2024-10-02 14:02 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: linux-kernel, Christian Brauner, Luigi Leonardi, Jason Wang,
Xuan Zhuo, Eugenio Pérez, Stefan Hajnoczi, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, Marco Pinna,
virtualization, kvm, netdev
On Wed, Oct 02, 2024 at 09:41:42AM GMT, Michael S. Tsirkin wrote:
>virtio_transport_send_pkt in now called on transport fast path,
>under RCU read lock. In that case, we have a bug: virtio_add_sgs
>is called with GFP_KERNEL, and might sleep.
>
>Pass the gfp flags as an argument, and use GFP_ATOMIC on
>the fast path.
>
>Link: https://lore.kernel.org/all/hfcr2aget2zojmqpr4uhlzvnep4vgskblx5b6xf2ddosbsrke7@nt34bxgp7j2x
>Fixes: efcd71af38be ("vsock/virtio: avoid queuing packets when intermediate queue is empty")
>Reported-by: Christian Brauner <brauner@kernel.org>
>Cc: Stefano Garzarella <sgarzare@redhat.com>
>Cc: Luigi Leonardi <luigi.leonardi@outlook.com>
>Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>---
>
>Lightly tested. Christian, could you pls confirm this fixes the problem
>for you? Stefano, it's a holiday here - could you pls help test!
Sure, thanks for the quick fix! I was thinking something similar ;-)
>Thanks!
>
>
> net/vmw_vsock/virtio_transport.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
>diff --git a/net/vmw_vsock/virtio_transport.c b/net/vmw_vsock/virtio_transport.c
>index f992f9a216f0..0cd965f24609 100644
>--- a/net/vmw_vsock/virtio_transport.c
>+++ b/net/vmw_vsock/virtio_transport.c
>@@ -96,7 +96,7 @@ static u32 virtio_transport_get_local_cid(void)
>
> /* Caller need to hold vsock->tx_lock on vq */
> static int virtio_transport_send_skb(struct sk_buff *skb, struct virtqueue *vq,
>- struct virtio_vsock *vsock)
>+ struct virtio_vsock *vsock, gfp_t gfp)
> {
> int ret, in_sg = 0, out_sg = 0;
> struct scatterlist **sgs;
>@@ -140,7 +140,7 @@ static int virtio_transport_send_skb(struct sk_buff *skb, struct virtqueue *vq,
> }
> }
>
>- ret = virtqueue_add_sgs(vq, sgs, out_sg, in_sg, skb, GFP_KERNEL);
>+ ret = virtqueue_add_sgs(vq, sgs, out_sg, in_sg, skb, gfp);
> /* Usually this means that there is no more space available in
> * the vq
> */
>@@ -178,7 +178,7 @@ virtio_transport_send_pkt_work(struct work_struct *work)
>
> reply = virtio_vsock_skb_reply(skb);
>
>- ret = virtio_transport_send_skb(skb, vq, vsock);
>+ ret = virtio_transport_send_skb(skb, vq, vsock, GFP_KERNEL);
> if (ret < 0) {
> virtio_vsock_skb_queue_head(&vsock->send_pkt_queue, skb);
> break;
>@@ -221,7 +221,7 @@ static int virtio_transport_send_skb_fast_path(struct virtio_vsock *vsock, struc
> if (unlikely(ret == 0))
> return -EBUSY;
>
>- ret = virtio_transport_send_skb(skb, vq, vsock);
nit: maybe we can add a comment here:
/* GFP_ATOMIC because we are in RCU section, so we can't sleep */
>+ ret = virtio_transport_send_skb(skb, vq, vsock, GFP_ATOMIC);
> if (ret == 0)
> virtqueue_kick(vq);
>
>--
>MST
>
I'll run some tests and come back with R-b when it's done.
Thanks,
Stefano
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] vsock/virtio: use GFP_ATOMIC under RCU read lock
2024-10-02 14:02 ` Stefano Garzarella
@ 2024-10-02 16:42 ` Stefano Garzarella
0 siblings, 0 replies; 8+ messages in thread
From: Stefano Garzarella @ 2024-10-02 16:42 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: linux-kernel, Christian Brauner, Luigi Leonardi, Jason Wang,
Xuan Zhuo, Eugenio Pérez, Stefan Hajnoczi, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, Marco Pinna,
virtualization, kvm, netdev
On Wed, Oct 02, 2024 at 04:02:06PM GMT, Stefano Garzarella wrote:
>On Wed, Oct 02, 2024 at 09:41:42AM GMT, Michael S. Tsirkin wrote:
>>virtio_transport_send_pkt in now called on transport fast path,
>>under RCU read lock. In that case, we have a bug: virtio_add_sgs
>>is called with GFP_KERNEL, and might sleep.
>>
>>Pass the gfp flags as an argument, and use GFP_ATOMIC on
>>the fast path.
>>
>>Link: https://lore.kernel.org/all/hfcr2aget2zojmqpr4uhlzvnep4vgskblx5b6xf2ddosbsrke7@nt34bxgp7j2x
>>Fixes: efcd71af38be ("vsock/virtio: avoid queuing packets when intermediate queue is empty")
>>Reported-by: Christian Brauner <brauner@kernel.org>
>>Cc: Stefano Garzarella <sgarzare@redhat.com>
>>Cc: Luigi Leonardi <luigi.leonardi@outlook.com>
>>Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>>---
>>
>>Lightly tested. Christian, could you pls confirm this fixes the problem
>>for you? Stefano, it's a holiday here - could you pls help test!
>
>Sure, thanks for the quick fix! I was thinking something similar ;-)
>
>>Thanks!
>>
>>
>>net/vmw_vsock/virtio_transport.c | 8 ++++----
>>1 file changed, 4 insertions(+), 4 deletions(-)
>>
>>diff --git a/net/vmw_vsock/virtio_transport.c b/net/vmw_vsock/virtio_transport.c
>>index f992f9a216f0..0cd965f24609 100644
>>--- a/net/vmw_vsock/virtio_transport.c
>>+++ b/net/vmw_vsock/virtio_transport.c
>>@@ -96,7 +96,7 @@ static u32 virtio_transport_get_local_cid(void)
>>
>>/* Caller need to hold vsock->tx_lock on vq */
>>static int virtio_transport_send_skb(struct sk_buff *skb, struct virtqueue *vq,
>>- struct virtio_vsock *vsock)
>>+ struct virtio_vsock *vsock, gfp_t gfp)
>>{
>> int ret, in_sg = 0, out_sg = 0;
>> struct scatterlist **sgs;
>>@@ -140,7 +140,7 @@ static int virtio_transport_send_skb(struct sk_buff *skb, struct virtqueue *vq,
>> }
>> }
>>
>>- ret = virtqueue_add_sgs(vq, sgs, out_sg, in_sg, skb, GFP_KERNEL);
>>+ ret = virtqueue_add_sgs(vq, sgs, out_sg, in_sg, skb, gfp);
>> /* Usually this means that there is no more space available in
>> * the vq
>> */
>>@@ -178,7 +178,7 @@ virtio_transport_send_pkt_work(struct work_struct *work)
>>
>> reply = virtio_vsock_skb_reply(skb);
>>
>>- ret = virtio_transport_send_skb(skb, vq, vsock);
>>+ ret = virtio_transport_send_skb(skb, vq, vsock, GFP_KERNEL);
>> if (ret < 0) {
>> virtio_vsock_skb_queue_head(&vsock->send_pkt_queue,
>> skb);
>> break;
>>@@ -221,7 +221,7 @@ static int virtio_transport_send_skb_fast_path(struct virtio_vsock *vsock, struc
>> if (unlikely(ret == 0))
>> return -EBUSY;
>>
>>- ret = virtio_transport_send_skb(skb, vq, vsock);
>
>nit: maybe we can add a comment here:
> /* GFP_ATOMIC because we are in RCU section, so we can't sleep */
>>+ ret = virtio_transport_send_skb(skb, vq, vsock, GFP_ATOMIC);
>> if (ret == 0)
>> virtqueue_kick(vq);
>>
>>--
>>MST
>>
>
>I'll run some tests and come back with R-b when it's done.
I replicated the issue enabling CONFIG_DEBUG_ATOMIC_SLEEP.
With that enabled, as soon as I run iperf-vsock, dmesg is flooded with
those messages. With this patch applied instead everything is fine.
I also ran the usual tests with various debugging options enabled and
everything seems okay.
With or without adding the comment I suggested in the previous email:
Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] vsock/virtio: use GFP_ATOMIC under RCU read lock
2024-10-02 13:41 [PATCH] vsock/virtio: use GFP_ATOMIC under RCU read lock Michael S. Tsirkin
2024-10-02 14:02 ` Stefano Garzarella
@ 2024-10-03 1:33 ` Luigi Leonardi
2024-10-03 9:09 ` Christian Brauner
` (2 subsequent siblings)
4 siblings, 0 replies; 8+ messages in thread
From: Luigi Leonardi @ 2024-10-03 1:33 UTC (permalink / raw)
To: mst
Cc: brauner, davem, edumazet, eperezma, jasowang, kuba, kvm,
linux-kernel, luigi.leonardi, marco.pinn95, netdev, pabeni,
sgarzare, stefanha, virtualization, xuanzhuo
> Link: https://lore.kernel.org/all/hfcr2aget2zojmqpr4uhlzvnep4vgskblx5b6xf2ddosbsrke7@nt34bxgp7j2x
> Fixes: efcd71af38be ("vsock/virtio: avoid queuing packets when intermediate queue is empty")
> Reported-by: Christian Brauner <brauner@kernel.org>
> Cc: Stefano Garzarella <sgarzare@redhat.com>
> Cc: Luigi Leonardi <luigi.leonardi@outlook.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>
> Lightly tested. Christian, could you pls confirm this fixes the problem
> for you? Stefano, it's a holiday here - could you pls help test!
> Thanks!
>
>
> net/vmw_vsock/virtio_transport.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/net/vmw_vsock/virtio_transport.c b/net/vmw_vsock/virtio_transport.c
> index f992f9a216f0..0cd965f24609 100644
> --- a/net/vmw_vsock/virtio_transport.c
> +++ b/net/vmw_vsock/virtio_transport.c
> @@ -96,7 +96,7 @@ static u32 virtio_transport_get_local_cid(void)
>
> /* Caller need to hold vsock->tx_lock on vq */
> static int virtio_transport_send_skb(struct sk_buff *skb, struct virtqueue *vq,
> - struct virtio_vsock *vsock)
> + struct virtio_vsock *vsock, gfp_t gfp)
> {
> int ret, in_sg = 0, out_sg = 0;
> struct scatterlist **sgs;
> @@ -140,7 +140,7 @@ static int virtio_transport_send_skb(struct sk_buff *skb, struct virtqueue *vq,
> }
> }
>
> - ret = virtqueue_add_sgs(vq, sgs, out_sg, in_sg, skb, GFP_KERNEL);
> + ret = virtqueue_add_sgs(vq, sgs, out_sg, in_sg, skb, gfp);
> /* Usually this means that there is no more space available in
> * the vq
> */
> @@ -178,7 +178,7 @@ virtio_transport_send_pkt_work(struct work_struct *work)
>
> reply = virtio_vsock_skb_reply(skb);
>
> - ret = virtio_transport_send_skb(skb, vq, vsock);
> + ret = virtio_transport_send_skb(skb, vq, vsock, GFP_KERNEL);
> if (ret < 0) {
> virtio_vsock_skb_queue_head(&vsock->send_pkt_queue, skb);
> break;
> @@ -221,7 +221,7 @@ static int virtio_transport_send_skb_fast_path(struct virtio_vsock *vsock, struc
> if (unlikely(ret == 0))
> return -EBUSY;
>
> - ret = virtio_transport_send_skb(skb, vq, vsock);
> + ret = virtio_transport_send_skb(skb, vq, vsock, GFP_ATOMIC);
> if (ret == 0)
> virtqueue_kick(vq);
>
> --
> MST
>
>
Thanks for fixing this!
I enabled CONFIG_DEBUG_ATOMIC_SLEEP as Stefano suggested and tested with and
without the fix, I can confirm that this fixes the problem.
Reviewed-by: Luigi Leonardi <luigi.leonardi@outlook.com>
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] vsock/virtio: use GFP_ATOMIC under RCU read lock
2024-10-02 13:41 [PATCH] vsock/virtio: use GFP_ATOMIC under RCU read lock Michael S. Tsirkin
2024-10-02 14:02 ` Stefano Garzarella
2024-10-03 1:33 ` Luigi Leonardi
@ 2024-10-03 9:09 ` Christian Brauner
2024-10-03 10:03 ` Gupta, Pankaj
2024-10-07 15:39 ` Jakub Kicinski
4 siblings, 0 replies; 8+ messages in thread
From: Christian Brauner @ 2024-10-03 9:09 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: linux-kernel, Stefano Garzarella, Luigi Leonardi, Jason Wang,
Xuan Zhuo, Eugenio Pérez, Stefan Hajnoczi, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, Marco Pinna,
virtualization, kvm, netdev
On Wed, Oct 02, 2024 at 09:41:42AM GMT, Michael S. Tsirkin wrote:
> virtio_transport_send_pkt in now called on transport fast path,
> under RCU read lock. In that case, we have a bug: virtio_add_sgs
> is called with GFP_KERNEL, and might sleep.
>
> Pass the gfp flags as an argument, and use GFP_ATOMIC on
> the fast path.
>
> Link: https://lore.kernel.org/all/hfcr2aget2zojmqpr4uhlzvnep4vgskblx5b6xf2ddosbsrke7@nt34bxgp7j2x
> Fixes: efcd71af38be ("vsock/virtio: avoid queuing packets when intermediate queue is empty")
> Reported-by: Christian Brauner <brauner@kernel.org>
> Cc: Stefano Garzarella <sgarzare@redhat.com>
> Cc: Luigi Leonardi <luigi.leonardi@outlook.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>
> Lightly tested. Christian, could you pls confirm this fixes the problem
> for you? Stefano, it's a holiday here - could you pls help test!
> Thanks!
Thank you for the quick fix:
Reviewed-by: Christian Brauner <brauner@kernel.org>
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] vsock/virtio: use GFP_ATOMIC under RCU read lock
2024-10-02 13:41 [PATCH] vsock/virtio: use GFP_ATOMIC under RCU read lock Michael S. Tsirkin
` (2 preceding siblings ...)
2024-10-03 9:09 ` Christian Brauner
@ 2024-10-03 10:03 ` Gupta, Pankaj
2024-10-07 15:39 ` Jakub Kicinski
4 siblings, 0 replies; 8+ messages in thread
From: Gupta, Pankaj @ 2024-10-03 10:03 UTC (permalink / raw)
To: Michael S. Tsirkin, linux-kernel
Cc: Christian Brauner, Stefano Garzarella, Luigi Leonardi, Jason Wang,
Xuan Zhuo, Eugenio Pérez, Stefan Hajnoczi, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, Marco Pinna,
virtualization, kvm, netdev
> virtio_transport_send_pkt in now called on transport fast path,
> under RCU read lock. In that case, we have a bug: virtio_add_sgs
> is called with GFP_KERNEL, and might sleep.
>
> Pass the gfp flags as an argument, and use GFP_ATOMIC on
> the fast path.
>
> Link: https://lore.kernel.org/all/hfcr2aget2zojmqpr4uhlzvnep4vgskblx5b6xf2ddosbsrke7@nt34bxgp7j2x
> Fixes: efcd71af38be ("vsock/virtio: avoid queuing packets when intermediate queue is empty")
> Reported-by: Christian Brauner <brauner@kernel.org>
> Cc: Stefano Garzarella <sgarzare@redhat.com>
> Cc: Luigi Leonardi <luigi.leonardi@outlook.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Reviewed-by: Pankaj Gupta <pankaj.gupta@amd.com>
> ---
>
> Lightly tested. Christian, could you pls confirm this fixes the problem
> for you? Stefano, it's a holiday here - could you pls help test!
> Thanks!
>
>
> net/vmw_vsock/virtio_transport.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/net/vmw_vsock/virtio_transport.c b/net/vmw_vsock/virtio_transport.c
> index f992f9a216f0..0cd965f24609 100644
> --- a/net/vmw_vsock/virtio_transport.c
> +++ b/net/vmw_vsock/virtio_transport.c
> @@ -96,7 +96,7 @@ static u32 virtio_transport_get_local_cid(void)
>
> /* Caller need to hold vsock->tx_lock on vq */
> static int virtio_transport_send_skb(struct sk_buff *skb, struct virtqueue *vq,
> - struct virtio_vsock *vsock)
> + struct virtio_vsock *vsock, gfp_t gfp)
> {
> int ret, in_sg = 0, out_sg = 0;
> struct scatterlist **sgs;
> @@ -140,7 +140,7 @@ static int virtio_transport_send_skb(struct sk_buff *skb, struct virtqueue *vq,
> }
> }
>
> - ret = virtqueue_add_sgs(vq, sgs, out_sg, in_sg, skb, GFP_KERNEL);
> + ret = virtqueue_add_sgs(vq, sgs, out_sg, in_sg, skb, gfp);
> /* Usually this means that there is no more space available in
> * the vq
> */
> @@ -178,7 +178,7 @@ virtio_transport_send_pkt_work(struct work_struct *work)
>
> reply = virtio_vsock_skb_reply(skb);
>
> - ret = virtio_transport_send_skb(skb, vq, vsock);
> + ret = virtio_transport_send_skb(skb, vq, vsock, GFP_KERNEL);
> if (ret < 0) {
> virtio_vsock_skb_queue_head(&vsock->send_pkt_queue, skb);
> break;
> @@ -221,7 +221,7 @@ static int virtio_transport_send_skb_fast_path(struct virtio_vsock *vsock, struc
> if (unlikely(ret == 0))
> return -EBUSY;
>
> - ret = virtio_transport_send_skb(skb, vq, vsock);
> + ret = virtio_transport_send_skb(skb, vq, vsock, GFP_ATOMIC);
> if (ret == 0)
> virtqueue_kick(vq);
>
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] vsock/virtio: use GFP_ATOMIC under RCU read lock
2024-10-02 13:41 [PATCH] vsock/virtio: use GFP_ATOMIC under RCU read lock Michael S. Tsirkin
` (3 preceding siblings ...)
2024-10-03 10:03 ` Gupta, Pankaj
@ 2024-10-07 15:39 ` Jakub Kicinski
2024-10-07 15:46 ` Michael S. Tsirkin
4 siblings, 1 reply; 8+ messages in thread
From: Jakub Kicinski @ 2024-10-07 15:39 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: linux-kernel, Christian Brauner, Stefano Garzarella,
Luigi Leonardi, Jason Wang, Xuan Zhuo, Eugenio Pérez,
Stefan Hajnoczi, David S. Miller, Eric Dumazet, Paolo Abeni,
Marco Pinna, virtualization, kvm, netdev
On Wed, 2 Oct 2024 09:41:42 -0400 Michael S. Tsirkin wrote:
> virtio_transport_send_pkt in now called on transport fast path,
> under RCU read lock. In that case, we have a bug: virtio_add_sgs
> is called with GFP_KERNEL, and might sleep.
>
> Pass the gfp flags as an argument, and use GFP_ATOMIC on
> the fast path.
Hi Michael! The To: linux-kernel@vger.kernel.org doesn't give much info
on who you expect to apply this ;) Please let us know if you plan to
take it via your own tree, otherwise we'll ship it to Linus on Thu.
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] vsock/virtio: use GFP_ATOMIC under RCU read lock
2024-10-07 15:39 ` Jakub Kicinski
@ 2024-10-07 15:46 ` Michael S. Tsirkin
0 siblings, 0 replies; 8+ messages in thread
From: Michael S. Tsirkin @ 2024-10-07 15:46 UTC (permalink / raw)
To: Jakub Kicinski
Cc: linux-kernel, Christian Brauner, Stefano Garzarella,
Luigi Leonardi, Jason Wang, Xuan Zhuo, Eugenio Pérez,
Stefan Hajnoczi, David S. Miller, Eric Dumazet, Paolo Abeni,
Marco Pinna, virtualization, kvm, netdev
On Mon, Oct 07, 2024 at 08:39:20AM -0700, Jakub Kicinski wrote:
> On Wed, 2 Oct 2024 09:41:42 -0400 Michael S. Tsirkin wrote:
> > virtio_transport_send_pkt in now called on transport fast path,
> > under RCU read lock. In that case, we have a bug: virtio_add_sgs
> > is called with GFP_KERNEL, and might sleep.
> >
> > Pass the gfp flags as an argument, and use GFP_ATOMIC on
> > the fast path.
>
> Hi Michael! The To: linux-kernel@vger.kernel.org doesn't give much info
> on who you expect to apply this ;) Please let us know if you plan to
> take it via your own tree, otherwise we'll ship it to Linus on Thu.
Hi!
It's in my tree, was in the process of sending a pull request actually.
^ permalink raw reply [flat|nested] 8+ messages in thread