----- Original Message -----
> From: "Aurélien Aptel" <aaptel@xxxxxxxx>
> To: "Ronnie Sahlberg" <lsahlber@xxxxxxxxxx>, "linux-cifs" <linux-cifs@xxxxxxxxxxxxxxx>, "Long Li"
> <longli@xxxxxxxxxxxxx>
> Cc: "Steve French" <smfrench@xxxxxxxxx>
> Sent: Thursday, 1 March, 2018 1:21:43 AM
> Subject: Re: [PATCH 00/14] cifs: add compounding support for smb2+
>
> Hi Ronnie, all,
>
> I've rebasing this on current for-next since it couldn't be applied
> anymore. Beside the simple conflicts there are a couple of non-trivial
> changes I've made.
>
> Since some SMB DIRECT code made it in the meantime I had to handle some
> conflicts.
>
> in smb2pdu.c, smb2_async_writev():
>
> > rqst.rq_iov = iov;
> > rqst.rq_nvec = 2;
> > rqst.rq_pages = wdata->pages;
> > rqst.rq_npages = wdata->nr_pages;
> > rqst.rq_pagesz = wdata->pagesz;
> > rqst.rq_tailsz = wdata->tailsz;
> > #ifdef CONFIG_CIFS_SMB_DIRECT
> > if (wdata->mr) {
> > iov[0].iov_len += sizeof(struct smbd_buffer_descriptor_v1);
> > rqst.rq_npages = 0;
> > }
> > #endif
>
> since iov is an array of size 1, I've changed the SMB_DIRECT code to use
> iov[0] instead of iov[1]. But I have no idea if it's really correct.
>
> The thing that bothers me is iov[] has a size of 1 yet
> > rqst.rq_iov = iov;
> > rqst.rq_nvec = 2;
>
> mhh..
>
> =========
>
> The other thing I changed was moving the SMB311 preauth hash update to the
> new
> cifs_send_receive() wrapper like so:
>
> > int
> > cifs_send_recv(const unsigned int xid, struct cifs_ses *ses,
> > struct smb_rqst *rqst, int *resp_buf_type, const int flags,
> > struct kvec *resp_iov)
> > {
> > int rc;
> >
> > #ifdef CONFIG_CIFS_SMB311
> > if (ses->status == CifsNew)
> > smb311_update_preauth_hash(ses, rqst->rq_iov,
> > rqst->rq_nvec);
> > #endif
> >
> > rc = compound_send_recv(xid, ses, flags, 1, rqst, resp_buf_type,
> > resp_iov);
> >
> > #ifdef CONFIG_CIFS_SMB311
> > if (ses->status == CifsNew) {
> > smb311_update_preauth_hash(ses, resp_iov, 1);
> > }
> > #endif
> > return rc;
> >
>
> =========
>
> Small change for a sparse warning I was getting:
>
> > #define COMPOUND_FID 0xFFFFFFFFFFFFFFFFULL
>
> Added the ULL suffix as the constant is too big for an int.
>
> =========
>
> Now these I have left these as-is but I have some comments:
>
> =========
>
> in transport.c, compound_send_receive():
>
> > for (i = 0; i < num_rqst; i++) {
> > cifs_save_when_sent(midQ[i]);
> >
> > if (rc < 0)
> > ses->server->sequence_number -= 2;
> > mutex_unlock(&ses->server->srv_mutex);
>
> We potentially unlock the mutex at every loop. Is that on purpose?
No that is a bug I have fixed.
>
> =========
>
> In cifs_demultiplex_thread():
>
> > pdu_length = get_rfc1002_length(buf);
> > - server->total_size = pdu_length;
> >
> > - cifs_dbg(FYI, "RFC1002 header 0x%x\n", pdu_length);
> > + cifs_dbg(FYI, "RFC1002 header 0x%x\n", server->total_size);
> > if (!is_smb_response(server, buf[0]))
> > continue;
> > +next_pdu:
> > + server->total_size = pdu_length;
>
> You set server->total_size *after* printing it.
>
> =========
>
>
> After the rebase I've tried testing mounting a WS2016 share and listing
> the root. If the correct files are listed its OK anything else is ERR:
>
> vers=1.0 ... OK
> vers=2.0 ... OK
> vers=2.0,sec=ntlmsspi ... ERR
> vers=2.1 ... OK
> vers=2.1,sec=ntlmsspi ... ERR
> vers=3.0 ... OK
> vers=3.0,sec=ntlmsspi ... ERR
> vers=3.0,seal ... OK
> vers=3.0,seal,sec=ntlmsspi ... OK
> vers=3.11 ... ERR
> vers=3.11,sec=ntlmsspi ... ERR
> vers=3.11,seal ... ERR
> vers=3.11,seal,sec=ntlmsspi ... ERR
>
> Most of the ntlmsspi errors (thats when we require signing) are due to
> bad signing on certain packets (ones triggered by ls):
>
> Find Request File: SMB2_FIND_ID_FULL_DIRECTORY_INFO Pattern: *
> Find Response, Error: STATUS_ACCESS_DENIED
>
The signing is a bug in __cifs_calc_signature() where I did not fix the check for too short iovs :
if (... && iov[1].iov_len <= 4)
break; /* nothing to sign or corrupt header */
I have fixed that.
> (if access is ok without signing -> error due to wrong sig)
>
> Weirdly enough, encryption+signingon 3.0 works.
>
> As for SMB311, mount fails and I see lengths issues
>
> > fs/cifs/transport.c: Sending smb: smb_len=170
> > fs/cifs/connect.c: RFC1002 header 0x0
> > fs/cifs/smb2misc.c: SMB2 data length 120 offset 128
> > fs/cifs/smb2misc.c: SMB2 len 248
> > fs/cifs/smb2misc.c: Calculated size 248 length 308 mismatch mid 0
> > SMB2 server sent bad RFC1001 len 308 not 248
>
> And it does weird things from there.
>
> I can provide full kernel debug log and network captures each of the
> run.
>
> I've attached my rebased patches (I've added my sign-off to patches I
> had to modify to apply or added changes to).
>
> Cheers,
>
> --
> Aurélien Aptel / SUSE Labs Samba Team
> GPG: 1839 CB5F 9F5B FB9B AA97 8C99 03C8 A49B 521B D5D3
> SUSE Linux GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
> GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
>
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
