updated ... I added SMB3: to the title 62593011247c SMB3: Resolve data corruption of TCP server info fields (since it was SMB3 only fix doesn't affect cifs) On Wed, Oct 21, 2020 at 4:03 PM Pavel Shilovsky <piastryyy@xxxxxxxxx> wrote: > > Thanks. Should we add "cifs: " prefix to the beginning of the patch > title? Given that the patch goes to stable this may worth doing > another rebase. > -- > Best regards, > Pavel Shilovsky > > вт, 20 окт. 2020 г. в 15:08, Steve French <smfrench@xxxxxxxxx>: > > > > Added cc:stable 5.4+ and the 2 Reviewed-bys and merged into > > cifs-2.6.git for-next > > > > On Tue, Oct 20, 2020 at 4:43 PM Pavel Shilovsky <piastryyy@xxxxxxxxx> wrote: > > > > > > Any ideas about stable? esize mount option went into kernel v5.4. > > > > > > -- > > > Best regards, > > > Pavel Shilovsky > > > > > > пн, 19 окт. 2020 г. в 09:48, Pavel Shilovsky <piastryyy@xxxxxxxxx>: > > > > > > > > Thanks for the patch! > > > > > > > > Reviewed-by: Pavel Shilovsky <pshilov@xxxxxxxxxxxxx> > > > > > > > > -- > > > > Best regards, > > > > Pavel Shilovsky > > > > > > > > пн, 19 окт. 2020 г. в 00:28, Rohith Surabattula <rohiths.msft@xxxxxxxxx>: > > > > > > > > > > Hi Pavel, > > > > > > > > > > Corrected the patch with the suggested changes. > > > > > Attached the patch. > > > > > > > > > > Regards, > > > > > Rohith > > > > > > > > > > On Thu, Oct 15, 2020 at 9:39 PM Pavel Shilovsky <piastryyy@xxxxxxxxx> wrote: > > > > > > > > > > > > In receive_encrypted_standard(), server->total_read is set to the > > > > > > total packet length before calling decrypt_raw_data(). The total > > > > > > packet length includes the transform header but the idea of updating > > > > > > server->total_read after decryption is to set it to a decrypted packet > > > > > > size without the transform header (see memmove in decrypt_raw_data). > > > > > > > > > > > > We would probably need to backport the patch to stable trees, so, I > > > > > > would try to make the smallest possible change in terms of scope - > > > > > > meaning just fixing the read codepath with esize mount option turned > > > > > > on. > > > > > > > > > > > > -- > > > > > > Best regards, > > > > > > Pavel Shilovsky > > > > > > > > > > > > ср, 14 окт. 2020 г. в 20:21, Rohith Surabattula <rohiths.msft@xxxxxxxxx>: > > > > > > > > > > > > > > Hi Pavel, > > > > > > > > > > > > > > In receive_encrypted_standard function also, server->total_read is > > > > > > > updated properly before calling decrypt_raw_data. So, no need to > > > > > > > update the same field again. > > > > > > > > > > > > > > I have checked all instances where decrypt_raw_data is used and didn’t > > > > > > > find any issue. > > > > > > > > > > > > > > Regards, > > > > > > > Rohith > > > > > > > > > > > > > > On Thu, Oct 15, 2020 at 4:18 AM Pavel Shilovsky <piastryyy@xxxxxxxxx> wrote: > > > > > > > > > > > > > > > > Hi Rohith, > > > > > > > > > > > > > > > > Thanks for catching the problem and proposing the patch! > > > > > > > > > > > > > > > > I think there is a problem with just removing server->total_read > > > > > > > > updates inside decrypt_raw_data(): > > > > > > > > > > > > > > > > The same function is used in receive_encrypted_standard() which then > > > > > > > > calls cifs_handle_standard(). The latter uses server->total_read in at > > > > > > > > least two places: in server->ops->check_message and cifs_dump_mem(). > > > > > > > > > > > > > > > > There may be other places in the code that assume server->total_read > > > > > > > > to be correct. I would avoid simply removing this in all code paths > > > > > > > > and would rather make a more specific fix for the offloaded reads. > > > > > > > > > > > > > > > > -- > > > > > > > > Best regards, > > > > > > > > Pavel Shilovsky > > > > > > > > > > > > > > > > чт, 8 окт. 2020 г. в 13:36, Steve French <smfrench@xxxxxxxxx>: > > > > > > > > > > > > > > > > > > Fixed up 2 small checkpatch warnings and merged into cifs-2.6.git for-next > > > > > > > > > > > > > > > > > > On Thu, Oct 8, 2020 at 9:40 AM Rohith Surabattula > > > > > > > > > <rohiths.msft@xxxxxxxxx> wrote: > > > > > > > > > > > > > > > > > > > > Hi All, > > > > > > > > > > > > > > > > > > > > With the "esize" mount option, I observed data corruption and cifs > > > > > > > > > > reconnects during performance tests. > > > > > > > > > > > > > > > > > > > > TCP server info field server->total_read is modified parallely by > > > > > > > > > > demultiplex thread and decrypt offload worker thread. server->total_read > > > > > > > > > > is used in calculation to discard the remaining data of PDU which is > > > > > > > > > > not read into memory. > > > > > > > > > > > > > > > > > > > > Because of parallel modification, “server->total_read” value got > > > > > > > > > > corrupted and instead of discarding the remaining data, it discarded > > > > > > > > > > some valid data from the next PDU. > > > > > > > > > > > > > > > > > > > > server->total_read field is already updated properly during read from > > > > > > > > > > socket. So, no need to update the same field again after decryption. > > > > > > > > > > > > > > > > > > > > Regards, > > > > > > > > > > Rohith > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > -- > > > > > > > > > Thanks, > > > > > > > > > > > > > > > > > > Steve > > > > > > > > -- > > Thanks, > > > > Steve -- Thanks, Steve
