| Message ID | 63445f2f-a0bb-153c-0e15-74a09ea26dc1@sberdevices.ru (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fix header length on skb merging | expand |
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 >
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 >> >
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 --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; } }
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(-)