| Message ID | 0683cc6e-5130-484c-1105-ef2eb792d355@sberdevices.ru (mailing list archive) |
|---|---|
| Headers | show |
| Series | fix header length on skb merging | expand |
Hello: This series was applied to netdev/net.git (main) by Paolo Abeni <pabeni@redhat.com>: On Tue, 28 Mar 2023 14:30:10 +0300 you wrote: > Hello, > > this patchset fixes appending newly arrived skbuff to the last skbuff of > the socket's queue during rx path. Problem fires when we are trying to > append data to skbuff which was already processed in dequeue callback > at least once. Dequeue callback calls function 'skb_pull()' which changes > 'skb->len'. In current implementation 'skb->len' is used to update length > in header of last skbuff after new data was copied to it. This is bug, > because value in header is used to calculate 'rx_bytes'/'fwd_cnt' and > thus must be constant during skbuff lifetime. Here is example, we have > two skbuffs: skb0 with length 10 and skb1 with length 4. > > [...] Here is the summary with links: - [net,v2,1/3] virtio/vsock: fix header length on skb merging https://git.kernel.org/netdev/net/c/f7154d967bc4 - [net,v2,2/3] virtio/vsock: WARN_ONCE() for invalid state of socket https://git.kernel.org/netdev/net/c/b8d2f61fdf2a - [net,v2,3/3] test/vsock: new skbuff appending test https://git.kernel.org/netdev/net/c/25209a3209ec You are awesome, thank you!
Hello, this patchset fixes appending newly arrived skbuff to the last skbuff of the socket's queue during rx path. Problem fires when we are trying to append data to skbuff which was already processed in dequeue callback at least once. Dequeue callback calls function 'skb_pull()' which changes 'skb->len'. In current implementation 'skb->len' is used to update length in header of last skbuff after new data was copied to it. This is bug, because value in header is used to calculate 'rx_bytes'/'fwd_cnt' and thus must be constant during skbuff lifetime. Here is example, we have two skbuffs: skb0 with length 10 and skb1 with length 4. 1) skb0 arrives, hdr->len == skb->len == 10, rx_bytes == 10 2) Read 3 bytes from skb0, skb->len == 7, hdr->len == 10, rx_bytes == 10 3) skb1 arrives, hdr->len == skb->len == 4, rx_bytes == 14 4) Append skb1 to skb0, skb0 now has skb->len == 11, hdr->len == 11. But value of 11 in header is invalid. 5) Read whole skb0, update rx_bytes by 11 from skb0's header. 6) At this moment rx_bytes == 3, but socket's queue is empty. This bug starts to fire since: commit 077706165717 ("virtio/vsock: don't use skbuff state to account credit") In fact, it presents before, but didn't triggered due to a little bit buggy implementation of credit calculation logic. So i'll use Fixes tag for it. I really forgot about this branch in rx path when implemented patch 077706165717. This patchset contains 3 patches: 1) Fix itself. 2) Patch with WARN_ONCE() to catch such problems in future. 3) Patch with test which triggers skb appending logic. It looks like simple test with several 'send()' and 'recv()', but it checks, that skbuff appending works ok. Link to v1: https://lore.kernel.org/netdev/e141e6f1-00ae-232c-b840-b146bdb10e99@sberdevices.ru/ Changelog: v1 -> v2: - Replace 'WARN()' with 'WARN_ONCE()'. - Clean and refactor source code of the reproducer, now it is test for vsock_test suite. - Commit messages update. Arseniy Krasnov (3): virtio/vsock: fix header length on skb merging virtio/vsock: WARN_ONCE() for invalid state of socket test/vsock: new skbuff appending test net/vmw_vsock/virtio_transport_common.c | 9 ++- tools/testing/vsock/vsock_test.c | 90 +++++++++++++++++++++++++ 2 files changed, 98 insertions(+), 1 deletion(-)