[PATCH] Fix bug 9412 - SMB2 server doesn't support recvfile.

[Stefan (metze) Metzmacher]

Hi Jeremy,

> The good news is that it got a lot smaller :-).
> I'm really pleased with how little I had to
> change to make this work. It's only a 10 element
> patch set, and most of these are 5-10 line micro
> changes.
> 
> It turns out the changes made for 4.0.x meant
> that I had to make *no* changes in the
> smbd/smb2_write.c code to make RECVFILE work
> (which is pretty amazing :-), and I think proves
> we have some good abstractions in the 4.0.x code.
> 
> This patchset has been tested by me under valgrind,
> and by the OEM on their box, and gives an 18%
> improvement in SMB2 write speed which finally makes
> SMB2 faster than SMB1 on their smaller machines.
> 
> Please review and give feedback (feel free to push
> if you're happy with it :-).

This really looks great! Thanks!

I have a few comments see below.

> diff --git a/source3/smbd/smb2_server.c b/source3/smbd/smb2_server.c
> index eb7059e..edd54cc 100644
> --- a/source3/smbd/smb2_server.c
> +++ b/source3/smbd/smb2_server.c
> @@ -2621,11 +2621,20 @@ NTSTATUS smbd_smb2_request_error_ex(struct smbd_smb2_request *req,
>  {
>  	DATA_BLOB body;
>  	uint8_t *outhdr = SMBD_SMB2_OUT_HDR_PTR(req);
> +	size_t unread_bytes = smbd_smb2_unread_bytes(req);
>  
>  	DEBUG(10,("smbd_smb2_request_error_ex: idx[%d] status[%s] |%s| at %s\n",
>  		  req->current_idx, nt_errstr(status), info ? " +info" : "",
>  		  location));
>  
> +	if (unread_bytes) {
> +		/* Recvfile error. Drain incoming socket. */
> +		if (drain_socket(req->sconn->sock, unread_bytes) !=
> +				unread_bytes) {
> +			smb_panic("Failed to drain SMB2 socket\n");
> +		}
> +	}
> +

Also a helper variable to avoid the function call within the if
statement would be nice.

I think it would be also better to return an error and correctly
terminate the connection
instead of using smb_panic(), it's not unlikely that the connection got
broken.
For a broken connection we should terminate gracefully and let the
client reopen
durable handles.

I think we should add some protection to req->sconn->smb2.recv_queue,
when we do a short read we should place a dummy tevent_req into the
recv_queue,
which blocks the socket, and remove it when the socket is drained or
recvfile is done.

next to smb1req->unread_bytes we could setup smb2req->recvfile_blocker;

struct smbd_smb2_recvfile_blocker {
	uint8_t dummy;
};

static void smbd_smb2_recvfile_blocker_trigger(struct tevent_req *req,
					       void *private_data)
{
	return;
}

...
	struct smbd_smb2_recvfile_blocker *recvfile_blocker;
	smb2req->recvfile_blocker = tevent_req_create(smb2req, &recvfile_blocker,
						      smbd_smb2_recvfile_blocker);
	if (smb2req->recvfile_blocker == NULL) {
		...
	}
	ok = tevent_queue_add(smb2req->sconn->smb2.recv_queue,
			      ...event_context,
			      smb2req->recvfile_blocker,
			      smbd_smb2_recvfile_blocker_trigger,
			      NULL);

smbd_smb2_request_error_ex() would then also do
TALLOC_FREE(smb2req->recvfile_blocker);
after drain_socket() and even if unread_bytes was 0.

> +		} else {
> +			/*
> +			 * Either this is a receivefile write so we've
> +			 * done a short read, or if not we have all the data.
> +			 * Either way, we're done and the caller will handle
> +			 * and short read case by looking at the
> +			 * state->doing_receivefile value.
> +			 */
> +			*_vector = NULL;
> +			*_count = 0;
> +		}

Shouldn't the caller take a look at smb1req->unread_bytes?
state->doing_receivefile is not visible to the caller.

>  	vector[0].iov_base = (void *)state->pktbuf;
> -	vector[0].iov_len = state->pktlen;
> +	if (state->min_recv_size &&
> +			state->pktlen > SMBD_SMB2_SHORT_RECEIVEFILE_WRITE_LEN +
> +					state->min_recv_size) {

Can you add a helper variable to get rid of the strange formatting?
Something like:

min_recvfile_size = SMBD_SMB2_SHORT_RECEIVEFILE_WRITE_LEN;
min_recvfile_size += state->min_recv_size;
if (state->min_recv_size == 0) {
	min_recvfile_size = UINT32_MAX;
}
if (state->pktlen > min_recvfile_size) {

> +	if (IVAL(state->pktbuf, 0) == SMB2_TF_MAGIC) {
> +		/* Transform header. Cannot recvfile. */
> +		return false;
> +	}
> +
> +	if (IVAL(state->pktbuf, 0) != SMB2_MAGIC) {
> +		/* Not SMB2. Normal error path will cope. */
> +		return false;
> +	}
> +	if (SVAL(state->pktbuf, 4) != SMB2_HDR_BODY) {
> +		/* Not SMB2. Normal error path will cope. */
> +		return false;
> +	}
> +	if (IVAL(state->pktbuf, SMB2_HDR_NEXT_COMMAND) != 0) {
> +		/* Chained. Cannot recvfile. */
> +		return false;
> +	}
> +	if (IVAL(state->pktbuf, SMB2_HDR_FLAGS) &
> +			(SMB2_HDR_FLAG_CHAINED|
> +			 SMB2_HDR_FLAG_SIGNED)) {
> +		/* Signed or chained. Cannot recvfile. */
> +		return false;
> +	}

Can you split this into 2 checks to avoid the strange formatting?

> +	if (SVAL(state->pktbuf, SMB2_HDR_OPCODE) != SMB2_OP_WRITE) {
> +		/* Needs to be a WRITE. */
> +		return false;
> +	}

Shouldn't we do this check as the first check?

metze

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 261 bytes
Desc: OpenPGP digital signature
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20130404/a9657151/attachment.pgp>