diff mbox series

[RFC,v2,2/4] virtio/vsock: remove all data from sk_buff

Message ID dfadea17-a91e-105f-c213-a73f9731c8bd@sberdevices.ru
State New
Headers
Series virtio/vsock: fix credit update logic |

Commit Message

Arseniy Krasnov March 5, 2023, 8:07 p.m. UTC 
  In case of SOCK_SEQPACKET all sk_buffs are used once - after read some
data from it, it will be removed, so user will never read rest of the
data. Thus we need to update credit parameters of the socket like whole
sk_buff is read - so call 'skb_pull()' for the whole buffer.

Fixes: 71dc9ec9ac7d ("virtio/vsock: replace virtio_vsock_pkt with sk_buff")
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 6, 2023, 12:08 p.m. UTC | #1 
On Sun, Mar 05, 2023 at 11:07:37PM +0300, Arseniy Krasnov wrote:
>In case of SOCK_SEQPACKET all sk_buffs are used once - after read some
>data from it, it will be removed, so user will never read rest of the
>data. Thus we need to update credit parameters of the socket like whole
>sk_buff is read - so call 'skb_pull()' for the whole buffer.
>
>Fixes: 71dc9ec9ac7d ("virtio/vsock: replace virtio_vsock_pkt with sk_buff")
>Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru>
>---
> net/vmw_vsock/virtio_transport_common.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)

Maybe we could avoid this patch if we directly use pkt_len as I
suggested in the previous patch.

Thanks,
Stefano

>
>diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
>index 2e2a773df5c1..30b0539990ba 100644
>--- a/net/vmw_vsock/virtio_transport_common.c
>+++ b/net/vmw_vsock/virtio_transport_common.c
>@@ -466,7 +466,6 @@ static int virtio_transport_seqpacket_do_dequeue(struct vsock_sock *vsk,
> 					dequeued_len = err;
> 				} else {
> 					user_buf_len -= bytes_to_copy;
>-					skb_pull(skb, bytes_to_copy);
> 				}
>
> 				spin_lock_bh(&vvs->rx_lock);
>@@ -484,6 +483,7 @@ static int virtio_transport_seqpacket_do_dequeue(struct vsock_sock *vsk,
> 				msg->msg_flags |= MSG_EOR;
> 		}
>
>+		skb_pull(skb, skb->len);
> 		virtio_transport_dec_rx_pkt(vvs, skb);
> 		kfree_skb(skb);
> 	}
>-- 
>2.25.1
>
Arseniy Krasnov March 6, 2023, 3:31 p.m. UTC | #2 
On 06.03.2023 15:08, Stefano Garzarella wrote:
> On Sun, Mar 05, 2023 at 11:07:37PM +0300, Arseniy Krasnov wrote:
>> In case of SOCK_SEQPACKET all sk_buffs are used once - after read some
>> data from it, it will be removed, so user will never read rest of the
>> data. Thus we need to update credit parameters of the socket like whole
>> sk_buff is read - so call 'skb_pull()' for the whole buffer.
>>
>> Fixes: 71dc9ec9ac7d ("virtio/vsock: replace virtio_vsock_pkt with sk_buff")
>> Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru>
>> ---
>> net/vmw_vsock/virtio_transport_common.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
> 
> Maybe we could avoid this patch if we directly use pkt_len as I
> suggested in the previous patch.
Hm, may be we can avoid calling 'skb_pull()' here if 'virtio_transport_dec_rx_pkt()'
will use integer argument? Just call 'virtio_transport_dec_rx_pkt(skb->len)'. skb
is never returned to queue to read it again, so i think may be there is no sense for
extra call 'skb_pull'?

Thanks, Arseniy
> 
> Thanks,
> Stefano
> 
>>
>> diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
>> index 2e2a773df5c1..30b0539990ba 100644
>> --- a/net/vmw_vsock/virtio_transport_common.c
>> +++ b/net/vmw_vsock/virtio_transport_common.c
>> @@ -466,7 +466,6 @@ static int virtio_transport_seqpacket_do_dequeue(struct vsock_sock *vsk,
>>                     dequeued_len = err;
>>                 } else {
>>                     user_buf_len -= bytes_to_copy;
>> -                    skb_pull(skb, bytes_to_copy);
>>                 }
>>
>>                 spin_lock_bh(&vvs->rx_lock);
>> @@ -484,6 +483,7 @@ static int virtio_transport_seqpacket_do_dequeue(struct vsock_sock *vsk,
>>                 msg->msg_flags |= MSG_EOR;
>>         }
>>
>> +        skb_pull(skb, skb->len);
>>         virtio_transport_dec_rx_pkt(vvs, skb);
>>         kfree_skb(skb);
>>     }
>> -- 
>> 2.25.1
>>
>
Stefano Garzarella March 6, 2023, 3:51 p.m. UTC | #3 
On Mon, Mar 06, 2023 at 06:31:22PM +0300, Arseniy Krasnov wrote:
>
>
>On 06.03.2023 15:08, Stefano Garzarella wrote:
>> On Sun, Mar 05, 2023 at 11:07:37PM +0300, Arseniy Krasnov wrote:
>>> In case of SOCK_SEQPACKET all sk_buffs are used once - after read some
>>> data from it, it will be removed, so user will never read rest of the
>>> data. Thus we need to update credit parameters of the socket like whole
>>> sk_buff is read - so call 'skb_pull()' for the whole buffer.
>>>
>>> Fixes: 71dc9ec9ac7d ("virtio/vsock: replace virtio_vsock_pkt with sk_buff")
>>> Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru>
>>> ---
>>> net/vmw_vsock/virtio_transport_common.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> Maybe we could avoid this patch if we directly use pkt_len as I
>> suggested in the previous patch.
>Hm, may be we can avoid calling 'skb_pull()' here if 'virtio_transport_dec_rx_pkt()'
>will use integer argument?

Yep, exactly!

>Just call 'virtio_transport_dec_rx_pkt(skb->len)'. skb

It depends on how we call virtio_transport_inc_rx_pkt(). If we use
hdr->len there I would use the same to avoid confusion. Plus that's the
value the other peer sent us, so definitely the right value to increase
fwd_cnt with. But if skb->len always reflects it, then that's fine.

>is never returned to queue to read it again, so i think may be there is no sense for
>extra call 'skb_pull'?

Right!

Thanks,
Stefano
Arseniy Krasnov March 6, 2023, 4 p.m. UTC | #4 
On 06.03.2023 18:51, Stefano Garzarella wrote:
> On Mon, Mar 06, 2023 at 06:31:22PM +0300, Arseniy Krasnov wrote:
>>
>>
>> On 06.03.2023 15:08, Stefano Garzarella wrote:
>>> On Sun, Mar 05, 2023 at 11:07:37PM +0300, Arseniy Krasnov wrote:
>>>> In case of SOCK_SEQPACKET all sk_buffs are used once - after read some
>>>> data from it, it will be removed, so user will never read rest of the
>>>> data. Thus we need to update credit parameters of the socket like whole
>>>> sk_buff is read - so call 'skb_pull()' for the whole buffer.
>>>>
>>>> Fixes: 71dc9ec9ac7d ("virtio/vsock: replace virtio_vsock_pkt with sk_buff")
>>>> Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru>
>>>> ---
>>>> net/vmw_vsock/virtio_transport_common.c | 2 +-
>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> Maybe we could avoid this patch if we directly use pkt_len as I
>>> suggested in the previous patch.
>> Hm, may be we can avoid calling 'skb_pull()' here if 'virtio_transport_dec_rx_pkt()'
>> will use integer argument?
> 
> Yep, exactly!
> 
>> Just call 'virtio_transport_dec_rx_pkt(skb->len)'. skb
> 
> It depends on how we call virtio_transport_inc_rx_pkt(). If we use
> hdr->len there I would use the same to avoid confusion. Plus that's the
> value the other peer sent us, so definitely the right value to increase
> fwd_cnt with. But if skb->len always reflects it, then that's fine.
i've checked 'virtio_transport_rx_work()', it calls 'virtio_vsock_skb_rx_put()' which
sets 'skb->len'. Value is used from header, so seems 'skb->len' == 'hdr->len' in this
case.
> 
>> is never returned to queue to read it again, so i think may be there is no sense for
>> extra call 'skb_pull'?
> 
> Right!
> 
> Thanks,
> Stefano
>
Stefano Garzarella March 6, 2023, 4:18 p.m. UTC | #5 
On Mon, Mar 06, 2023 at 07:00:10PM +0300, Arseniy Krasnov wrote:
>
>
>On 06.03.2023 18:51, Stefano Garzarella wrote:
>> On Mon, Mar 06, 2023 at 06:31:22PM +0300, Arseniy Krasnov wrote:
>>>
>>>
>>> On 06.03.2023 15:08, Stefano Garzarella wrote:
>>>> On Sun, Mar 05, 2023 at 11:07:37PM +0300, Arseniy Krasnov wrote:
>>>>> In case of SOCK_SEQPACKET all sk_buffs are used once - after read some
>>>>> data from it, it will be removed, so user will never read rest of the
>>>>> data. Thus we need to update credit parameters of the socket like whole
>>>>> sk_buff is read - so call 'skb_pull()' for the whole buffer.
>>>>>
>>>>> Fixes: 71dc9ec9ac7d ("virtio/vsock: replace virtio_vsock_pkt with sk_buff")
>>>>> Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru>
>>>>> ---
>>>>> net/vmw_vsock/virtio_transport_common.c | 2 +-
>>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> Maybe we could avoid this patch if we directly use pkt_len as I
>>>> suggested in the previous patch.
>>> Hm, may be we can avoid calling 'skb_pull()' here if 'virtio_transport_dec_rx_pkt()'
>>> will use integer argument?
>>
>> Yep, exactly!
>>
>>> Just call 'virtio_transport_dec_rx_pkt(skb->len)'. skb
>>
>> It depends on how we call virtio_transport_inc_rx_pkt(). If we use
>> hdr->len there I would use the same to avoid confusion. Plus that's the
>> value the other peer sent us, so definitely the right value to increase
>> fwd_cnt with. But if skb->len always reflects it, then that's fine.
>i've checked 'virtio_transport_rx_work()', it calls 'virtio_vsock_skb_rx_put()' which
>sets 'skb->len'. Value is used from header, so seems 'skb->len' == 'hdr->len' in this
>case.

Thank you for checking it.

However, I still think it is better to use `hdr->len` (we have to assign 
it to `pkt_len` anyway, as in the proposal I sent for patch 1), 
otherwise we have to go every time to check if skb_* functions touch 
skb->len.

E.g. skb_pull() decrease skb->len, so I'm not sure we can call 
virtio_transport_dec_rx_pkt(skb->len) if we don't remove `skb_pull(skb, 
bytes_to_copy);` inside the loop.

Thanks,
Stefano
Bobby Eshleman March 7, 2023, 11:53 p.m. UTC | #6 
On Mon, Mar 06, 2023 at 05:18:52PM +0100, Stefano Garzarella wrote:
> On Mon, Mar 06, 2023 at 07:00:10PM +0300, Arseniy Krasnov wrote:
> > 
> > 
> > On 06.03.2023 18:51, Stefano Garzarella wrote:
> > > On Mon, Mar 06, 2023 at 06:31:22PM +0300, Arseniy Krasnov wrote:
> > > > 
> > > > 
> > > > On 06.03.2023 15:08, Stefano Garzarella wrote:
> > > > > On Sun, Mar 05, 2023 at 11:07:37PM +0300, Arseniy Krasnov wrote:
> > > > > > In case of SOCK_SEQPACKET all sk_buffs are used once - after read some
> > > > > > data from it, it will be removed, so user will never read rest of the
> > > > > > data. Thus we need to update credit parameters of the socket like whole
> > > > > > sk_buff is read - so call 'skb_pull()' for the whole buffer.
> > > > > > 
> > > > > > Fixes: 71dc9ec9ac7d ("virtio/vsock: replace virtio_vsock_pkt with sk_buff")
> > > > > > Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru>
> > > > > > ---
> > > > > > net/vmw_vsock/virtio_transport_common.c | 2 +-
> > > > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > 
> > > > > Maybe we could avoid this patch if we directly use pkt_len as I
> > > > > suggested in the previous patch.
> > > > Hm, may be we can avoid calling 'skb_pull()' here if 'virtio_transport_dec_rx_pkt()'
> > > > will use integer argument?
> > > 
> > > Yep, exactly!
> > > 
> > > > Just call 'virtio_transport_dec_rx_pkt(skb->len)'. skb
> > > 
> > > It depends on how we call virtio_transport_inc_rx_pkt(). If we use
> > > hdr->len there I would use the same to avoid confusion. Plus that's the
> > > value the other peer sent us, so definitely the right value to increase
> > > fwd_cnt with. But if skb->len always reflects it, then that's fine.
> > i've checked 'virtio_transport_rx_work()', it calls 'virtio_vsock_skb_rx_put()' which
> > sets 'skb->len'. Value is used from header, so seems 'skb->len' == 'hdr->len' in this
> > case.
> 
> Thank you for checking it.
> 
> However, I still think it is better to use `hdr->len` (we have to assign it
> to `pkt_len` anyway, as in the proposal I sent for patch 1), otherwise we
> have to go every time to check if skb_* functions touch skb->len.
> 
> E.g. skb_pull() decrease skb->len, so I'm not sure we can call
> virtio_transport_dec_rx_pkt(skb->len) if we don't remove `skb_pull(skb,
> bytes_to_copy);` inside the loop.
> 

I think it does make reasoning about the bytes accounting easier if it
is based off of the non-mutating hdr->len.

Especially if vsock does ever support tunneling (e.g., through
virtio-net) or some future feature that makes the skb->len more dynamic.

Best,
Bobby
diff mbox series

Patch

diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
index 2e2a773df5c1..30b0539990ba 100644
--- a/net/vmw_vsock/virtio_transport_common.c
+++ b/net/vmw_vsock/virtio_transport_common.c
@@ -466,7 +466,6 @@  static int virtio_transport_seqpacket_do_dequeue(struct vsock_sock *vsk,
 					dequeued_len = err;
 				} else {
 					user_buf_len -= bytes_to_copy;
-					skb_pull(skb, bytes_to_copy);
 				}
 
 				spin_lock_bh(&vvs->rx_lock);
@@ -484,6 +483,7 @@  static int virtio_transport_seqpacket_do_dequeue(struct vsock_sock *vsk,
 				msg->msg_flags |= MSG_EOR;
 		}
 
+		skb_pull(skb, skb->len);
 		virtio_transport_dec_rx_pkt(vvs, skb);
 		kfree_skb(skb);
 	}