On 08/11/2020 8:59, Shai Malin wrote:
> On 09/10/2020 1:44, Sagi Grimberg wrote:
>> On 9/30/20 7:20 PM, Boris Pismenny wrote:
>>
>>> crc offload of the nvme capsule. Check if all the skb bits are on,
>>> and if not recalculate the crc in SW and check it.
>> Can you clarify in the patch description that this is only for pdu
>> data digest and not header digest?
>>
> Not a security expert, but according to my understanding, the NVMeTCP data
> digest is a layer 5 CRC, and as such it is expected to be end-to-end,
> meaning it is computed by layer 5 on the transmitter and verified on layer 5
> on the receiver.
> Any data corruption which happens in any of the lower layers, including their
> software processing, should be protected by this CRC. For example, if the IP
> or TCP stack has a bug that corrupts the NVMeTCP payload data, the CRC should
> protect against it. It seems that may not be the case with this offload.
If the TCP/IP stack corrupts packet data, then likely many TCP/IP consumers
will be effected, and it will be fixed promptly.
Unlike with TOE, these bugs can be easily fixed/hotpatched by the community.
>
>>> This patch reworks the receive-side crc calculation to always run at
>>> the end, so as to keep a single flow for both offload and non-offload.
>>> This change simplifies the code, but it may degrade performance for
>>> non-offload crc calculation.
>> ??
>>
>> From my scan it doeesn't look like you do that.. Am I missing something?
>> Can you explain?
>>
>>> Signed-off-by: Boris Pismenny <bor...@mellanox.com>
>>> Signed-off-by: Ben Ben-Ishay <benis...@mellanox.com>
>>> Signed-off-by: Or Gerlitz <ogerl...@mellanox.com>
>>> Signed-off-by: Yoray Zack <yor...@mellanox.com>
>>> ---
>>> drivers/nvme/host/tcp.c | 66
>> ++++++++++++++++++++++++++++++++++++-----
>>> 1 file changed, 58 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c index
>>> 7bd97f856677..9a620d1dacb4 100644
>>> --- a/drivers/nvme/host/tcp.c
>>> +++ b/drivers/nvme/host/tcp.c
>>> @@ -94,6 +94,7 @@ struct nvme_tcp_queue {
>>> size_t data_remaining;
>>> size_t ddgst_remaining;
>>> unsigned int nr_cqe;
>>> + bool crc_valid;
> I suggest to rename it to ddgst_valid.
>
Sure
