* Fwd: [PATCH][SMB client] send ChannelSequence number after reconnect
[not found] <CAH2r5mucC=YxgaQV5nAPCfduAmjyEyxYw+XdToOwELezqe=e0g@mail.gmail.com>
@ 2023-08-25 4:51 ` Steve French
2023-08-25 4:55 ` Steve French
2023-08-30 13:56 ` Shyam Prasad N
1 sibling, 1 reply; 6+ messages in thread
From: Steve French @ 2023-08-25 4:51 UTC (permalink / raw)
To: CIFS, samba-technical
[-- Attachment #1: Type: text/plain, Size: 656 bytes --]
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
--
Thanks,
Steve
[-- Attachment #2: 0001-SMB3-send-channel-sequence-number-in-SMB3-requests-a.patch --]
[-- Type: application/x-patch, Size: 4677 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH][SMB client] send ChannelSequence number after reconnect
2023-08-25 4:51 ` Fwd: [PATCH][SMB client] send ChannelSequence number after reconnect Steve French
@ 2023-08-25 4:55 ` Steve French
0 siblings, 0 replies; 6+ messages in thread
From: Steve French @ 2023-08-25 4:55 UTC (permalink / raw)
To: CIFS, samba-technical, Tom Talpey
> How have you tested this? Seems like some significantly random
> connection fault injection is needed, when doing active multichannel
> load testing to a Windows server sku.
I tested this to Windows by pausing the Windows server VM in HyperV to
force timeout, then resumed the VM to see reconnect (I also tried this
with the Windows client to see how it updated the ChannelSequence).
I also tried this to Samba by dropping the network interface and
letting the requests time out ("ifconfig lo down") and then doing
"ifconfig lo up" and letting it reconnect
On Thu, Aug 24, 2023 at 11:51 PM Steve French <smfrench@gmail.com> 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
>
>
> --
> Thanks,
>
> Steve
--
Thanks,
Steve
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH][SMB client] send ChannelSequence number after reconnect
[not found] <CAH2r5mucC=YxgaQV5nAPCfduAmjyEyxYw+XdToOwELezqe=e0g@mail.gmail.com>
2023-08-25 4:51 ` Fwd: [PATCH][SMB client] send ChannelSequence number after reconnect Steve French
@ 2023-08-30 13:56 ` Shyam Prasad N
[not found] ` <CAH2r5mtRJtNiN128kJTSknv_F4Q6uPDsETcKH7Pjkfk0Fco6zg@mail.gmail.com>
1 sibling, 1 reply; 6+ messages in thread
From: Shyam Prasad N @ 2023-08-30 13:56 UTC (permalink / raw)
To: Steve French; +Cc: CIFS, Tom Talpey, Bharath S M, Aurélien Aptel
On Fri, Aug 25, 2023 at 10:09 AM Steve French <smfrench@gmail.com> 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Fwd: [PATCH][SMB client] send ChannelSequence number after reconnect
[not found] ` <CAH2r5mtRJtNiN128kJTSknv_F4Q6uPDsETcKH7Pjkfk0Fco6zg@mail.gmail.com>
@ 2023-08-30 14:02 ` Steve French
2023-08-30 14:06 ` Steve French
1 sibling, 0 replies; 6+ messages in thread
From: Steve French @ 2023-08-30 14:02 UTC (permalink / raw)
To: CIFS
---------- Forwarded message ---------
From: Steve French <smfrench@gmail.com>
Date: Wed, Aug 30, 2023 at 9:02 AM
Subject: Re: [PATCH][SMB client] send ChannelSequence number after reconnect
To: Shyam Prasad N <nspmangalore@gmail.com>
Cc: CIFS <linux-cifs@vger.kernel.org>, Tom Talpey <tom@talpey.com>,
Bharath S M <bharathsm@microsoft.com>, Aurélien Aptel
<aaptel@samba.org>
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@gmail.com> wrote:
>
> On Fri, Aug 25, 2023 at 10:09 AM Steve French <smfrench@gmail.com> 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH][SMB client] send ChannelSequence number after reconnect
[not found] ` <CAH2r5mtRJtNiN128kJTSknv_F4Q6uPDsETcKH7Pjkfk0Fco6zg@mail.gmail.com>
2023-08-30 14:02 ` Fwd: " Steve French
@ 2023-08-30 14:06 ` Steve French
2023-09-17 15:59 ` Shyam Prasad N
1 sibling, 1 reply; 6+ messages in thread
From: Steve French @ 2023-08-30 14:06 UTC (permalink / raw)
To: Shyam Prasad N; +Cc: CIFS, Tom Talpey, Bharath S M, Aurélien Aptel
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@gmail.com> 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@gmail.com> wrote:
>>
>> On Fri, Aug 25, 2023 at 10:09 AM Steve French <smfrench@gmail.com> 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH][SMB client] send ChannelSequence number after reconnect
2023-08-30 14:06 ` Steve French
@ 2023-09-17 15:59 ` Shyam Prasad N
0 siblings, 0 replies; 6+ messages in thread
From: Shyam Prasad N @ 2023-09-17 15:59 UTC (permalink / raw)
To: Steve French; +Cc: CIFS, Tom Talpey, Bharath S M, Aurélien Aptel
On Wed, Aug 30, 2023 at 7:36 PM Steve French <smfrench@gmail.com> 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@gmail.com> 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@gmail.com> wrote:
> >>
> >> On Fri, Aug 25, 2023 at 10:09 AM Steve French <smfrench@gmail.com> 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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-09-17 16:00 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <CAH2r5mucC=YxgaQV5nAPCfduAmjyEyxYw+XdToOwELezqe=e0g@mail.gmail.com>
2023-08-25 4:51 ` Fwd: [PATCH][SMB client] send ChannelSequence number after reconnect Steve French
2023-08-25 4:55 ` Steve French
2023-08-30 13:56 ` Shyam Prasad N
[not found] ` <CAH2r5mtRJtNiN128kJTSknv_F4Q6uPDsETcKH7Pjkfk0Fco6zg@mail.gmail.com>
2023-08-30 14:02 ` Fwd: " Steve French
2023-08-30 14:06 ` Steve French
2023-09-17 15:59 ` Shyam Prasad N
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).