| Message ID | 20231216122938.4511-1-linkinjeon@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | [1/3] ksmbd: set v2 lease version on lease upgrade | expand |
On 12/16/2023 7:29 AM, Namjae Jeon wrote: > If file opened with v2 lease is upgraded with v1 lease, smb server Can you elaborate on this scenario? Lease v1 is SMB2, while v2 is SMB3. So how can the same client expect to do both? And how can the server support that? MS-SMB2: > 3.2.4.3.8 Requesting a Lease on a File or a Directory ... > If Connection.Dialect belongs to the SMB 3.x dialect family, the client MUST attach an > SMB2_CREATE_REQUEST_LEASE_V2 create context to the request. The create context MUST be > formatted as described in section 2.2.13.2.10 with the following values ... > If Connection.Dialect is equal to "2.1", the client MUST attach an SMB2_CREATE_REQUEST_LEASE > create context to the request. The create context MUST be formatted as described in section > 2.2.13.2.8, with the LeaseState value provided by the application Tom. > should response v2 lease create context to client. > This patch fix smb2.lease.v2_epoch2 test failure. > > Signed-off-by: Namjae Jeon <linkinjeon@kernel.org> > --- > fs/smb/server/oplock.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/fs/smb/server/oplock.c b/fs/smb/server/oplock.c > index 562b180459a1..9a19d8b06c8c 100644 > --- a/fs/smb/server/oplock.c > +++ b/fs/smb/server/oplock.c > @@ -1036,6 +1036,7 @@ static void copy_lease(struct oplock_info *op1, struct oplock_info *op2) > lease2->duration = lease1->duration; > lease2->flags = lease1->flags; > lease2->epoch = lease1->epoch++; > + lease2->version = lease1->version; > } > > static int add_lease_global_list(struct oplock_info *opinfo)
2023-12-16 22:37 GMT+09:00, Tom Talpey <tom@talpey.com>: > On 12/16/2023 7:29 AM, Namjae Jeon wrote: >> If file opened with v2 lease is upgraded with v1 lease, smb server > > Can you elaborate on this scenario? Lease v1 is SMB2, while v2 is SMB3. > So how can the same client expect to do both? And how can the server > support that? This patch is to fix smb2.lease.epoch2 testcase in smbtorture. This test case assumes the following scenario: 1. smb2 create with v2 lease(R, LEASE1 key) 2. smb server return smb2 create response with v2 lease context(R, LEASE1 key, epoch + 1) 3. smb2 create with v1 lease(RH, LEASE1 key) 4. smb server return smb2 create response with v2 lease context(RH, LEASE1 key, epoch + 2) i.e. If same client(same lease key) try to open a file that is being opened with v2 lease with v1 lease, smb server should return v2 lease create context to client. > > MS-SMB2: > >> 3.2.4.3.8 Requesting a Lease on a File or a Directory > ... >> If Connection.Dialect belongs to the SMB 3.x dialect family, the client >> MUST attach an >> SMB2_CREATE_REQUEST_LEASE_V2 create context to the request. The create >> context MUST be >> formatted as described in section 2.2.13.2.10 with the following values > ... >> If Connection.Dialect is equal to "2.1", the client MUST attach an >> SMB2_CREATE_REQUEST_LEASE >> create context to the request. The create context MUST be formatted as >> described in section >> 2.2.13.2.8, with the LeaseState value provided by the application > > > > Tom. > >> should response v2 lease create context to client. >> This patch fix smb2.lease.v2_epoch2 test failure. >> >> Signed-off-by: Namjae Jeon <linkinjeon@kernel.org> >> --- >> fs/smb/server/oplock.c | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/fs/smb/server/oplock.c b/fs/smb/server/oplock.c >> index 562b180459a1..9a19d8b06c8c 100644 >> --- a/fs/smb/server/oplock.c >> +++ b/fs/smb/server/oplock.c >> @@ -1036,6 +1036,7 @@ static void copy_lease(struct oplock_info *op1, >> struct oplock_info *op2) >> lease2->duration = lease1->duration; >> lease2->flags = lease1->flags; >> lease2->epoch = lease1->epoch++; >> + lease2->version = lease1->version; >> } >> >> static int add_lease_global_list(struct oplock_info *opinfo) >
On 12/17/2023 6:58 PM, Namjae Jeon wrote: > 2023-12-16 22:37 GMT+09:00, Tom Talpey <tom@talpey.com>: >> On 12/16/2023 7:29 AM, Namjae Jeon wrote: >>> If file opened with v2 lease is upgraded with v1 lease, smb server >> >> Can you elaborate on this scenario? Lease v1 is SMB2, while v2 is SMB3. >> So how can the same client expect to do both? And how can the server >> support that? > This patch is to fix smb2.lease.epoch2 testcase in smbtorture. > This test case assumes the following scenario: > 1. smb2 create with v2 lease(R, LEASE1 key) > 2. smb server return smb2 create response with v2 lease context(R, > LEASE1 key, epoch + 1) > 3. smb2 create with v1 lease(RH, LEASE1 key) > 4. smb server return smb2 create response with v2 lease context(RH, > LEASE1 key, epoch + 2) Oh, this makes my head hurt. The protocol requires the client to never mix REQUEST_LEASE and REQUEST_LEASE_V2 on a connection (as I quoted below). BUT! There is no requirement for the server to enforce this, and in fact a requirement instead that it merge v1 and v2 leases, where possible. This kinda sorta makes sense for SMB2.1 and SMB3+ interop. But it opens up this ambiguity! Practically speaking, I don't think this should ever happen, given a conformantly written client. But the patch does appear to match the required server behavior. So, reluctantly... Acked-by: Tom Talpey <tom@talpey.com> > > i.e. If same client(same lease key) try to open a file that is being > opened with v2 lease with v1 lease, smb server should return v2 lease > create context to client. > >> >> MS-SMB2: >> >>> 3.2.4.3.8 Requesting a Lease on a File or a Directory >> ... >>> If Connection.Dialect belongs to the SMB 3.x dialect family, the client >>> MUST attach an >>> SMB2_CREATE_REQUEST_LEASE_V2 create context to the request. The create >>> context MUST be >>> formatted as described in section 2.2.13.2.10 with the following values >> ... >>> If Connection.Dialect is equal to "2.1", the client MUST attach an >>> SMB2_CREATE_REQUEST_LEASE >>> create context to the request. The create context MUST be formatted as >>> described in section >>> 2.2.13.2.8, with the LeaseState value provided by the application >> >> >> >> Tom. >> >>> should response v2 lease create context to client. >>> This patch fix smb2.lease.v2_epoch2 test failure. >>> >>> Signed-off-by: Namjae Jeon <linkinjeon@kernel.org> >>> --- >>> fs/smb/server/oplock.c | 1 + >>> 1 file changed, 1 insertion(+) >>> >>> diff --git a/fs/smb/server/oplock.c b/fs/smb/server/oplock.c >>> index 562b180459a1..9a19d8b06c8c 100644 >>> --- a/fs/smb/server/oplock.c >>> +++ b/fs/smb/server/oplock.c >>> @@ -1036,6 +1036,7 @@ static void copy_lease(struct oplock_info *op1, >>> struct oplock_info *op2) >>> lease2->duration = lease1->duration; >>> lease2->flags = lease1->flags; >>> lease2->epoch = lease1->epoch++; >>> + lease2->version = lease1->version; >>> } >>> >>> static int add_lease_global_list(struct oplock_info *opinfo) >> >
2023-12-18 10:33 GMT+09:00, Tom Talpey <tom@talpey.com>: > On 12/17/2023 6:58 PM, Namjae Jeon wrote: >> 2023-12-16 22:37 GMT+09:00, Tom Talpey <tom@talpey.com>: >>> On 12/16/2023 7:29 AM, Namjae Jeon wrote: >>>> If file opened with v2 lease is upgraded with v1 lease, smb server >>> >>> Can you elaborate on this scenario? Lease v1 is SMB2, while v2 is SMB3. >>> So how can the same client expect to do both? And how can the server >>> support that? >> This patch is to fix smb2.lease.epoch2 testcase in smbtorture. >> This test case assumes the following scenario: >> 1. smb2 create with v2 lease(R, LEASE1 key) >> 2. smb server return smb2 create response with v2 lease context(R, >> LEASE1 key, epoch + 1) >> 3. smb2 create with v1 lease(RH, LEASE1 key) >> 4. smb server return smb2 create response with v2 lease context(RH, >> LEASE1 key, epoch + 2) > > Oh, this makes my head hurt. The protocol requires the client to never > mix REQUEST_LEASE and REQUEST_LEASE_V2 on a connection (as I quoted > below). > > BUT! There is no requirement for the server to enforce this, and in fact > a requirement instead that it merge v1 and v2 leases, where possible. > This kinda sorta makes sense for SMB2.1 and SMB3+ interop. But it opens > up this ambiguity! > > Practically speaking, I don't think this should ever happen, given a > conformantly written client. But the patch does appear to match the > required server behavior. > > So, reluctantly... > > Acked-by: Tom Talpey <tom@talpey.com> Thanks for your ack:) and I will update the patch description also. > > >> >> i.e. If same client(same lease key) try to open a file that is being >> opened with v2 lease with v1 lease, smb server should return v2 lease >> create context to client. >> >>> >>> MS-SMB2: >>> >>>> 3.2.4.3.8 Requesting a Lease on a File or a Directory >>> ... >>>> If Connection.Dialect belongs to the SMB 3.x dialect family, the client >>>> MUST attach an >>>> SMB2_CREATE_REQUEST_LEASE_V2 create context to the request. The create >>>> context MUST be >>>> formatted as described in section 2.2.13.2.10 with the following values >>> ... >>>> If Connection.Dialect is equal to "2.1", the client MUST attach an >>>> SMB2_CREATE_REQUEST_LEASE >>>> create context to the request. The create context MUST be formatted as >>>> described in section >>>> 2.2.13.2.8, with the LeaseState value provided by the application >>> >>> >>> >>> Tom. >>> >>>> should response v2 lease create context to client. >>>> This patch fix smb2.lease.v2_epoch2 test failure. >>>> >>>> Signed-off-by: Namjae Jeon <linkinjeon@kernel.org> >>>> --- >>>> fs/smb/server/oplock.c | 1 + >>>> 1 file changed, 1 insertion(+) >>>> >>>> diff --git a/fs/smb/server/oplock.c b/fs/smb/server/oplock.c >>>> index 562b180459a1..9a19d8b06c8c 100644 >>>> --- a/fs/smb/server/oplock.c >>>> +++ b/fs/smb/server/oplock.c >>>> @@ -1036,6 +1036,7 @@ static void copy_lease(struct oplock_info *op1, >>>> struct oplock_info *op2) >>>> lease2->duration = lease1->duration; >>>> lease2->flags = lease1->flags; >>>> lease2->epoch = lease1->epoch++; >>>> + lease2->version = lease1->version; >>>> } >>>> >>>> static int add_lease_global_list(struct oplock_info *opinfo) >>> >> >
diff --git a/fs/smb/server/oplock.c b/fs/smb/server/oplock.c index 562b180459a1..9a19d8b06c8c 100644 --- a/fs/smb/server/oplock.c +++ b/fs/smb/server/oplock.c @@ -1036,6 +1036,7 @@ static void copy_lease(struct oplock_info *op1, struct oplock_info *op2) lease2->duration = lease1->duration; lease2->flags = lease1->flags; lease2->epoch = lease1->epoch++; + lease2->version = lease1->version; } static int add_lease_global_list(struct oplock_info *opinfo)
If file opened with v2 lease is upgraded with v1 lease, smb server should response v2 lease create context to client. This patch fix smb2.lease.v2_epoch2 test failure. Signed-off-by: Namjae Jeon <linkinjeon@kernel.org> --- fs/smb/server/oplock.c | 1 + 1 file changed, 1 insertion(+)