| Message ID | dfadea17-a91e-105f-c213-a73f9731c8bd@sberdevices.ru (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | virtio/vsock: fix credit update logic | expand |
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 >
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 >> >
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
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 >
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
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 --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); }
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(-)