On Wed, Aug 30, 2023 at 7:36 PM Steve French <smfrench@xxxxxxxxx> wrote: > > There is also an smbtorture case in the samba test suite for this > (replay.c) we can take a look at that. > > On the channel sequence number question - there is interesting > additional information (although probably not indicating a client > change) on channel sequence overflow at source3/smbd/smb2_server.c > line 2944ff > > On Wed, Aug 30, 2023 at 9:02 AM Steve French <smfrench@xxxxxxxxx> wrote: > > > > we could do a follow on with your suggestion, but seems like without the replay flag would just look like a new write to the same offset (which should take precedence over an older queued write to the same offset that has a lower sequence number) > > > > I will code up a follow on patch to do replay operation patch > > > > On Wed, Aug 30, 2023 at 8:56 AM Shyam Prasad N <nspmangalore@xxxxxxxxx> wrote: > >> > >> On Fri, Aug 25, 2023 at 10:09 AM Steve French <smfrench@xxxxxxxxx> wrote: > >> > > >> > The ChannelSequence field in the SMB3 header is supposed to be > >> > increased after reconnect to allow the server to distinguish > >> > requests from before and after the reconnect. We had always > >> > been setting it to zero. There are cases where incrementing > >> > ChannelSequence on requests after network reconnects can reduce > >> > the chance of data corruptions. > >> > > >> > See MS-SMB2 3.2.4.1 and 3.2.7.1 > >> > > >> > Note that (as Tom Talpey pointed out) a macro "CIFS_SERVER_IS_CHAN" used by this patch is confusing (has a confusing name) since multichannel is not supported for older dialects like CIFS. I will fix that macro name in a followon patch. > >> > > >> > -- > >> > Thanks, > >> > > >> > Steve > >> > >> Theoretically seems okay. Although MS-SMB2 says that replay requests > >> need to be indicated as replay in the header, which we are not doing > >> currently. > >> I don't know what maybe a side effect of not sending that could be. > >> Will this patch without that make things worse? > >> > >> -- > >> Regards, > >> Shyam > > > > > > > > -- > > Thanks, > > > > Steve > > > > -- > Thanks, > > Steve I was doing some more reading up on this today. And it does look like our ChannelSequence operations should be okay. The spec recommends that each session for a given server maintains it's ChannelSequence. We use the channel_sequence_num on the primary channel. So we are good that way. However, a few other things that I observed about what the spec says about how the server implements the ChannelSequence (3.3.5.2.10 Verifying the Channel Sequence Number): 1. The ChannelSequence need not be incremented on each reconnection of any channel. It needs to be incremented only when all the channels are down. I can create a follow on patch for this. 2. If the server sees that the ChannelSequence for a request is not the latest, then it may return STATUS_FILE_NOT_AVAILABLE. The client should be prepared to handle this by replaying the request again. 3. If we carelessly increment the channel_sequence_number, more requests can start failing with STATUS_FILE_NOT_AVAILABLE. So #1 and #2 become more important to implement. 4. >> which should take precedence over an older queued write to the same offset that has a lower sequence number I'm not sure that this is the correct assumption. Based on the spec, it seems to me like ordering of the old queued writes need not be preserved if the replay bit is not set. If the replay bit is set, it looks like conflicting writes can result in the later one being returned STATUS_FILE_NOT_AVAILABLE. -- Regards, Shyam
