Failure with compound requests and aio enabled

Jeremy Allison jra at samba.org
Fri Sep 22 01:02:52 UTC 2017


On Thu, Sep 21, 2017 at 05:49:24PM -0700, Christof Schmitt wrote:
> On Thu, Sep 21, 2017 at 05:34:44PM -0700, Jeremy Allison wrote:
> > On Thu, Sep 21, 2017 at 05:31:16PM -0700, Christof Schmitt wrote:
> > > 
> > > Nevermind, i found a way. schedule_smb2_sendfile_read() already checks
> > > smb2req->in.vector_count to determine whether the request is part of a
> > > compound. I will reuse this check. Updated patch will follow.
> > 
> > Oh that's *great* - thanks ! Not adding that bool
> > preserves the VFS ABI. I thought there must be a
> > way to detect this...
> 
> See attached patches. Should i open a bugzilla for a backport?

Thanks - I'll review. There is already a bug report I think
I closed :-). You could always reopen that one :-).

> From 41981207ac7d25c4f7042fcde917d2bfaba45f00 Mon Sep 17 00:00:00 2001
> From: Christof Schmitt <cs at samba.org>
> Date: Thu, 21 Sep 2017 17:41:25 -0700
> Subject: [PATCH 1/4] smbd: Move check for SMB2 compound request to new
>  function
> 
> Signed-off-by: Christof Schmitt <cs at samba.org>
> ---
>  source3/smbd/globals.h     | 1 +
>  source3/smbd/smb2_read.c   | 2 +-
>  source3/smbd/smb2_server.c | 5 +++++
>  3 files changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/source3/smbd/globals.h b/source3/smbd/globals.h
> index ae5ecf4..78f1260 100644
> --- a/source3/smbd/globals.h
> +++ b/source3/smbd/globals.h
> @@ -225,6 +225,7 @@ void smbd_server_connection_terminate_ex(struct smbXsrv_connection *xconn,
>  
>  const char *smb2_opcode_name(uint16_t opcode);
>  bool smbd_is_smb2_header(const uint8_t *inbuf, size_t size);
> +bool smbd_smb2_is_compound(const struct smbd_smb2_request *req);
>  
>  NTSTATUS smbd_add_connection(struct smbXsrv_client *client, int sock_fd,
>  			     struct smbXsrv_connection **_xconn);
> diff --git a/source3/smbd/smb2_read.c b/source3/smbd/smb2_read.c
> index ecedf1e..273c56c 100644
> --- a/source3/smbd/smb2_read.c
> +++ b/source3/smbd/smb2_read.c
> @@ -350,7 +350,7 @@ static NTSTATUS schedule_smb2_sendfile_read(struct smbd_smb2_request *smb2req,
>  	if (!lp__use_sendfile(SNUM(fsp->conn)) ||
>  	    smb2req->do_signing ||
>  	    smb2req->do_encryption ||
> -	    smb2req->in.vector_count >= (2*SMBD_SMB2_NUM_IOV_PER_REQ) ||
> +	    smbd_smb2_is_compound(smb2req) ||
>  	    (fsp->base_fsp != NULL) ||
>  	    (fsp->wcp != NULL) ||
>  	    (!S_ISREG(fsp->fsp_name->st.st_ex_mode)) ||
> diff --git a/source3/smbd/smb2_server.c b/source3/smbd/smb2_server.c
> index d95631f..68a024f 100644
> --- a/source3/smbd/smb2_server.c
> +++ b/source3/smbd/smb2_server.c
> @@ -204,6 +204,11 @@ bool smbd_is_smb2_header(const uint8_t *inbuf, size_t size)
>  	return true;
>  }
>  
> +bool smbd_smb2_is_compound(const struct smbd_smb2_request *req)
> +{
> +	return req->in.vector_count >= (2*SMBD_SMB2_NUM_IOV_PER_REQ);
> +}
> +
>  static NTSTATUS smbd_initialize_smb2(struct smbXsrv_connection *xconn,
>  				     uint64_t expected_seq_low)
>  {
> -- 
> 1.8.3.1
> 
> 
> From ef6f0d91c0b5b2abd78f5650539bf708f5b61d2c Mon Sep 17 00:00:00 2001
> From: Christof Schmitt <cs at samba.org>
> Date: Thu, 21 Sep 2017 12:08:01 -0700
> Subject: [PATCH 2/4] smbd/aio: Do not go async for SMB2 compound requests
> 
> Signed-off-by: Christof Schmitt <cs at samba.org>
> ---
>  source3/smbd/aio.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/source3/smbd/aio.c b/source3/smbd/aio.c
> index f455d04..d3611ba 100644
> --- a/source3/smbd/aio.c
> +++ b/source3/smbd/aio.c
> @@ -697,6 +697,10 @@ NTSTATUS schedule_smb2_aio_read(connection_struct *conn,
>  		return NT_STATUS_RETRY;
>  	}
>  
> +	if (smbd_smb2_is_compound(smbreq->smb2req)) {
> +		return NT_STATUS_RETRY;
> +	}
> +
>  	/* Create the out buffer. */
>  	*preadbuf = data_blob_talloc(ctx, NULL, smb_maxcnt);
>  	if (preadbuf->data == NULL) {
> @@ -841,6 +845,10 @@ NTSTATUS schedule_aio_smb2_write(connection_struct *conn,
>  		return NT_STATUS_RETRY;
>  	}
>  
> +	if (smbd_smb2_is_compound(smbreq->smb2req)) {
> +		return NT_STATUS_RETRY;
> +	}
> +
>  	if (smbreq->unread_bytes) {
>  		/* Can't do async with recvfile. */
>  		return NT_STATUS_RETRY;
> -- 
> 1.8.3.1
> 
> 
> From c4c381dd40e979218c101c6a855b5778758ec522 Mon Sep 17 00:00:00 2001
> From: Christof Schmitt <cs at samba.org>
> Date: Wed, 20 Sep 2017 16:07:50 -0700
> Subject: [PATCH 3/4] torture: Add testcase for compound CREATE-WRITE-CLOSE
>  request
> 
> Signed-off-by: Christof Schmitt <cs at samba.org>
> ---
>  source4/torture/smb2/compound.c | 73 +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 73 insertions(+)
> 
> diff --git a/source4/torture/smb2/compound.c b/source4/torture/smb2/compound.c
> index e501870..c592308 100644
> --- a/source4/torture/smb2/compound.c
> +++ b/source4/torture/smb2/compound.c
> @@ -671,6 +671,77 @@ done:
>  	return ret;
>  }
>  
> +static bool test_compound_create_write_close(struct torture_context *tctx,
> +					     struct smb2_tree *tree)
> +{
> +	struct smb2_handle handle = { .data = { UINT64_MAX, UINT64_MAX } };
> +	struct smb2_create create;
> +	struct smb2_write write;
> +	struct smb2_close close;
> +	const char *fname = "compound_create_write_close.dat";
> +	struct smb2_request *req[3];
> +	NTSTATUS status;
> +	bool ret = false;
> +
> +	smb2_util_unlink(tree, fname);
> +
> +	ZERO_STRUCT(create);
> +	create.in.security_flags = 0x00;
> +	create.in.oplock_level = 0;
> +	create.in.impersonation_level = NTCREATEX_IMPERSONATION_IMPERSONATION;
> +	create.in.create_flags = 0x00000000;
> +	create.in.reserved = 0x00000000;
> +	create.in.desired_access = SEC_RIGHTS_FILE_ALL;
> +	create.in.file_attributes = FILE_ATTRIBUTE_NORMAL;
> +	create.in.share_access = NTCREATEX_SHARE_ACCESS_READ |
> +		NTCREATEX_SHARE_ACCESS_WRITE |
> +		NTCREATEX_SHARE_ACCESS_DELETE;
> +	create.in.create_disposition = NTCREATEX_DISP_OPEN_IF;
> +	create.in.create_options = NTCREATEX_OPTIONS_SEQUENTIAL_ONLY |
> +		NTCREATEX_OPTIONS_ASYNC_ALERT |
> +		NTCREATEX_OPTIONS_NON_DIRECTORY_FILE |
> +		0x00200000;
> +	create.in.fname = fname;
> +
> +	smb2_transport_compound_start(tree->session->transport, 3);
> +
> +	req[0] = smb2_create_send(tree, &create);
> +
> +	smb2_transport_compound_set_related(tree->session->transport, true);
> +
> +	ZERO_STRUCT(write);
> +	write.in.file.handle = handle;
> +	write.in.offset = 0;
> +	write.in.data = data_blob_talloc(tctx, NULL, 1024);
> +
> +	req[1] = smb2_write_send(tree, &write);
> +
> +	ZERO_STRUCT(close);
> +	close.in.file.handle = handle;
> +
> +	req[2] = smb2_close_send(tree, &close);
> +
> +	status = smb2_create_recv(req[0], tree, &create);
> +	torture_assert_ntstatus_ok_goto(tctx, status, ret, done,
> +					"CREATE failed.");
> +
> +	status = smb2_write_recv(req[1], &write);
> +	torture_assert_ntstatus_ok_goto(tctx, status, ret, done,
> +					"WRITE failed.");
> +
> +	status = smb2_close_recv(req[2], &close);
> +	torture_assert_ntstatus_ok_goto(tctx, status, ret, done,
> +					"CLOSE failed.");
> +
> +	status = smb2_util_unlink(tree, fname);
> +	torture_assert_ntstatus_ok_goto(tctx, status, ret, done,
> +					"File deletion failed.");
> +
> +	ret = true;
> +done:
> +	return ret;
> +}
> +
>  static bool test_compound_unrelated1(struct torture_context *tctx,
>  				     struct smb2_tree *tree)
>  {
> @@ -1230,6 +1301,8 @@ struct torture_suite *torture_smb2_compound_init(TALLOC_CTX *ctx)
>  	torture_suite_add_1smb2_test(suite, "interim2",  test_compound_interim2);
>  	torture_suite_add_1smb2_test(suite, "compound-break", test_compound_break);
>  	torture_suite_add_1smb2_test(suite, "compound-padding", test_compound_padding);
> +	torture_suite_add_1smb2_test(suite, "create-write-close",
> +				     test_compound_create_write_close);
>  
>  	suite->description = talloc_strdup(suite, "SMB2-COMPOUND tests");
>  
> -- 
> 1.8.3.1
> 
> 
> From 377ac70e3698de179b7c0a22311ec6e2828710aa Mon Sep 17 00:00:00 2001
> From: Christof Schmitt <cs at samba.org>
> Date: Wed, 20 Sep 2017 16:13:38 -0700
> Subject: [PATCH 4/4] selftest: Also run smbtorture smb2.compound with aio
>  enabled
> 
> Signed-off-by: Christof Schmitt <cs at samba.org>
> ---
>  selftest/knownfail        | 1 +
>  source3/selftest/tests.py | 4 ++++
>  2 files changed, 5 insertions(+)
> 
> diff --git a/selftest/knownfail b/selftest/knownfail
> index aa89dab..953b181 100644
> --- a/selftest/knownfail
> +++ b/selftest/knownfail
> @@ -172,6 +172,7 @@
>  ^samba3.smb2.setinfo.setinfo
>  ^samba3.smb2.session.*reauth5 # some special anonymous checks?
>  ^samba3.smb2.compound.interim2 # wrong return code (STATUS_CANCELLED)
> +^samba3.smb2.compound.aio.interim2 # wrong return code (STATUS_CANCELLED)
>  ^samba3.smb2.replay.channel-sequence
>  ^samba3.smb2.replay.replay3
>  ^samba3.smb2.replay.replay4
> diff --git a/source3/selftest/tests.py b/source3/selftest/tests.py
> index 139bba0..73479fc 100755
> --- a/source3/selftest/tests.py
> +++ b/source3/selftest/tests.py
> @@ -518,6 +518,10 @@ for t in tests:
>      elif t == "rpc.samr.users.privileges":
>          plansmbtorture4testsuite(t, "nt4_dc", '//$SERVER_IP/tmp -U$USERNAME%$PASSWORD --option=torture:nt4_dc=true')
>          plansmbtorture4testsuite(t, "ad_dc", '//$SERVER/tmp -U$USERNAME%$PASSWORD')
> +    elif t == "smb2.compound":
> +        plansmbtorture4testsuite(t, "nt4_dc", '//$SERVER_IP/tmp -U$USERNAME%$PASSWORD')
> +        plansmbtorture4testsuite(t, "nt4_dc", '//$SERVER_IP/aio -U$USERNAME%$PASSWORD', 'aio')
> +        plansmbtorture4testsuite(t, "ad_dc", '//$SERVER/tmp -U$USERNAME%$PASSWORD')
>      else:
>          plansmbtorture4testsuite(t, "nt4_dc", '//$SERVER_IP/tmp -U$USERNAME%$PASSWORD')
>          plansmbtorture4testsuite(t, "ad_dc", '//$SERVER/tmp -U$USERNAME%$PASSWORD')
> -- 
> 1.8.3.1
> 




More information about the samba-technical mailing list