Re: [PATCH v2 07/24] CIFS: Make demultiplex_thread work with SMB2 code

[Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>]

On Fri, 22 Jun 2012 16:55:13 -0500
Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> On Fri, Jun 22, 2012 at 9:59 AM, Pavel Shilovsky <pshilovsky-eUNUBHrolfY@public.gmane.orgg> wrote:
> > 2012/6/21 Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>:
> >> On Wed, 20 Jun 2012 18:30:47 +0400
> >> Pavel Shilovsky <pshilovsky-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org> wrote:
> >>
> >>> From: Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> >>>
> >>> Now we can process SMB2 messages: check message, get message id
> >>> and wakeup awaiting routines.
> >>>
> >>> Signed-off-by: Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> >>> ---
> >>>  fs/cifs/Makefile     |    2 +-
> >>>  fs/cifs/cifs_debug.c |    2 +-
> >>>  fs/cifs/smb2misc.c   |  304 ++++++++++++++++++++++++++++++++++++++++++++++++++
> >>>  fs/cifs/smb2ops.c    |   37 ++++++
> >>>  fs/cifs/smb2pdu.h    |   30 +++++
> >>>  fs/cifs/smb2proto.h  |    2 +
> >>>  6 files changed, 375 insertions(+), 2 deletions(-)
> >>>  create 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) += dns_resolve.o cifs_dfs_ref.o
> >>>
> >>>  cifs-$(CONFIG_CIFS_FSCACHE) += fscache.o cache.o
> >>>
> >>> -cifs-$(CONFIG_CIFS_SMB2) += smb2ops.o smb2maperror.o smb2transport.o
> >>> +cifs-$(CONFIG_CIFS_SMB2) += smb2ops.o smb2maperror.o smb2transport.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)
> >>>       cERROR(1, "Cmd: %d Err: 0x%x Flags: 0x%x Flgs2: 0x%x Mid: %d Pid: %d",
> >>>                 smb->Command, smb->Status.CifsError,
> >>>                 smb->Flags, smb->Flags2, smb->Mid, smb->Pid);
> >>> -     cERROR(1, "smb buf %p len %d", smb, smbCalcSize(smb));
> >>> +     cERROR(1, "smb buf %p len %u", smb, smbCalcSize(smb));
> >>>  #endif /* CONFIG_CIFS_DEBUG2 */
> >>>  }
> >>>
> >>> 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 @@
> >>> +/*
> >>> + *   fs/cifs/smb2misc.c
> >>> + *
> >>> + *   Copyright (C) International Business Machines  Corp., 2002,2011
> >>> + *                 Etersoft, 2012
> >>> + *   Author(s): Steve French (sfrench-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org)
> >>> + *              Pavel Shilovsky (pshilovsky@samba.org) 2012
> >>> + *
> >>> + *   This library is free software; you can redistribute it and/or modify
> >>> + *   it under the terms of the GNU Lesser General Public License as published
> >>> + *   by the Free Software Foundation; either version 2.1 of the License, or
> >>> + *   (at your option) any later version.
> >>> + *
> >>> + *   This library is distributed in the hope that it will be useful,
> >>> + *   but WITHOUT ANY WARRANTY; without even the implied warranty of
> >>> + *   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See
> >>> + *   the GNU Lesser General Public License for more details.
> >>> + *
> >>> + *   You should have received a copy of the GNU Lesser General Public License
> >>> + *   along with this library; if not, write to the Free Software
> >>> + *   Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
> >>> + */
> >>> +#include <linux/ctype.h>
> >>> +#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)
> >>> +{
> >>> +     /*
> >>> +      * Make sure that this really is an SMB, that it is a response,
> >>> +      * and that the message ids match
> >>> +      */
> >>> +     if ((*(__le32 *)hdr->ProtocolId == cpu_to_le32(0x424d53fe)) &&
> >>
> >>                        ... == __constant_cpu_to_le32()
> >>
> >>                It would also be good to turn that into a #define'ed constant and use it in the check below.
> >>
> >>> +         (mid == hdr->MessageId)) {
> >>> +             if (hdr->Flags & SMB2_FLAGS_SERVER_TO_REDIR)
> >>> +                     return 0;
> >>> +             else {
> >>> +                     /* only one valid case where server sends us request */
> >>> +                     if (hdr->Command == SMB2_OPLOCK_BREAK)
> >>> +                             return 0;
> >>> +                     else
> >>> +                             cERROR(1, "Received Request not response");
> >>> +             }
> >>> +     } else { /* bad signature or mid */
> >>> +             if (*(__le32 *)hdr->ProtocolId != cpu_to_le32(0x424d53fe))
> >>> +                     cERROR(1, "Bad protocol string signature header %x",
> >>> +                            *(unsigned int *) hdr->ProtocolId);
> >>> +             if (mid != hdr->MessageId)
> >>> +                     cERROR(1, "Mids do not match");
> >>> +     }
> >>> +     cERROR(1, "Bad SMB detected. The Mid=%llu", hdr->MessageId);
> >>> +     return 1;
> >>> +}
> >>> +
> >>> +/*
> >>> + *  The following table defines the expected "StructureSize" of SMB2 responses
> >>> + *  in order by SMB2 command.  This is similar to "wct" in SMB/CIFS responses.
> >>> + *
> >>> + *  Note that commands are defined in smb2pdu.h in le16 but the array below is
> >>> + *  indexed by command in host byte order
> >>> + */
> >>> +static const int smb2_rsp_struct_sizes[NUMBER_OF_SMB2_COMMANDS] = {
> >>> +     /* SMB2_NEGOTIATE */ 65,
> >>> +     /* SMB2_SESSION_SETUP */ 9,
> >>> +     /* SMB2_LOGOFF */ 4,
> >>> +     /* SMB2_TREE_CONNECT */ 16,
> >>> +     /* SMB2_TREE_DISCONNECT */ 4,
> >>> +     /* SMB2_CREATE */ 89,
> >>> +     /* SMB2_CLOSE */ 60,
> >>> +     /* SMB2_FLUSH */ 4,
> >>> +     /* SMB2_READ */ 17,
> >>> +     /* SMB2_WRITE */ 17,
> >>> +     /* SMB2_LOCK */ 4,
> >>> +     /* SMB2_IOCTL */ 49,
> >>> +     /* SMB2_CANCEL */ 0, /* BB CHECK this ... not listed in documentation */
> >>> +     /* SMB2_ECHO */ 4,
> >>> +     /* SMB2_QUERY_DIRECTORY */ 9,
> >>> +     /* SMB2_CHANGE_NOTIFY */ 9,
> >>> +     /* SMB2_QUERY_INFO */ 9,
> >>> +     /* SMB2_SET_INFO */ 2,
> >>> +     /* SMB2_OPLOCK_BREAK */ 24 /* BB FIXME can also be 44 for 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)
> >>> +{
> >>> +     struct smb2_hdr *hdr = (struct smb2_hdr *)buf;
> >>> +     struct smb2_pdu *pdu = (struct smb2_pdu *)hdr;
> >>> +     __u64 mid = hdr->MessageId;
> >>> +     __u32 len = get_rfc1002_length(buf);
> >>> +     __u32 clc_len;  /* calculated length */
> >>> +     int command;
> >>> +
> >>> +     /* BB disable following printk later */
> >>> +     cFYI(1, "checkSMB Length: 0x%x, smb_buf_length: 0x%x", length, len);
> >>> +
> >>> +     /*
> >>> +      * Add function to do table lookup of StructureSize by command
> >>> +      * ie Validate the wct via smb2_struct_sizes table above
> >>> +      */
> >>> +
> >>> +     if (length < 2 + sizeof(struct smb2_hdr)) {
> >>> +             if ((length >= sizeof(struct smb2_hdr)) && (hdr->Status != 0)) {
> >>> +                     pdu->StructureSize2 = 0;
> >>> +                     /*
> >>> +                      * As with SMB/CIFS, on some error cases servers may
> >>> +                      * not return wct properly
> >>> +                      */
> >>> +                     return 0;
> >>> +             } else {
> >>> +                     cERROR(1, "Length less than smb header size");
> >>> +             }
> >>> +             return 1;
> >>> +     }
> >>> +     if (len > CIFSMaxBufSize + MAX_SMB2_HDR_SIZE - 4) {
> >>> +             cERROR(1, "smb length greater than maximum, mid=%lld", mid);
> >>> +             return 1;
> >>> +     }
> >>> +
> >>> +     if (check_smb2_hdr(hdr, mid))
> >>> +             return 1;
> >>> +
> >>> +     if (le16_to_cpu(hdr->StructureSize) != 64) {
> >>> +             cERROR(1, "Illegal structure size %d",
> >>> +                       le16_to_cpu(hdr->StructureSize));
> >>> +             return 1;
> >>> +     }
> >>> +
> >>> +     command = le16_to_cpu(hdr->Command);
> >>> +     if (command >= NUMBER_OF_SMB2_COMMANDS) {
> >>> +             cERROR(1, "illegal SMB2 command %d", command);
> >>> +             return 1;
> >>> +     }
> >>> +
> >>> +     if (smb2_rsp_struct_sizes[command] !=
> >>> +         le16_to_cpu(pdu->StructureSize2)) {
> >>> +             if ((hdr->Status == 0) ||
> >>> +                 (le16_to_cpu(pdu->StructureSize2) != 9)) {
> >> The 9 here should probably be a preprocessor constant:
> >>
> >>    #define SMB2_ERROR_STRUCTURE_SIZE2  __constant_le16_to_cpu(9)
> >>
> >> Then you could just compare that to a bare pdu->StructureSize2
> >>
> >>> +                     /* error packets have 9 byte structure size */
> >>> +                     cERROR(1, "Illegal response size %d for command %d",
> >>> +                                le16_to_cpu(pdu->StructureSize2), command);
> >>> +                     return 1;
> >>> +             }
> >>> +     }
> >>> +
> >>> +     clc_len = smb2_calc_size(hdr);
> >>> +
> >>> +     if (4 + len != length) {
> >>> +             cERROR(1, "Length read does not match RFC1001 length %d",
> >>> +                        len);
> >>> +             return 1;
> >>> +     }
> >>
> >>
> >> Does RFC1001 apply here? The error message above could be clearer.
> >>
> >>> +
> >>> +     if (4 + len != clc_len) {
> >>> +             cFYI(1, "Calculated size %d length %d mismatch for mid %lld",
> >>> +                      clc_len, 4 + len, mid);
> >>> +             if (clc_len == 4 + len + 1) /* BB FIXME (fix samba) */
> >>> +                     return 0; /* BB workaround Samba 3 bug SessSetup rsp */
> >>> +             return 1;
> >>> +     }
> >>> +     return 0;
> >>> +}
> >>> +
> >>> +/*
> >>> + *  The size of the variable area depends on the offset and length fields
> >>> + *  located in different fields for various SMB2 responses.  SMB2 responses
> >>> + *  with no variable length info, show an offset of zero for the offset field.
> >>> + */
> >>> +static const bool has_smb2_data_area[NUMBER_OF_SMB2_COMMANDS] = {
> >>> +     /* SMB2_NEGOTIATE */ true,
> >>> +     /* SMB2_SESSION_SETUP */ true,
> >>> +     /* SMB2_LOGOFF */ false,
> >>> +     /* SMB2_TREE_CONNECT */ false,
> >>> +     /* SMB2_TREE_DISCONNECT */ false,
> >>> +     /* SMB2_CREATE */ true,
> >>> +     /* SMB2_CLOSE */ false,
> >>> +     /* SMB2_FLUSH */ false,
> >>> +     /* SMB2_READ */ true,
> >>> +     /* SMB2_WRITE */ false,
> >>> +     /* SMB2_LOCK */ false,
> >>> +     /* SMB2_IOCTL */ true,
> >>> +     /* SMB2_CANCEL */ false, /* BB CHECK this not listed in documentation */
> >>> +     /* SMB2_ECHO */ false,
> >>> +     /* SMB2_QUERY_DIRECTORY */ true,
> >>> +     /* SMB2_CHANGE_NOTIFY */ true,
> >>> +     /* SMB2_QUERY_INFO */ true,
> >>> +     /* SMB2_SET_INFO */ false,
> >>> +     /* 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)
> >>> +{
> >>> +     *off = 0;
> >>> +     *len = 0;
> >>> +
> >>> +     /* error responses do not have data area */
> >>> +     if (hdr->Status &&
> >>> +        (le32_to_cpu(hdr->Status) != STATUS_MORE_PROCESSING_REQUIRED) &&
> >>
> >> I wonder...should we also convert the STATUS_* constants to LE values?
> >> That would save a few cycles on BE machines...
> >
> > In this case we should probably convert nterr.h too -- Steve, what do you think?
> 
> 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
> 


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,
-- 
Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>