[PATCH 1/3] ksmbd: set v2 lease version on lease upgrade

[PATCH 1/3] ksmbd: set v2 lease version on lease upgrade

[Namjae Jeon]

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(+)

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)
-- 
2.25.1

[PATCH 2/3] ksmbd: fix potential circular locking issue in smb2_set_ea()

[Namjae Jeon]

smb2_set_ea() can be called in parent inode lock range.
So add get_write argument to smb2_set_ea() not to call nested
mnt_want_write().

Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
---
 fs/smb/server/smb2pdu.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/fs/smb/server/smb2pdu.c b/fs/smb/server/smb2pdu.c
index 652ab429bf2e..a2f729675183 100644
--- a/fs/smb/server/smb2pdu.c
+++ b/fs/smb/server/smb2pdu.c
@@ -2311,11 +2311,12 @@ static noinline int create_smb2_pipe(struct ksmbd_work *work)
  * @eabuf:	set info command buffer
  * @buf_len:	set info command buffer length
  * @path:	dentry path for get ea
+ * @get_write:	get write access to a mount
  *
  * Return:	0 on success, otherwise error
  */
 static int smb2_set_ea(struct smb2_ea_info *eabuf, unsigned int buf_len,
-		       const struct path *path)
+		       const struct path *path, bool get_write)
 {
 	struct mnt_idmap *idmap = mnt_idmap(path->mnt);
 	char *attr_name = NULL, *value;
@@ -3003,7 +3004,7 @@ int smb2_open(struct ksmbd_work *work)
 
 			rc = smb2_set_ea(&ea_buf->ea,
 					 le32_to_cpu(ea_buf->ccontext.DataLength),
-					 &path);
+					 &path, false);
 			if (rc == -EOPNOTSUPP)
 				rc = 0;
 			else if (rc)
@@ -5992,7 +5993,7 @@ static int smb2_set_info_file(struct ksmbd_work *work, struct ksmbd_file *fp,
 			return -EINVAL;
 
 		return smb2_set_ea((struct smb2_ea_info *)req->Buffer,
-				   buf_len, &fp->filp->f_path);
+				   buf_len, &fp->filp->f_path, true);
 	}
 	case FILE_POSITION_INFORMATION:
 	{
-- 
2.25.1

[PATCH 3/3] ksmbd: don't increment epoch if current state and request state are same

[Namjae Jeon]

If existing lease state and request state are same, don't increment
epoch in create context.

Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
---
 fs/smb/server/oplock.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/fs/smb/server/oplock.c b/fs/smb/server/oplock.c
index 9a19d8b06c8c..77f20d34ed9a 100644
--- a/fs/smb/server/oplock.c
+++ b/fs/smb/server/oplock.c
@@ -105,7 +105,7 @@ static int alloc_lease(struct oplock_info *opinfo, struct lease_ctx_info *lctx)
 	lease->is_dir = lctx->is_dir;
 	memcpy(lease->parent_lease_key, lctx->parent_lease_key, SMB2_LEASE_KEY_SIZE);
 	lease->version = lctx->version;
-	lease->epoch = le16_to_cpu(lctx->epoch);
+	lease->epoch = le16_to_cpu(lctx->epoch) + 1;
 	INIT_LIST_HEAD(&opinfo->lease_entry);
 	opinfo->o_lease = lease;
 
@@ -541,6 +541,9 @@ static struct oplock_info *same_client_has_lease(struct ksmbd_inode *ci,
 				continue;
 			}
 
+			if (lctx->req_state != lease->state)
+				lease->epoch++;
+
 			/* upgrading lease */
 			if ((atomic_read(&ci->op_count) +
 			     atomic_read(&ci->sop_count)) == 1) {
@@ -1035,7 +1038,7 @@ static void copy_lease(struct oplock_info *op1, struct oplock_info *op2)
 	       SMB2_LEASE_KEY_SIZE);
 	lease2->duration = lease1->duration;
 	lease2->flags = lease1->flags;
-	lease2->epoch = lease1->epoch++;
+	lease2->epoch = lease1->epoch;
 	lease2->version = lease1->version;
 }
 
@@ -1448,7 +1451,7 @@ void create_lease_buf(u8 *rbuf, struct lease *lease)
 		memcpy(buf->lcontext.LeaseKey, lease->lease_key,
 		       SMB2_LEASE_KEY_SIZE);
 		buf->lcontext.LeaseFlags = lease->flags;
-		buf->lcontext.Epoch = cpu_to_le16(++lease->epoch);
+		buf->lcontext.Epoch = cpu_to_le16(lease->epoch);
 		buf->lcontext.LeaseState = lease->state;
 		memcpy(buf->lcontext.ParentLeaseKey, lease->parent_lease_key,
 		       SMB2_LEASE_KEY_SIZE);
-- 
2.25.1

Re: [PATCH 1/3] ksmbd: set v2 lease version on lease upgrade

[Tom Talpey]

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)

Re: [PATCH 1/3] ksmbd: set v2 lease version on lease upgrade

[Namjae Jeon]

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)
>

Re: [PATCH 1/3] ksmbd: set v2 lease version on lease upgrade

[Tom Talpey]

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)
>>
> 

Re: [PATCH 1/3] ksmbd: set v2 lease version on lease upgrade

[Namjae Jeon]

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)
>>>
>>
>