[PATCH] ksmbd: improve credits management

[PATCH] ksmbd: improve credits management

[Hyunchul Lee]

* Requests except READ, WRITE, IOCTL, INFO, QUERY
DIRECOTRY, CANCEL must consume one credit.
* If client's granted credits are insufficient,
refuse to handle requests.
* Windows server 2016 or later grant up to 8192
credits to clients at once.
* For an asynchronous operation, grant credits
for an interim response and 0 credit for the
final response.

Signed-off-by: Hyunchul Lee <hyc.lee@gmail.com>
---
 fs/ksmbd/smb2misc.c | 38 ++++++++++++++------
 fs/ksmbd/smb2ops.c  |  4 +++
 fs/ksmbd/smb2pdu.c  | 86 ++++++++++++++++++++-------------------------
 3 files changed, 71 insertions(+), 57 deletions(-)

diff --git a/fs/ksmbd/smb2misc.c b/fs/ksmbd/smb2misc.c
index 9aa46bb3e10d..cbbea6937816 100644
--- a/fs/ksmbd/smb2misc.c
+++ b/fs/ksmbd/smb2misc.c
@@ -287,11 +287,13 @@ static inline int smb2_ioctl_resp_len(struct smb2_ioctl_req *h)
 		le32_to_cpu(h->MaxOutputResponse);
 }
 
-static int smb2_validate_credit_charge(struct smb2_hdr *hdr)
+static int smb2_validate_credit_charge(struct ksmbd_conn *conn,
+				       struct smb2_hdr *hdr)
 {
-	int req_len = 0, expect_resp_len = 0, calc_credit_num, max_len;
-	int credit_charge = le16_to_cpu(hdr->CreditCharge);
+	unsigned int req_len = 0, expect_resp_len = 0, calc_credit_num, max_len;
+	unsigned short credit_charge = le16_to_cpu(hdr->CreditCharge);
 	void *__hdr = hdr;
+	int ret;
 
 	switch (hdr->Command) {
 	case SMB2_QUERY_INFO:
@@ -313,21 +315,37 @@ static int smb2_validate_credit_charge(struct smb2_hdr *hdr)
 		req_len = smb2_ioctl_req_len(__hdr);
 		expect_resp_len = smb2_ioctl_resp_len(__hdr);
 		break;
-	default:
+	case SMB2_CANCEL:
 		return 0;
+	default:
+		req_len = 1;
+		break;
 	}
 
-	credit_charge = max(1, credit_charge);
-	max_len = max(req_len, expect_resp_len);
+	credit_charge = max_t(unsigned short, credit_charge, 1);
+	max_len = max_t(unsigned int, req_len, expect_resp_len);
 	calc_credit_num = DIV_ROUND_UP(max_len, SMB2_MAX_BUFFER_SIZE);
 
 	if (credit_charge < calc_credit_num) {
-		pr_err("Insufficient credit charge, given: %d, needed: %d\n",
-		       credit_charge, calc_credit_num);
+		ksmbd_debug(SMB, "Insufficient credit charge, given: %d, needed: %d\n",
+			    credit_charge, calc_credit_num);
+		return 1;
+	} else if (credit_charge > conn->max_credits) {
+		ksmbd_debug(SMB, "Too large credit charge: %d\n", credit_charge);
 		return 1;
 	}
 
-	return 0;
+	spin_lock(&conn->credits_lock);
+	if (credit_charge <= conn->total_credits) {
+		conn->total_credits -= credit_charge;
+		ret = 0;
+	} else {
+		ksmbd_debug(SMB, "Insufficient credits granted, given: %u, granted: %u\n",
+			    credit_charge, conn->total_credits);
+		ret = 1;
+	}
+	spin_unlock(&conn->credits_lock);
+	return ret;
 }
 
 int ksmbd_smb2_check_message(struct ksmbd_work *work)
@@ -386,7 +404,7 @@ int ksmbd_smb2_check_message(struct ksmbd_work *work)
 	}
 
 	if ((work->conn->vals->capabilities & SMB2_GLOBAL_CAP_LARGE_MTU) &&
-	    smb2_validate_credit_charge(hdr)) {
+	    smb2_validate_credit_charge(work->conn, hdr)) {
 		work->conn->ops->set_rsp_status(work, STATUS_INVALID_PARAMETER);
 		return 1;
 	}
diff --git a/fs/ksmbd/smb2ops.c b/fs/ksmbd/smb2ops.c
index 197473871aa4..d7919f95be40 100644
--- a/fs/ksmbd/smb2ops.c
+++ b/fs/ksmbd/smb2ops.c
@@ -203,6 +203,7 @@ void init_smb2_1_server(struct ksmbd_conn *conn)
 	conn->ops = &smb2_0_server_ops;
 	conn->cmds = smb2_0_server_cmds;
 	conn->max_cmds = ARRAY_SIZE(smb2_0_server_cmds);
+	conn->total_credits = 1;
 	conn->max_credits = SMB2_MAX_CREDITS;
 	conn->signing_algorithm = SIGNING_ALG_HMAC_SHA256;
 
@@ -221,6 +222,7 @@ void init_smb3_0_server(struct ksmbd_conn *conn)
 	conn->ops = &smb3_0_server_ops;
 	conn->cmds = smb2_0_server_cmds;
 	conn->max_cmds = ARRAY_SIZE(smb2_0_server_cmds);
+	conn->total_credits = 1;
 	conn->max_credits = SMB2_MAX_CREDITS;
 	conn->signing_algorithm = SIGNING_ALG_AES_CMAC;
 
@@ -246,6 +248,7 @@ void init_smb3_02_server(struct ksmbd_conn *conn)
 	conn->ops = &smb3_0_server_ops;
 	conn->cmds = smb2_0_server_cmds;
 	conn->max_cmds = ARRAY_SIZE(smb2_0_server_cmds);
+	conn->total_credits = 1;
 	conn->max_credits = SMB2_MAX_CREDITS;
 	conn->signing_algorithm = SIGNING_ALG_AES_CMAC;
 
@@ -271,6 +274,7 @@ int init_smb3_11_server(struct ksmbd_conn *conn)
 	conn->ops = &smb3_11_server_ops;
 	conn->cmds = smb2_0_server_cmds;
 	conn->max_cmds = ARRAY_SIZE(smb2_0_server_cmds);
+	conn->total_credits = 1;
 	conn->max_credits = SMB2_MAX_CREDITS;
 	conn->signing_algorithm = SIGNING_ALG_AES_CMAC;
 
diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
index dcf907738610..6df0b0759d3f 100644
--- a/fs/ksmbd/smb2pdu.c
+++ b/fs/ksmbd/smb2pdu.c
@@ -295,22 +295,6 @@ int init_smb2_neg_rsp(struct ksmbd_work *work)
 	return 0;
 }
 
-static int smb2_consume_credit_charge(struct ksmbd_work *work,
-				      unsigned short credit_charge)
-{
-	struct ksmbd_conn *conn = work->conn;
-	unsigned int rsp_credits = 1;
-
-	if (!conn->total_credits)
-		return 0;
-
-	if (credit_charge > 0)
-		rsp_credits = credit_charge;
-
-	conn->total_credits -= rsp_credits;
-	return rsp_credits;
-}
-
 /**
  * smb2_set_rsp_credits() - set number of credits in response buffer
  * @work:	smb work containing smb response buffer
@@ -320,48 +304,51 @@ int smb2_set_rsp_credits(struct ksmbd_work *work)
 	struct smb2_hdr *req_hdr = ksmbd_req_buf_next(work);
 	struct smb2_hdr *hdr = ksmbd_resp_buf_next(work);
 	struct ksmbd_conn *conn = work->conn;
-	unsigned short credits_requested = le16_to_cpu(req_hdr->CreditRequest);
-	unsigned short credit_charge = 1, credits_granted = 0;
-	unsigned short aux_max, aux_credits, min_credits;
-	int rsp_credit_charge;
+	unsigned short credits_requested;
+	unsigned short credit_charge, credits_granted = 0;
+	unsigned short aux_max, aux_credits;
 
-	if (hdr->Command == SMB2_CANCEL)
-		goto out;
+	if (work->send_no_response)
+		return 0;
 
-	/* get default minimum credits by shifting maximum credits by 4 */
-	min_credits = conn->max_credits >> 4;
+	hdr->CreditCharge = req_hdr->CreditCharge;
 
-	if (conn->total_credits >= conn->max_credits) {
+	if (conn->total_credits > conn->max_credits) {
+		hdr->CreditRequest = 0;
 		pr_err("Total credits overflow: %d\n", conn->total_credits);
-		conn->total_credits = min_credits;
-	}
-
-	rsp_credit_charge =
-		smb2_consume_credit_charge(work, le16_to_cpu(req_hdr->CreditCharge));
-	if (rsp_credit_charge < 0)
 		return -EINVAL;
+	}
 
-	hdr->CreditCharge = cpu_to_le16(rsp_credit_charge);
+	credit_charge = max_t(unsigned short,
+			      le16_to_cpu(req_hdr->CreditCharge), 1);
+	credits_requested = max_t(unsigned short,
+				  le16_to_cpu(req_hdr->CreditRequest), 1);
 
-	if (credits_requested > 0) {
+	/* We must grant 0 credit for the final response of an asynchronous
+	 * operation.
+	 */
+	if ((hdr->Flags & SMB2_FLAGS_ASYNC_COMMAND) && !work->multiRsp) {
+		credits_granted = 0;
+	} else {
+		/* according to smb2.credits smbtorture, Windows server
+		 * 2016 or later grant up to 8192 credits at one.
+		 */
 		aux_credits = credits_requested - 1;
-		aux_max = 32;
 		if (hdr->Command == SMB2_NEGOTIATE)
 			aux_max = 0;
-		aux_credits = (aux_credits < aux_max) ? aux_credits : aux_max;
-		credits_granted = aux_credits + credit_charge;
+		else
+			aux_max = conn->max_credits - credit_charge;
+		aux_credits = min_t(unsigned short, aux_credits, aux_max);
+		credits_granted = credit_charge + aux_credits;
+
+		if (conn->max_credits - conn->total_credits < credits_granted)
+			credits_granted = conn->max_credits -
+				conn->total_credits;
 
-		/* if credits granted per client is getting bigger than default
-		 * minimum credits then we should wrap it up within the limits.
-		 */
-		if ((conn->total_credits + credits_granted) > min_credits)
-			credits_granted = min_credits -	conn->total_credits;
 		/*
 		 * TODO: Need to adjuct CreditRequest value according to
 		 * current cpu load
 		 */
-	} else if (conn->total_credits == 0) {
-		credits_granted = 1;
 	}
 
 	conn->total_credits += credits_granted;
@@ -371,7 +358,6 @@ int smb2_set_rsp_credits(struct ksmbd_work *work)
 		/* Update CreditRequest in last request */
 		hdr->CreditRequest = cpu_to_le16(work->credits_granted);
 	}
-out:
 	ksmbd_debug(SMB,
 		    "credits: requested[%d] granted[%d] total_granted[%d]\n",
 		    credits_requested, credits_granted,
@@ -692,13 +678,18 @@ int setup_async_work(struct ksmbd_work *work, void (*fn)(void **), void **arg)
 
 void smb2_send_interim_resp(struct ksmbd_work *work, __le32 status)
 {
-	struct smb2_hdr *rsp_hdr;
+	struct smb2_hdr *rsp_hdr = work->response_buf;
+
+	work->multiRsp = 1;
+	if (status != STATUS_CANCELLED) {
+		spin_lock(&work->conn->credits_lock);
+		smb2_set_rsp_credits(work);
+		spin_unlock(&work->conn->credits_lock);
+	}
 
-	rsp_hdr = work->response_buf;
 	smb2_set_err_rsp(work);
 	rsp_hdr->Status = status;
 
-	work->multiRsp = 1;
 	ksmbd_conn_write(work);
 	rsp_hdr->Status = 0;
 	work->multiRsp = 0;
@@ -1233,6 +1224,7 @@ int smb2_handle_negotiate(struct ksmbd_work *work)
 
 	conn->srv_sec_mode = le16_to_cpu(rsp->SecurityMode);
 	ksmbd_conn_set_need_negotiate(work);
+	conn->total_credits = 0;
 
 err_out:
 	if (rc < 0)
-- 
2.25.1

Re: [PATCH] ksmbd: improve credits management

[Namjae Jeon]

[snip]
>
> -	if (credits_requested > 0) {
> +	/* We must grant 0 credit for the final response of an asynchronous
> +	 * operation.
> +	 */
> +	if ((hdr->Flags & SMB2_FLAGS_ASYNC_COMMAND) && !work->multiRsp) {
Can you elaborate what is this check ? especially this !work->multiRsp..
> +		credits_granted = 0;
> +	} else {
> +		/* according to smb2.credits smbtorture, Windows server
> +		 * 2016 or later grant up to 8192 credits at one.
> +		 */
>  		aux_credits = credits_requested - 1;
> -		aux_max = 32;
>  		if (hdr->Command == SMB2_NEGOTIATE)
>  			aux_max = 0;
> -		aux_credits = (aux_credits < aux_max) ? aux_credits : aux_max;
> -		credits_granted = aux_credits + credit_charge;
> +		else
> +			aux_max = conn->max_credits - credit_charge;
> +		aux_credits = min_t(unsigned short, aux_credits, aux_max);
> +		credits_granted = credit_charge + aux_credits;
> +
> +		if (conn->max_credits - conn->total_credits < credits_granted)
> +			credits_granted = conn->max_credits -
> +				conn->total_credits;
>
> -		/* if credits granted per client is getting bigger than default
> -		 * minimum credits then we should wrap it up within the limits.
> -		 */
> -		if ((conn->total_credits + credits_granted) > min_credits)
> -			credits_granted = min_credits -	conn->total_credits;
>  		/*
>  		 * TODO: Need to adjuct CreditRequest value according to
>  		 * current cpu load
>  		 */
> -	} else if (conn->total_credits == 0) {
> -		credits_granted = 1;
>  	}
>
>  	conn->total_credits += credits_granted;
> @@ -371,7 +358,6 @@ int smb2_set_rsp_credits(struct ksmbd_work *work)
>  		/* Update CreditRequest in last request */
>  		hdr->CreditRequest = cpu_to_le16(work->credits_granted);
>  	}
> -out:
>  	ksmbd_debug(SMB,
>  		    "credits: requested[%d] granted[%d] total_granted[%d]\n",
>  		    credits_requested, credits_granted,
> @@ -692,13 +678,18 @@ int setup_async_work(struct ksmbd_work *work, void
> (*fn)(void **), void **arg)
>
>  void smb2_send_interim_resp(struct ksmbd_work *work, __le32 status)
>  {
> -	struct smb2_hdr *rsp_hdr;
> +	struct smb2_hdr *rsp_hdr = work->response_buf;
> +
> +	work->multiRsp = 1;
> +	if (status != STATUS_CANCELLED) {
> +		spin_lock(&work->conn->credits_lock);
> +		smb2_set_rsp_credits(work);
Can you explain why this code is needed in smb2_send_interim_resp() ?

> +		spin_unlock(&work->conn->credits_lock);
> +	}
>
> -	rsp_hdr = work->response_buf;
>  	smb2_set_err_rsp(work);
>  	rsp_hdr->Status = status;
>
> -	work->multiRsp = 1;
>  	ksmbd_conn_write(work);
>  	rsp_hdr->Status = 0;
>  	work->multiRsp = 0;
> @@ -1233,6 +1224,7 @@ int smb2_handle_negotiate(struct ksmbd_work *work)
>
>  	conn->srv_sec_mode = le16_to_cpu(rsp->SecurityMode);
>  	ksmbd_conn_set_need_negotiate(work);
> +	conn->total_credits = 0;
This line is needed ?

Thanks!
>
>  err_out:
>  	if (rc < 0)
> --
> 2.25.1
>
>

Re: [PATCH] ksmbd: improve credits management

[Hyunchul Lee]

2021년 10월 6일 (수) 오전 11:06, Namjae Jeon <linkinjeon@kernel.org>님이 작성:
>
> [snip]
> >
> > -     if (credits_requested > 0) {
> > +     /* We must grant 0 credit for the final response of an asynchronous
> > +      * operation.
> > +      */
> > +     if ((hdr->Flags & SMB2_FLAGS_ASYNC_COMMAND) && !work->multiRsp) {
> Can you elaborate what is this check ? especially this !work->multiRsp..

According to 3.3.4.2 Sending an Interim Response for an Asynchronous Operation
in MS-SMB2,
For the request that will be handled asynchronously,  a server should send
an interim response, and grant credits to the interim response.
on the other hand, a server should grant 0 credit to the final response for the
request.

"hdr->Flags & SMB2_FLAGS_ASYNC" means that a response is an interim
or final, and "work->multiRsp == 0" means that a response is final. So in this
case, a server should grant 0 credit.

> > +             credits_granted = 0;
> > +     } else {
> > +             /* according to smb2.credits smbtorture, Windows server
> > +              * 2016 or later grant up to 8192 credits at one.
> > +              */
> >               aux_credits = credits_requested - 1;
> > -             aux_max = 32;
> >               if (hdr->Command == SMB2_NEGOTIATE)
> >                       aux_max = 0;
> > -             aux_credits = (aux_credits < aux_max) ? aux_credits : aux_max;
> > -             credits_granted = aux_credits + credit_charge;
> > +             else
> > +                     aux_max = conn->max_credits - credit_charge;
> > +             aux_credits = min_t(unsigned short, aux_credits, aux_max);
> > +             credits_granted = credit_charge + aux_credits;
> > +
> > +             if (conn->max_credits - conn->total_credits < credits_granted)
> > +                     credits_granted = conn->max_credits -
> > +                             conn->total_credits;
> >
> > -             /* if credits granted per client is getting bigger than default
> > -              * minimum credits then we should wrap it up within the limits.
> > -              */
> > -             if ((conn->total_credits + credits_granted) > min_credits)
> > -                     credits_granted = min_credits - conn->total_credits;
> >               /*
> >                * TODO: Need to adjuct CreditRequest value according to
> >                * current cpu load
> >                */
> > -     } else if (conn->total_credits == 0) {
> > -             credits_granted = 1;
> >       }
> >
> >       conn->total_credits += credits_granted;
> > @@ -371,7 +358,6 @@ int smb2_set_rsp_credits(struct ksmbd_work *work)
> >               /* Update CreditRequest in last request */
> >               hdr->CreditRequest = cpu_to_le16(work->credits_granted);
> >       }
> > -out:
> >       ksmbd_debug(SMB,
> >                   "credits: requested[%d] granted[%d] total_granted[%d]\n",
> >                   credits_requested, credits_granted,
> > @@ -692,13 +678,18 @@ int setup_async_work(struct ksmbd_work *work, void
> > (*fn)(void **), void **arg)
> >
> >  void smb2_send_interim_resp(struct ksmbd_work *work, __le32 status)
> >  {
> > -     struct smb2_hdr *rsp_hdr;
> > +     struct smb2_hdr *rsp_hdr = work->response_buf;
> > +
> > +     work->multiRsp = 1;
> > +     if (status != STATUS_CANCELLED) {
> > +             spin_lock(&work->conn->credits_lock);
> > +             smb2_set_rsp_credits(work);
> Can you explain why this code is needed in smb2_send_interim_resp() ?
>

As I wrote above, a server should grant credits to an interim
response.

> > +             spin_unlock(&work->conn->credits_lock);
> > +     }
> >
> > -     rsp_hdr = work->response_buf;
> >       smb2_set_err_rsp(work);
> >       rsp_hdr->Status = status;
> >
> > -     work->multiRsp = 1;
> >       ksmbd_conn_write(work);
> >       rsp_hdr->Status = 0;
> >       work->multiRsp = 0;
> > @@ -1233,6 +1224,7 @@ int smb2_handle_negotiate(struct ksmbd_work *work)
> >
> >       conn->srv_sec_mode = le16_to_cpu(rsp->SecurityMode);
> >       ksmbd_conn_set_need_negotiate(work);
> > +     conn->total_credits = 0;
> This line is needed ?

Yes, conn->total_credits becomes 0 when receiving a SMB2_NEGOTIATION
request. But init_smbX_X_server functions are called many times and
conn->total_credits is set to 1 in these functions while negotiation.

>
> Thanks!
> >
> >  err_out:
> >       if (rc < 0)
> > --
> > 2.25.1
> >
> >



-- 
Thanks,
Hyunchul

Re: [PATCH] ksmbd: improve credits management

[Namjae Jeon]

2021-10-06 15:27 GMT+09:00, Hyunchul Lee <hyc.lee@gmail.com>:
> 2021년 10월 6일 (수) 오전 11:06, Namjae Jeon <linkinjeon@kernel.org>님이 작성:
>>
>> [snip]
>> >
>> > -     if (credits_requested > 0) {
>> > +     /* We must grant 0 credit for the final response of an
>> > asynchronous
>> > +      * operation.
>> > +      */
>> > +     if ((hdr->Flags & SMB2_FLAGS_ASYNC_COMMAND) && !work->multiRsp) {
>> Can you elaborate what is this check ? especially this !work->multiRsp..
>
> According to 3.3.4.2 Sending an Interim Response for an Asynchronous
> Operation
> in MS-SMB2,
> For the request that will be handled asynchronously,  a server should send
> an interim response, and grant credits to the interim response.
> on the other hand, a server should grant 0 credit to the final response for
> the
> request.
>
> "hdr->Flags & SMB2_FLAGS_ASYNC" means that a response is an interim
> or final, and "work->multiRsp == 0" means that a response is final. So in
> this
> case, a server should grant 0 credit.
Okay.
>
>> > +             credits_granted = 0;
>> > +     } else {
>> > +             /* according to smb2.credits smbtorture, Windows server
>> > +              * 2016 or later grant up to 8192 credits at one.
>> > +              */
>> >               aux_credits = credits_requested - 1;
>> > -             aux_max = 32;
>> >               if (hdr->Command == SMB2_NEGOTIATE)
>> >                       aux_max = 0;
>> > -             aux_credits = (aux_credits < aux_max) ? aux_credits :
>> > aux_max;
>> > -             credits_granted = aux_credits + credit_charge;
>> > +             else
>> > +                     aux_max = conn->max_credits - credit_charge;
>> > +             aux_credits = min_t(unsigned short, aux_credits,
>> > aux_max);
>> > +             credits_granted = credit_charge + aux_credits;
>> > +
>> > +             if (conn->max_credits - conn->total_credits <
>> > credits_granted)
>> > +                     credits_granted = conn->max_credits -
>> > +                             conn->total_credits;
>> >
>> > -             /* if credits granted per client is getting bigger than
>> > default
>> > -              * minimum credits then we should wrap it up within the
>> > limits.
>> > -              */
>> > -             if ((conn->total_credits + credits_granted) >
>> > min_credits)
>> > -                     credits_granted = min_credits -
>> > conn->total_credits;
>> >               /*
>> >                * TODO: Need to adjuct CreditRequest value according to
>> >                * current cpu load
>> >                */
>> > -     } else if (conn->total_credits == 0) {
>> > -             credits_granted = 1;
>> >       }
>> >
>> >       conn->total_credits += credits_granted;
>> > @@ -371,7 +358,6 @@ int smb2_set_rsp_credits(struct ksmbd_work *work)
>> >               /* Update CreditRequest in last request */
>> >               hdr->CreditRequest = cpu_to_le16(work->credits_granted);
>> >       }
>> > -out:
>> >       ksmbd_debug(SMB,
>> >                   "credits: requested[%d] granted[%d]
>> > total_granted[%d]\n",
>> >                   credits_requested, credits_granted,
>> > @@ -692,13 +678,18 @@ int setup_async_work(struct ksmbd_work *work,
>> > void
>> > (*fn)(void **), void **arg)
>> >
>> >  void smb2_send_interim_resp(struct ksmbd_work *work, __le32 status)
>> >  {
>> > -     struct smb2_hdr *rsp_hdr;
>> > +     struct smb2_hdr *rsp_hdr = work->response_buf;
>> > +
>> > +     work->multiRsp = 1;
>> > +     if (status != STATUS_CANCELLED) {
>> > +             spin_lock(&work->conn->credits_lock);
>> > +             smb2_set_rsp_credits(work);
>> Can you explain why this code is needed in smb2_send_interim_resp() ?
>>
>
> As I wrote above, a server should grant credits to an interim
> response.
Okay.
>
>> > +             spin_unlock(&work->conn->credits_lock);
>> > +     }
>> >
>> > -     rsp_hdr = work->response_buf;
>> >       smb2_set_err_rsp(work);
>> >       rsp_hdr->Status = status;
>> >
>> > -     work->multiRsp = 1;
>> >       ksmbd_conn_write(work);
>> >       rsp_hdr->Status = 0;
>> >       work->multiRsp = 0;
>> > @@ -1233,6 +1224,7 @@ int smb2_handle_negotiate(struct ksmbd_work
>> > *work)
>> >
>> >       conn->srv_sec_mode = le16_to_cpu(rsp->SecurityMode);
>> >       ksmbd_conn_set_need_negotiate(work);
>> > +     conn->total_credits = 0;
>> This line is needed ?
>
> Yes, conn->total_credits becomes 0 when receiving a SMB2_NEGOTIATION
> request. But init_smbX_X_server functions are called many times and
> conn->total_credits is set to 1 in these functions while negotiation.
If so, can we move conn->total_credits = 1 in  init_smbX_X_server() to
ksmbd_conn_alloc() ?
>
>>
>> Thanks!
>> >
>> >  err_out:
>> >       if (rc < 0)
>> > --
>> > 2.25.1
>> >
>> >
>
>
>
> --
> Thanks,
> Hyunchul
>

Re: [PATCH] ksmbd: improve credits management

[Ralph Boehme]

[-- Attachment #1.1: Type: text/plain, Size: 649 bytes --]

Am 05.10.21 um 12:00 schrieb Hyunchul Lee:
> * For an asynchronous operation, grant credits
> for an interim response and 0 credit for the
> final response.

fwiw, Samba also does this but this can cause significant problems as it 
means the server looses control over the receive window size. We've seen 
aggressive client go nuts about this overwhelming the server with IO 
requests leading to disconnects (iirc). So this may need careful 
checking how Windows implements this server side.

Cheers!
-slow

-- 
Ralph Boehme, Samba Team                 https://samba.org/
SerNet Samba Team Lead      https://sernet.de/en/team-samba

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH] ksmbd: improve credits management

[Hyunchul Lee]

2021년 10월 6일 (수) 오후 3:52, Namjae Jeon <linkinjeon@kernel.org>님이 작성:
>
> 2021-10-06 15:27 GMT+09:00, Hyunchul Lee <hyc.lee@gmail.com>:
> > 2021년 10월 6일 (수) 오전 11:06, Namjae Jeon <linkinjeon@kernel.org>님이 작성:
> >>
> >> [snip]
> >> >
> >> > -     if (credits_requested > 0) {
> >> > +     /* We must grant 0 credit for the final response of an
> >> > asynchronous
> >> > +      * operation.
> >> > +      */
> >> > +     if ((hdr->Flags & SMB2_FLAGS_ASYNC_COMMAND) && !work->multiRsp) {
> >> Can you elaborate what is this check ? especially this !work->multiRsp..
> >
> > According to 3.3.4.2 Sending an Interim Response for an Asynchronous
> > Operation
> > in MS-SMB2,
> > For the request that will be handled asynchronously,  a server should send
> > an interim response, and grant credits to the interim response.
> > on the other hand, a server should grant 0 credit to the final response for
> > the
> > request.
> >
> > "hdr->Flags & SMB2_FLAGS_ASYNC" means that a response is an interim
> > or final, and "work->multiRsp == 0" means that a response is final. So in
> > this
> > case, a server should grant 0 credit.
> Okay.
> >
> >> > +             credits_granted = 0;
> >> > +     } else {
> >> > +             /* according to smb2.credits smbtorture, Windows server
> >> > +              * 2016 or later grant up to 8192 credits at one.
> >> > +              */
> >> >               aux_credits = credits_requested - 1;
> >> > -             aux_max = 32;
> >> >               if (hdr->Command == SMB2_NEGOTIATE)
> >> >                       aux_max = 0;
> >> > -             aux_credits = (aux_credits < aux_max) ? aux_credits :
> >> > aux_max;
> >> > -             credits_granted = aux_credits + credit_charge;
> >> > +             else
> >> > +                     aux_max = conn->max_credits - credit_charge;
> >> > +             aux_credits = min_t(unsigned short, aux_credits,
> >> > aux_max);
> >> > +             credits_granted = credit_charge + aux_credits;
> >> > +
> >> > +             if (conn->max_credits - conn->total_credits <
> >> > credits_granted)
> >> > +                     credits_granted = conn->max_credits -
> >> > +                             conn->total_credits;
> >> >
> >> > -             /* if credits granted per client is getting bigger than
> >> > default
> >> > -              * minimum credits then we should wrap it up within the
> >> > limits.
> >> > -              */
> >> > -             if ((conn->total_credits + credits_granted) >
> >> > min_credits)
> >> > -                     credits_granted = min_credits -
> >> > conn->total_credits;
> >> >               /*
> >> >                * TODO: Need to adjuct CreditRequest value according to
> >> >                * current cpu load
> >> >                */
> >> > -     } else if (conn->total_credits == 0) {
> >> > -             credits_granted = 1;
> >> >       }
> >> >
> >> >       conn->total_credits += credits_granted;
> >> > @@ -371,7 +358,6 @@ int smb2_set_rsp_credits(struct ksmbd_work *work)
> >> >               /* Update CreditRequest in last request */
> >> >               hdr->CreditRequest = cpu_to_le16(work->credits_granted);
> >> >       }
> >> > -out:
> >> >       ksmbd_debug(SMB,
> >> >                   "credits: requested[%d] granted[%d]
> >> > total_granted[%d]\n",
> >> >                   credits_requested, credits_granted,
> >> > @@ -692,13 +678,18 @@ int setup_async_work(struct ksmbd_work *work,
> >> > void
> >> > (*fn)(void **), void **arg)
> >> >
> >> >  void smb2_send_interim_resp(struct ksmbd_work *work, __le32 status)
> >> >  {
> >> > -     struct smb2_hdr *rsp_hdr;
> >> > +     struct smb2_hdr *rsp_hdr = work->response_buf;
> >> > +
> >> > +     work->multiRsp = 1;
> >> > +     if (status != STATUS_CANCELLED) {
> >> > +             spin_lock(&work->conn->credits_lock);
> >> > +             smb2_set_rsp_credits(work);
> >> Can you explain why this code is needed in smb2_send_interim_resp() ?
> >>
> >
> > As I wrote above, a server should grant credits to an interim
> > response.
> Okay.
> >
> >> > +             spin_unlock(&work->conn->credits_lock);
> >> > +     }
> >> >
> >> > -     rsp_hdr = work->response_buf;
> >> >       smb2_set_err_rsp(work);
> >> >       rsp_hdr->Status = status;
> >> >
> >> > -     work->multiRsp = 1;
> >> >       ksmbd_conn_write(work);
> >> >       rsp_hdr->Status = 0;
> >> >       work->multiRsp = 0;
> >> > @@ -1233,6 +1224,7 @@ int smb2_handle_negotiate(struct ksmbd_work
> >> > *work)
> >> >
> >> >       conn->srv_sec_mode = le16_to_cpu(rsp->SecurityMode);
> >> >       ksmbd_conn_set_need_negotiate(work);
> >> > +     conn->total_credits = 0;
> >> This line is needed ?
> >
> > Yes, conn->total_credits becomes 0 when receiving a SMB2_NEGOTIATION
> > request. But init_smbX_X_server functions are called many times and
> > conn->total_credits is set to 1 in these functions while negotiation.
> If so, can we move conn->total_credits = 1 in  init_smbX_X_server() to
> ksmbd_conn_alloc() ?

Okay, I will move this into ksmbd_conn_alloc().
Thank you for your comments!

> >
> >>
> >> Thanks!
> >> >
> >> >  err_out:
> >> >       if (rc < 0)
> >> > --
> >> > 2.25.1
> >> >
> >> >
> >
> >
> >
> > --
> > Thanks,
> > Hyunchul
> >



-- 
Thanks,
Hyunchul

Re: [PATCH] ksmbd: improve credits management

[Hyunchul Lee]

2021년 10월 6일 (수) 오후 4:15, Ralph Boehme <slow@samba.org>님이 작성:
>
> Am 05.10.21 um 12:00 schrieb Hyunchul Lee:
> > * For an asynchronous operation, grant credits
> > for an interim response and 0 credit for the
> > final response.
>
> fwiw, Samba also does this but this can cause significant problems as it
> means the server looses control over the receive window size. We've seen
> aggressive client go nuts about this overwhelming the server with IO
> requests leading to disconnects (iirc). So this may need careful
> checking how Windows implements this server side.
>

Okay, I will drop this in the patch. And could you elaborate
on the situation that clients cause the problem?

Namjae, What do you think about Ralph's comment?

Thank you for your comments!

> Cheers!
> -slow
>
> --
> Ralph Boehme, Samba Team                 https://samba.org/
> SerNet Samba Team Lead      https://sernet.de/en/team-samba



-- 
Thanks,
Hyunchul

Re: [PATCH] ksmbd: improve credits management

[Ralph Boehme]

[-- Attachment #1.1: Type: text/plain, Size: 507 bytes --]

Am 06.10.21 um 10:43 schrieb Hyunchul Lee:
> Okay, I will drop this in the patch. And could you elaborate
> on the situation that clients cause the problem?

the client will just swamp you with IO requests and you've lost control 
over the creditting window. Iirc in Samba this resulted in OOM due to 
the many buffer allocations for the in-process IO requests.

-slow

-- 
Ralph Boehme, Samba Team                 https://samba.org/
SerNet Samba Team Lead      https://sernet.de/en/team-samba

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH] ksmbd: improve credits management

[Ralph Boehme]

[-- Attachment #1.1: Type: text/plain, Size: 570 bytes --]

Am 06.10.21 um 10:50 schrieb Ralph Boehme:
> Am 06.10.21 um 10:43 schrieb Hyunchul Lee:
>> Okay, I will drop this in the patch. And could you elaborate
>> on the situation that clients cause the problem?
> 
> the client will just swamp...

more precisely: the client *may* swamp you. Iirc we've seen this with a 
client where the (iirc Linux) application was writing some data that it 
generated very quickly *in memory*.

-slow

-- 
Ralph Boehme, Samba Team                 https://samba.org/
SerNet Samba Team Lead      https://sernet.de/en/team-samba

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH] ksmbd: improve credits management

[Namjae Jeon]

2021-10-06 17:43 GMT+09:00, Hyunchul Lee <hyc.lee@gmail.com>:
> 2021년 10월 6일 (수) 오후 4:15, Ralph Boehme <slow@samba.org>님이 작성:
>>
>> Am 05.10.21 um 12:00 schrieb Hyunchul Lee:
>> > * For an asynchronous operation, grant credits
>> > for an interim response and 0 credit for the
>> > final response.
>>
>> fwiw, Samba also does this but this can cause significant problems as it
>> means the server looses control over the receive window size. We've seen
>> aggressive client go nuts about this overwhelming the server with IO
>> requests leading to disconnects (iirc). So this may need careful
>> checking how Windows implements this server side.
>>
>
> Okay, I will drop this in the patch. And could you elaborate
> on the situation that clients cause the problem?
>
> Namjae, What do you think about Ralph's comment?
Let's remove async codes in this patch. I would like to know how I
verified this code.
i.e. not xfstests, Client attack that runs out of credits of ksmbd...
Should it be tested by change the credit management of the cifs client
or libsmb2?

Thanks!
>
> Thank you for your comments!
>
>> Cheers!
>> -slow
>>
>> --
>> Ralph Boehme, Samba Team                 https://samba.org/
>> SerNet Samba Team Lead      https://sernet.de/en/team-samba
>
>
>
> --
> Thanks,
> Hyunchul
>

Re: [PATCH] ksmbd: improve credits management

[Hyunchul Lee]

2021년 10월 7일 (목) 오전 10:11, Namjae Jeon <linkinjeon@kernel.org>님이 작성:
>
> 2021-10-06 17:43 GMT+09:00, Hyunchul Lee <hyc.lee@gmail.com>:
> > 2021년 10월 6일 (수) 오후 4:15, Ralph Boehme <slow@samba.org>님이 작성:
> >>
> >> Am 05.10.21 um 12:00 schrieb Hyunchul Lee:
> >> > * For an asynchronous operation, grant credits
> >> > for an interim response and 0 credit for the
> >> > final response.
> >>
> >> fwiw, Samba also does this but this can cause significant problems as it
> >> means the server looses control over the receive window size. We've seen
> >> aggressive client go nuts about this overwhelming the server with IO
> >> requests leading to disconnects (iirc). So this may need careful
> >> checking how Windows implements this server side.
> >>
> >
> > Okay, I will drop this in the patch. And could you elaborate
> > on the situation that clients cause the problem?
> >
> > Namjae, What do you think about Ralph's comment?
> Let's remove async codes in this patch. I would like to know how I
> verified this code.
> i.e. not xfstests, Client attack that runs out of credits of ksmbd...
> Should it be tested by change the credit management of the cifs client
> or libsmb2?

If we just check that ksmbd refuses requests when the number of granted credits
is 0, we can modify smb2-cat-sync and the library in libsmb2 temporarily.



>
> Thanks!
> >
> > Thank you for your comments!
> >
> >> Cheers!
> >> -slow
> >>
> >> --
> >> Ralph Boehme, Samba Team                 https://samba.org/
> >> SerNet Samba Team Lead      https://sernet.de/en/team-samba
> >
> >
> >
> > --
> > Thanks,
> > Hyunchul
> >



--
Thanks,
Hyunchul