diff mbox series

[RFC,v1,1/3] virtio/vsock: fix header length on skb merging

Message ID 63445f2f-a0bb-153c-0e15-74a09ea26dc1@sberdevices.ru
State New
Headers
Series fix header length on skb merging |

Commit Message

Arseniy Krasnov March 19, 2023, 6:51 p.m. UTC 
  This fixes header length calculation of skbuff during data appending to
it. When such skbuff is processed in dequeue callbacks, e.g. 'skb_pull()'
is called on it, 'skb->len' is dynamic value, so it is impossible to use
it in header, because value from header must be permanent for valid
credit calculation ('rx_bytes'/'fwd_cnt').

Fixes: 077706165717 ("virtio/vsock: don't use skbuff state to account credit")
Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru>
---
 net/vmw_vsock/virtio_transport_common.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Stefano Garzarella March 20, 2023, 2:57 p.m. UTC | #1 
On Sun, Mar 19, 2023 at 09:51:06PM +0300, Arseniy Krasnov wrote:
>This fixes header length calculation of skbuff during data appending to
>it. When such skbuff is processed in dequeue callbacks, e.g. 'skb_pull()'
>is called on it, 'skb->len' is dynamic value, so it is impossible to use
>it in header, because value from header must be permanent for valid
>credit calculation ('rx_bytes'/'fwd_cnt').
>
>Fixes: 077706165717 ("virtio/vsock: don't use skbuff state to account credit")

I don't understand how this commit introduced this problem, can you
explain it better?

Is it related more to the credit than to the size in the header itself?

Anyway, the patch LGTM, but we should explain better the issue.

Thanks,
Stefano

>Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru>
>---
> net/vmw_vsock/virtio_transport_common.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
>index 6d15cd4d090a..3c75986e16c2 100644
>--- a/net/vmw_vsock/virtio_transport_common.c
>+++ b/net/vmw_vsock/virtio_transport_common.c
>@@ -1091,7 +1091,7 @@ virtio_transport_recv_enqueue(struct vsock_sock *vsk,
> 			memcpy(skb_put(last_skb, skb->len), skb->data, skb->len);
> 			free_pkt = true;
> 			last_hdr->flags |= hdr->flags;
>-			last_hdr->len = cpu_to_le32(last_skb->len);
>+			le32_add_cpu(&last_hdr->len, len);
> 			goto out;
> 		}
> 	}
>-- 
>2.25.1
>
Arseniy Krasnov March 20, 2023, 6:10 p.m. UTC | #2 
On 20.03.2023 17:57, Stefano Garzarella wrote:
> On Sun, Mar 19, 2023 at 09:51:06PM +0300, Arseniy Krasnov wrote:
>> This fixes header length calculation of skbuff during data appending to
>> it. When such skbuff is processed in dequeue callbacks, e.g. 'skb_pull()'
>> is called on it, 'skb->len' is dynamic value, so it is impossible to use
>> it in header, because value from header must be permanent for valid
>> credit calculation ('rx_bytes'/'fwd_cnt').
>>
>> Fixes: 077706165717 ("virtio/vsock: don't use skbuff state to account credit")
> 
> I don't understand how this commit introduced this problem, can you
> explain it better?
Sorry, seems i said it wrong a little bit. Before 0777, implementation was buggy, but
exactly this problem was not actual - it didn't triggered somehow. I checked it with
reproducer from this patch. But in 0777 as value from header was used to 'rx_bytes'
calculation, bug become actual. Yes, may be it is not "Fixes:" for 0777, but critical
addition. I'm not sure.
> 
> Is it related more to the credit than to the size in the header itself?
> 
It is related to size in header more.
> Anyway, the patch LGTM, but we should explain better the issue.
>
 
Ok, I'll write it more clear in the commit message.

Thanks, Arseniy

> Thanks,
> Stefano
> 
>> Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru>
>> ---
>> net/vmw_vsock/virtio_transport_common.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
>> index 6d15cd4d090a..3c75986e16c2 100644
>> --- a/net/vmw_vsock/virtio_transport_common.c
>> +++ b/net/vmw_vsock/virtio_transport_common.c
>> @@ -1091,7 +1091,7 @@ virtio_transport_recv_enqueue(struct vsock_sock *vsk,
>>             memcpy(skb_put(last_skb, skb->len), skb->data, skb->len);
>>             free_pkt = true;
>>             last_hdr->flags |= hdr->flags;
>> -            last_hdr->len = cpu_to_le32(last_skb->len);
>> +            le32_add_cpu(&last_hdr->len, len);
>>             goto out;
>>         }
>>     }
>> -- 
>> 2.25.1
>>
>
Stefano Garzarella March 21, 2023, 8:31 a.m. UTC | #3 
On Mon, Mar 20, 2023 at 09:10:13PM +0300, Arseniy Krasnov wrote:
>
>
>On 20.03.2023 17:57, Stefano Garzarella wrote:
>> On Sun, Mar 19, 2023 at 09:51:06PM +0300, Arseniy Krasnov wrote:
>>> This fixes header length calculation of skbuff during data appending to
>>> it. When such skbuff is processed in dequeue callbacks, e.g. 'skb_pull()'
>>> is called on it, 'skb->len' is dynamic value, so it is impossible to use
>>> it in header, because value from header must be permanent for valid
>>> credit calculation ('rx_bytes'/'fwd_cnt').
>>>
>>> Fixes: 077706165717 ("virtio/vsock: don't use skbuff state to account credit")
>>
>> I don't understand how this commit introduced this problem, can you
>> explain it better?
>Sorry, seems i said it wrong a little bit. Before 0777, implementation was buggy, but
>exactly this problem was not actual - it didn't triggered somehow. I checked it with
>reproducer from this patch. But in 0777 as value from header was used to 'rx_bytes'
>calculation, bug become actual. Yes, may be it is not "Fixes:" for 0777, but critical
>addition. I'm not sure.
>>
>> Is it related more to the credit than to the size in the header itself?
>>
>It is related to size in header more.
>> Anyway, the patch LGTM, but we should explain better the issue.
>>
>
>Ok, I'll write it more clear in the commit message.

Okay, if 077706165717 triggered the problem, even if it was there from
before, then IMHO it is okay to use that commit in Fixes.

Please, explain it better in the message, so it's clear for everyone ;-)

Thanks,
Stefano
diff mbox series

Patch

diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
index 6d15cd4d090a..3c75986e16c2 100644
--- a/net/vmw_vsock/virtio_transport_common.c
+++ b/net/vmw_vsock/virtio_transport_common.c
@@ -1091,7 +1091,7 @@  virtio_transport_recv_enqueue(struct vsock_sock *vsk,
 			memcpy(skb_put(last_skb, skb->len), skb->data, skb->len);
 			free_pkt = true;
 			last_hdr->flags |= hdr->flags;
-			last_hdr->len = cpu_to_le32(last_skb->len);
+			le32_add_cpu(&last_hdr->len, len);
 			goto out;
 		}
 	}