From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jeff Layton Subject: Re: [PATCH v2 07/24] CIFS: Make demultiplex_thread work with SMB2 code Date: Fri, 22 Jun 2012 19:10:16 -0700 Message-ID: <20120622191016.1cf69fa4@corrin.poochiereds.net> References: <1340202664-28696-1-git-send-email-pshilovsky@samba.org> <1340202664-28696-8-git-send-email-pshilovsky@samba.org> <20120621114437.72a9483d@corrin.poochiereds.net> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: QUOTED-PRINTABLE Cc: Pavel Shilovsky , linux-cifs-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: Steve French Return-path: In-Reply-To: Sender: linux-cifs-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org List-ID: On Fri, 22 Jun 2012 16:55:13 -0500 Steve French wrote: > On Fri, Jun 22, 2012 at 9:59 AM, Pavel Shilovsky wrote: > > 2012/6/21 Jeff Layton : > >> On Wed, 20 Jun 2012 18:30:47 +0400 > >> Pavel Shilovsky wrote: > >> > >>> From: Pavel Shilovsky > >>> > >>> Now we can process SMB2 messages: check message, get message id > >>> and wakeup awaiting routines. > >>> > >>> Signed-off-by: Pavel Shilovsky > >>> --- > >>> =A0fs/cifs/Makefile =A0 =A0 | =A0 =A02 +- > >>> =A0fs/cifs/cifs_debug.c | =A0 =A02 +- > >>> =A0fs/cifs/smb2misc.c =A0 | =A0304 ++++++++++++++++++++++++++++++= ++++++++++++++++++++ > >>> =A0fs/cifs/smb2ops.c =A0 =A0| =A0 37 ++++++ > >>> =A0fs/cifs/smb2pdu.h =A0 =A0| =A0 30 +++++ > >>> =A0fs/cifs/smb2proto.h =A0| =A0 =A02 + > >>> =A06 files changed, 375 insertions(+), 2 deletions(-) > >>> =A0create mode 100644 fs/cifs/smb2misc.c > >>> > >>> diff --git a/fs/cifs/Makefile b/fs/cifs/Makefile > >>> index a73d7f8..b77e9ec 100644 > >>> --- a/fs/cifs/Makefile > >>> +++ b/fs/cifs/Makefile > >>> @@ -16,4 +16,4 @@ cifs-$(CONFIG_CIFS_DFS_UPCALL) +=3D dns_resolve= =2Eo cifs_dfs_ref.o > >>> > >>> =A0cifs-$(CONFIG_CIFS_FSCACHE) +=3D fscache.o cache.o > >>> > >>> -cifs-$(CONFIG_CIFS_SMB2) +=3D smb2ops.o smb2maperror.o smb2trans= port.o > >>> +cifs-$(CONFIG_CIFS_SMB2) +=3D smb2ops.o smb2maperror.o smb2trans= port.o smb2misc.o > >>> diff --git a/fs/cifs/cifs_debug.c b/fs/cifs/cifs_debug.c > >>> index e814052..8aa8693 100644 > >>> --- a/fs/cifs/cifs_debug.c > >>> +++ b/fs/cifs/cifs_debug.c > >>> @@ -65,7 +65,7 @@ void cifs_dump_detail(void *buf) > >>> =A0 =A0 =A0 cERROR(1, "Cmd: %d Err: 0x%x Flags: 0x%x Flgs2: 0x%x = Mid: %d Pid: %d", > >>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 smb->Command, smb->Status.CifsErr= or, > >>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 smb->Flags, smb->Flags2, smb->Mid= , smb->Pid); > >>> - =A0 =A0 cERROR(1, "smb buf %p len %d", smb, smbCalcSize(smb)); > >>> + =A0 =A0 cERROR(1, "smb buf %p len %u", smb, smbCalcSize(smb)); > >>> =A0#endif /* CONFIG_CIFS_DEBUG2 */ > >>> =A0} > >>> > >>> diff --git a/fs/cifs/smb2misc.c b/fs/cifs/smb2misc.c > >>> new file mode 100644 > >>> index 0000000..03167eb > >>> --- /dev/null > >>> +++ b/fs/cifs/smb2misc.c > >>> @@ -0,0 +1,304 @@ > >>> +/* > >>> + * =A0 fs/cifs/smb2misc.c > >>> + * > >>> + * =A0 Copyright (C) International Business Machines =A0Corp., 2= 002,2011 > >>> + * =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 Etersoft, 2012 > >>> + * =A0 Author(s): Steve French (sfrench-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org) > >>> + * =A0 =A0 =A0 =A0 =A0 =A0 =A0Pavel Shilovsky (pshilovsky@samba.= org) 2012 > >>> + * > >>> + * =A0 This library is free software; you can redistribute it an= d/or modify > >>> + * =A0 it under the terms of the GNU Lesser General Public Licen= se as published > >>> + * =A0 by the Free Software Foundation; either version 2.1 of th= e License, or > >>> + * =A0 (at your option) any later version. > >>> + * > >>> + * =A0 This library is distributed in the hope that it will be u= seful, > >>> + * =A0 but WITHOUT ANY WARRANTY; without even the implied warran= ty of > >>> + * =A0 MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. =A0S= ee > >>> + * =A0 the GNU Lesser General Public License for more details. > >>> + * > >>> + * =A0 You should have received a copy of the GNU Lesser General= Public License > >>> + * =A0 along with this library; if not, write to the Free Softwa= re > >>> + * =A0 Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA = 02111-1307 USA > >>> + */ > >>> +#include > >>> +#include "smb2pdu.h" > >>> +#include "cifsglob.h" > >>> +#include "cifsproto.h" > >>> +#include "smb2proto.h" > >>> +#include "cifs_debug.h" > >>> +#include "cifs_unicode.h" > >>> +#include "smb2status.h" > >>> + > >>> +static int > >>> +check_smb2_hdr(struct smb2_hdr *hdr, __u64 mid) > >>> +{ > >>> + =A0 =A0 /* > >>> + =A0 =A0 =A0* Make sure that this really is an SMB, that it is a= response, > >>> + =A0 =A0 =A0* and that the message ids match > >>> + =A0 =A0 =A0*/ > >>> + =A0 =A0 if ((*(__le32 *)hdr->ProtocolId =3D=3D cpu_to_le32(0x42= 4d53fe)) && > >> > >> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0... =3D=3D __consta= nt_cpu_to_le32() > >> > >> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0It would also be good to turn that = into a #define'ed constant and use it in the check below. > >> > >>> + =A0 =A0 =A0 =A0 (mid =3D=3D hdr->MessageId)) { > >>> + =A0 =A0 =A0 =A0 =A0 =A0 if (hdr->Flags & SMB2_FLAGS_SERVER_TO_R= EDIR) > >>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 return 0; > >>> + =A0 =A0 =A0 =A0 =A0 =A0 else { > >>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 /* only one valid case = where server sends us request */ > >>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (hdr->Command =3D=3D= SMB2_OPLOCK_BREAK) > >>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 return = 0; > >>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 else > >>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 cERROR(= 1, "Received Request not response"); > >>> + =A0 =A0 =A0 =A0 =A0 =A0 } > >>> + =A0 =A0 } else { /* bad signature or mid */ > >>> + =A0 =A0 =A0 =A0 =A0 =A0 if (*(__le32 *)hdr->ProtocolId !=3D cpu= _to_le32(0x424d53fe)) > >>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 cERROR(1, "Bad protocol= string signature header %x", > >>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0*(unsign= ed int *) hdr->ProtocolId); > >>> + =A0 =A0 =A0 =A0 =A0 =A0 if (mid !=3D hdr->MessageId) > >>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 cERROR(1, "Mids do not = match"); > >>> + =A0 =A0 } > >>> + =A0 =A0 cERROR(1, "Bad SMB detected. The Mid=3D%llu", hdr->Mess= ageId); > >>> + =A0 =A0 return 1; > >>> +} > >>> + > >>> +/* > >>> + * =A0The following table defines the expected "StructureSize" o= f SMB2 responses > >>> + * =A0in order by SMB2 command. =A0This is similar to "wct" in S= MB/CIFS responses. > >>> + * > >>> + * =A0Note that commands are defined in smb2pdu.h in le16 but th= e array below is > >>> + * =A0indexed by command in host byte order > >>> + */ > >>> +static const int smb2_rsp_struct_sizes[NUMBER_OF_SMB2_COMMANDS] = =3D { > >>> + =A0 =A0 /* SMB2_NEGOTIATE */ 65, > >>> + =A0 =A0 /* SMB2_SESSION_SETUP */ 9, > >>> + =A0 =A0 /* SMB2_LOGOFF */ 4, > >>> + =A0 =A0 /* SMB2_TREE_CONNECT */ 16, > >>> + =A0 =A0 /* SMB2_TREE_DISCONNECT */ 4, > >>> + =A0 =A0 /* SMB2_CREATE */ 89, > >>> + =A0 =A0 /* SMB2_CLOSE */ 60, > >>> + =A0 =A0 /* SMB2_FLUSH */ 4, > >>> + =A0 =A0 /* SMB2_READ */ 17, > >>> + =A0 =A0 /* SMB2_WRITE */ 17, > >>> + =A0 =A0 /* SMB2_LOCK */ 4, > >>> + =A0 =A0 /* SMB2_IOCTL */ 49, > >>> + =A0 =A0 /* SMB2_CANCEL */ 0, /* BB CHECK this ... not listed in= documentation */ > >>> + =A0 =A0 /* SMB2_ECHO */ 4, > >>> + =A0 =A0 /* SMB2_QUERY_DIRECTORY */ 9, > >>> + =A0 =A0 /* SMB2_CHANGE_NOTIFY */ 9, > >>> + =A0 =A0 /* SMB2_QUERY_INFO */ 9, > >>> + =A0 =A0 /* SMB2_SET_INFO */ 2, > >>> + =A0 =A0 /* SMB2_OPLOCK_BREAK */ 24 /* BB FIXME can also be 44 f= or lease break */ > >>> +}; > >>> + > >> > >> It might save a few cycles (and memory) to make the above an array= of > >> __le16's and go ahead and __constant_cpu_to_le16 the values. Then = you > >> wouldn't need to convert the values in the headers below. > > > > Yes, make sense. > > > >> > >>> +int > >>> +smb2_check_message(char *buf, unsigned int length) > >>> +{ > >>> + =A0 =A0 struct smb2_hdr *hdr =3D (struct smb2_hdr *)buf; > >>> + =A0 =A0 struct smb2_pdu *pdu =3D (struct smb2_pdu *)hdr; > >>> + =A0 =A0 __u64 mid =3D hdr->MessageId; > >>> + =A0 =A0 __u32 len =3D get_rfc1002_length(buf); > >>> + =A0 =A0 __u32 clc_len; =A0/* calculated length */ > >>> + =A0 =A0 int command; > >>> + > >>> + =A0 =A0 /* BB disable following printk later */ > >>> + =A0 =A0 cFYI(1, "checkSMB Length: 0x%x, smb_buf_length: 0x%x", = length, len); > >>> + > >>> + =A0 =A0 /* > >>> + =A0 =A0 =A0* Add function to do table lookup of StructureSize b= y command > >>> + =A0 =A0 =A0* ie Validate the wct via smb2_struct_sizes table ab= ove > >>> + =A0 =A0 =A0*/ > >>> + > >>> + =A0 =A0 if (length < 2 + sizeof(struct smb2_hdr)) { > >>> + =A0 =A0 =A0 =A0 =A0 =A0 if ((length >=3D sizeof(struct smb2_hdr= )) && (hdr->Status !=3D 0)) { > >>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 pdu->StructureSize2 =3D= 0; > >>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 /* > >>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0* As with SMB/CIFS, = on some error cases servers may > >>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0* not return wct pro= perly > >>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0*/ > >>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 return 0; > >>> + =A0 =A0 =A0 =A0 =A0 =A0 } else { > >>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 cERROR(1, "Length less = than smb header size"); > >>> + =A0 =A0 =A0 =A0 =A0 =A0 } > >>> + =A0 =A0 =A0 =A0 =A0 =A0 return 1; > >>> + =A0 =A0 } > >>> + =A0 =A0 if (len > CIFSMaxBufSize + MAX_SMB2_HDR_SIZE - 4) { > >>> + =A0 =A0 =A0 =A0 =A0 =A0 cERROR(1, "smb length greater than maxi= mum, mid=3D%lld", mid); > >>> + =A0 =A0 =A0 =A0 =A0 =A0 return 1; > >>> + =A0 =A0 } > >>> + > >>> + =A0 =A0 if (check_smb2_hdr(hdr, mid)) > >>> + =A0 =A0 =A0 =A0 =A0 =A0 return 1; > >>> + > >>> + =A0 =A0 if (le16_to_cpu(hdr->StructureSize) !=3D 64) { > >>> + =A0 =A0 =A0 =A0 =A0 =A0 cERROR(1, "Illegal structure size %d", > >>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 le16_to_cpu(hdr->St= ructureSize)); > >>> + =A0 =A0 =A0 =A0 =A0 =A0 return 1; > >>> + =A0 =A0 } > >>> + > >>> + =A0 =A0 command =3D le16_to_cpu(hdr->Command); > >>> + =A0 =A0 if (command >=3D NUMBER_OF_SMB2_COMMANDS) { > >>> + =A0 =A0 =A0 =A0 =A0 =A0 cERROR(1, "illegal SMB2 command %d", co= mmand); > >>> + =A0 =A0 =A0 =A0 =A0 =A0 return 1; > >>> + =A0 =A0 } > >>> + > >>> + =A0 =A0 if (smb2_rsp_struct_sizes[command] !=3D > >>> + =A0 =A0 =A0 =A0 le16_to_cpu(pdu->StructureSize2)) { > >>> + =A0 =A0 =A0 =A0 =A0 =A0 if ((hdr->Status =3D=3D 0) || > >>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 (le16_to_cpu(pdu->StructureSize= 2) !=3D 9)) { > >> The 9 here should probably be a preprocessor constant: > >> > >> =A0 =A0#define SMB2_ERROR_STRUCTURE_SIZE2 =A0__constant_le16_to_cp= u(9) > >> > >> Then you could just compare that to a bare pdu->StructureSize2 > >> > >>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 /* error packets have 9= byte structure size */ > >>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 cERROR(1, "Illegal resp= onse size %d for command %d", > >>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0= le16_to_cpu(pdu->StructureSize2), command); > >>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 return 1; > >>> + =A0 =A0 =A0 =A0 =A0 =A0 } > >>> + =A0 =A0 } > >>> + > >>> + =A0 =A0 clc_len =3D smb2_calc_size(hdr); > >>> + > >>> + =A0 =A0 if (4 + len !=3D length) { > >>> + =A0 =A0 =A0 =A0 =A0 =A0 cERROR(1, "Length read does not match R= =46C1001 length %d", > >>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0len); > >>> + =A0 =A0 =A0 =A0 =A0 =A0 return 1; > >>> + =A0 =A0 } > >> > >> > >> Does RFC1001 apply here? The error message above could be clearer. > >> > >>> + > >>> + =A0 =A0 if (4 + len !=3D clc_len) { > >>> + =A0 =A0 =A0 =A0 =A0 =A0 cFYI(1, "Calculated size %d length %d m= ismatch for mid %lld", > >>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0clc_len, 4 + len, mi= d); > >>> + =A0 =A0 =A0 =A0 =A0 =A0 if (clc_len =3D=3D 4 + len + 1) /* BB F= IXME (fix samba) */ > >>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 return 0; /* BB workaro= und Samba 3 bug SessSetup rsp */ > >>> + =A0 =A0 =A0 =A0 =A0 =A0 return 1; > >>> + =A0 =A0 } > >>> + =A0 =A0 return 0; > >>> +} > >>> + > >>> +/* > >>> + * =A0The size of the variable area depends on the offset and le= ngth fields > >>> + * =A0located in different fields for various SMB2 responses. =A0= SMB2 responses > >>> + * =A0with no variable length info, show an offset of zero for t= he offset field. > >>> + */ > >>> +static const bool has_smb2_data_area[NUMBER_OF_SMB2_COMMANDS] =3D= { > >>> + =A0 =A0 /* SMB2_NEGOTIATE */ true, > >>> + =A0 =A0 /* SMB2_SESSION_SETUP */ true, > >>> + =A0 =A0 /* SMB2_LOGOFF */ false, > >>> + =A0 =A0 /* SMB2_TREE_CONNECT */ false, > >>> + =A0 =A0 /* SMB2_TREE_DISCONNECT */ false, > >>> + =A0 =A0 /* SMB2_CREATE */ true, > >>> + =A0 =A0 /* SMB2_CLOSE */ false, > >>> + =A0 =A0 /* SMB2_FLUSH */ false, > >>> + =A0 =A0 /* SMB2_READ */ true, > >>> + =A0 =A0 /* SMB2_WRITE */ false, > >>> + =A0 =A0 /* SMB2_LOCK */ false, > >>> + =A0 =A0 /* SMB2_IOCTL */ true, > >>> + =A0 =A0 /* SMB2_CANCEL */ false, /* BB CHECK this not listed in= documentation */ > >>> + =A0 =A0 /* SMB2_ECHO */ false, > >>> + =A0 =A0 /* SMB2_QUERY_DIRECTORY */ true, > >>> + =A0 =A0 /* SMB2_CHANGE_NOTIFY */ true, > >>> + =A0 =A0 /* SMB2_QUERY_INFO */ true, > >>> + =A0 =A0 /* SMB2_SET_INFO */ false, > >>> + =A0 =A0 /* SMB2_OPLOCK_BREAK */ false > >>> +}; > >>> + > >>> +/* > >>> + * Returns the pointer to the beginning of the data area. Length= of the data > >>> + * area and the offset to it (from the beginning of the smb are = also returned. > >>> + */ > >>> +static char * > >>> +smb2_get_data_area_len(int *off, int *len, struct smb2_hdr *hdr) > >>> +{ > >>> + =A0 =A0 *off =3D 0; > >>> + =A0 =A0 *len =3D 0; > >>> + > >>> + =A0 =A0 /* error responses do not have data area */ > >>> + =A0 =A0 if (hdr->Status && > >>> + =A0 =A0 =A0 =A0(le32_to_cpu(hdr->Status) !=3D STATUS_MORE_PROCE= SSING_REQUIRED) && > >> > >> I wonder...should we also convert the STATUS_* constants to LE val= ues? > >> That would save a few cycles on BE machines... > > > > In this case we should probably convert nterr.h too -- Steve, what = do you think? >=20 > It is probably fine if it makes the cifs code a little cleaner, but I > am more concerned about optimizing the smb2.1/smb3 code than > optimizing the cifs code at this point. I don't >=20 Yeah, agreed. I wouldn't mess with smb1 at this point for this, let's focus on fixing up the smb2 code. If we need to make changes to the smb1 code in order to facilitate the smb2 changes then that's fine, but I'd refrain from doing anything unnecessary to the smb1 code at this point. Cheers, --=20 Jeff Layton