[HELP?] Weird problem with SMB client code

Tim Beale timbeale at catalyst.net.nz
Tue Jan 8 00:30:31 UTC 2019


I've dug a bit deeper and worked out the problem now. The SMB message
was getting dropped on the client-side due to lack of SMBv2 credits.

The attached patches fix the problem for me. Hopefully it explains the
problem I was seeing in more detail. The change is just updating
cli_smb2_list() to use a smaller response buffer, based on the
connection's available credits.

If someone could review the change, that'd be greatly appreciated.

CI link: https://gitlab.com/catalyst-samba/samba/pipelines/42484993

Thanks,
Tim

On 15/12/18 1:07 PM, Jeremy Allison wrote:
> On Fri, Dec 14, 2018 at 03:41:40PM +1300, Tim Beale via samba-technical wrote:
>> I'm hitting a weird problem with the SMB client code. I'll continue
>> debugging it in the new year, but just posting it here in case it rings
>> any bells with anyone.
>>
>> If I have an SMBv2 connection, and cli_list() is the first call I make
>> on the connection, then I get an NT_STATUS_INTERNAL_ERROR back from the
>> server. I couldn't see any obvious error message on the server (at debug
>> level 5).
>>
>> It doesn't happen on SMBv1 connections. If I do another SMB operation
>> first, then it's fine. E.g. I'm working around the problem by just
>> calling cli_list() a 2nd time if it fails.
>>
>> It doesn't seem to be a problem with smbclient, but I'm not sure what
>> the pylibsmb code is doing differently. The main differences in
>> smbclient I could see were:
>> - It does a cli_resolve_path() first. I'm not really sure what this is
>> doing. I tried it, but it didn't seem to help (but maybe I wasn't
>> calling it properly).
>> - A slightly different API is used to connect (cli_cm_open() vs
>> cli_full_connection_creds_send()).
>>
>> If anyone wants to see the problem first-hand, you can checkout the
>> tim-smb-wip branch on samba-team/devel/samba.git and then go: make test
>> TESTS=samba.tests.ntacls_backup
>> The test throws errors:
>>     return self.smb_conn.list(smb_path, attribs=SMB_FILE_ATTRIBUTE_FLAGS)
>> NTSTATUSError: (3221225701, 'An internal error occurred.')
>>
>> Attached is a hacky work-around that makes the problem go away if you
>> apply it.
> Run with debug level 10 to see if this is a client or
> server error.
>
> That should be the first step.
>
>> From fc0a86f2e7331bd3a2eaa99cf1594017f5d5c23f Mon Sep 17 00:00:00 2001
>> From: Tim Beale <timbeale at catalyst.net.nz>
>> Date: Thu, 13 Dec 2018 16:01:55 +1300
>> Subject: [PATCH] TODO: Weird problem with SMBv2 cli_list()
>>
>> The first cli_list() call on an SMBv2 connection currently always fails.
>> This was being masked in the s4 smb tests because they do a mkdir() in
>> the setUp(), which "fixes" the problem. The ntacls tests hit this
>> problem, however.
>> ---
>>  source3/libsmb/pylibsmb.c | 12 ++++++++++++
>>  1 file changed, 12 insertions(+)
>>
>> diff --git a/source3/libsmb/pylibsmb.c b/source3/libsmb/pylibsmb.c
>> index 3e0736d..cb41af1 100644
>> --- a/source3/libsmb/pylibsmb.c
>> +++ b/source3/libsmb/pylibsmb.c
>> @@ -1190,6 +1190,18 @@ static NTSTATUS do_listing(struct py_cli_state *self,
>>  	} else {
>>  		status = cli_list(self->cli, mask, attribute, callback_fn,
>>  				  priv);
>> +
>> +		// TODO??? This is weird. The first smb_conn.list() call seems
>> +		// to fail. Simply repeating the same thing again works. If you
>> +		// run another SMB operation (like smb_conn.mkdir()), then it
>> +		// also fixes the problem. Forcing SMBv1 (regardless of whether
>> +		// sync or async) also avoids the problem. The problem is
>> +		// smb2cli_query_directory_recv() returns:
>> +		//   NTSTATUSError: (3221225701, 'An internal error occurred.')
>> +		if (!NT_STATUS_IS_OK(status)) {
>> +			status = cli_list(self->cli, mask, attribute, callback_fn,
>> +					  priv);
>> +		}
>>  	}
>>  	TALLOC_FREE(mask);
>>  
>> -- 
>> 2.7.4
>>
-------------- next part --------------
From 19620832c331526e1934f6bdb626041aa44d1e25 Mon Sep 17 00:00:00 2001
From: Tim Beale <timbeale at catalyst.net.nz>
Date: Mon, 7 Jan 2019 12:06:15 +1300
Subject: [PATCH 1/2] libcli: Add error log if insufficient SMB2 credits

Although it's unusual to hit this case, I was seeing it happen while
working on the SMB python bindings. Even with debug level 10, there was
nothing coming out to help pin down the source of the
NT_STATUS_INTERNAL_ERROR.

BUG: https://bugzilla.samba.org/show_bug.cgi?id=13676

Signed-off-by: Tim Beale <timbeale at catalyst.net.nz>
---
 libcli/smb/smbXcli_base.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/libcli/smb/smbXcli_base.c b/libcli/smb/smbXcli_base.c
index 40480c8..df3b31c 100644
--- a/libcli/smb/smbXcli_base.c
+++ b/libcli/smb/smbXcli_base.c
@@ -3231,6 +3231,8 @@ NTSTATUS smb2cli_req_compound_submit(struct tevent_req **reqs,
 
 		avail = MIN(avail, state->conn->smb2.cur_credits);
 		if (avail < charge) {
+			DBG_ERR("Insufficient credits. %lu available, %u needed\n",
+				avail, charge);
 			return NT_STATUS_INTERNAL_ERROR;
 		}
 
-- 
2.7.4


From 8bb5df1e3f15aecc2168717fe4f930cc76cfbb93 Mon Sep 17 00:00:00 2001
From: Tim Beale <timbeale at catalyst.net.nz>
Date: Mon, 7 Jan 2019 15:28:12 +1300
Subject: [PATCH 2/2] s3:libsmb: cli_smb2_list() can sometimes fail initially
 on a connection

cli_smb2_list() appears to be a slightly unique SMB operation in that it
specifies the max transaction size for the response buffer size. The
Python bindings highlighted a problem where if cli_smb2_list() were one
of the first operations performed on the SMBv2 connection, it would fail
due to insufficient credits. Because the response buffer size is
(potentially) so much larger, it requires more credits (128) compared
with other SMB operations.

When talking to a samba DC, the connection credits seem to start off at
1, then increase by 32 for every SMB reply we receive back from the
server. After cli_full_connection(), the connection has 65 credits. The
cli_smb2_create_fnum() in cli_smb2_list() adds another 32 credits, but
this is still less than the 128 that smb2cli_query_directory() requires.

This problem doesn't happen for smbclient because the cli_cm_open() API
it uses ends up sending more messages, and so the connection has more
credits.

This patch changes cli_smb2_list(), so it requests a smaller response
buffer size if it doesn't have enough credits available for the max
transaction size. smb2cli_query_directory() is already in a loop, so it
can span multiple SMB messages if for some reason the transaction size
isn't big enough for the listings.

BUG: https://bugzilla.samba.org/show_bug.cgi?id=13676

Signed-off-by: Tim Beale <timbeale at catalyst.net.nz>
---
 source3/libsmb/cli_smb2_fnum.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/source3/libsmb/cli_smb2_fnum.c b/source3/libsmb/cli_smb2_fnum.c
index 6cba442..3a64438 100644
--- a/source3/libsmb/cli_smb2_fnum.c
+++ b/source3/libsmb/cli_smb2_fnum.c
@@ -919,7 +919,9 @@ NTSTATUS cli_smb2_list(struct cli_state *cli,
 	TALLOC_CTX *frame = talloc_stackframe();
 	TALLOC_CTX *subframe = NULL;
 	bool mask_has_wild;
-	uint32_t max_trans = smb2cli_conn_max_trans_size(cli->conn);
+	uint32_t max_trans;
+	uint32_t max_avail_len;
+	bool ok;
 
 	if (smbXcli_conn_has_async_calls(cli->conn)) {
 		/*
@@ -968,6 +970,16 @@ NTSTATUS cli_smb2_list(struct cli_state *cli,
 		goto fail;
 	}
 
+	/*
+	 * ideally, use the max transaction size, but don't send a request
+	 * bigger than we have credits available for
+	 */
+	max_trans = smb2cli_conn_max_trans_size(cli->conn);
+	ok = smb2cli_conn_req_possible(cli->conn, &max_avail_len);
+	if (ok) {
+		max_trans = MIN(max_trans, max_avail_len);
+	}
+
 	do {
 		uint8_t *dir_data = NULL;
 		uint32_t dir_data_length = 0;
-- 
2.7.4



More information about the samba-technical mailing list