Date:   Tue, 21 Mar 2023 09:31:18 +0100
From:   Stefano Garzarella <sgarzare@redhat.com>
To:     Arseniy Krasnov <avkrasnov@sberdevices.ru>
Cc:     Stefan Hajnoczi <stefanha@redhat.com>,
        "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>,
        Bobby Eshleman <bobby.eshleman@bytedance.com>,
        kvm@vger.kernel.org, virtualization@lists.linux-foundation.org,
        netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
        kernel@sberdevices.ru, oxffffaa@gmail.com
Subject: Re: [RFC PATCH v1 1/3] virtio/vsock: fix header length on skb merging
Message-ID: <20230321083118.63pcdeklvjbqs3as@sgarzare-redhat>
References: <e141e6f1-00ae-232c-b840-b146bdb10e99@sberdevices.ru>
 <63445f2f-a0bb-153c-0e15-74a09ea26dc1@sberdevices.ru>
 <20230320145718.5gytg6t5pcz5rpnm@sgarzare-redhat>
 <329372cf-ef01-8a20-da6e-8c1f9795e41a@sberdevices.ru>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii; format=flowed
Content-Disposition: inline
In-Reply-To: <329372cf-ef01-8a20-da6e-8c1f9795e41a@sberdevices.ru>
Precedence: bulk

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