Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <1585696903-96794-1-git-send-email-longli@linuxonhyperv.com>
In-Reply-To: <1585696903-96794-1-git-send-email-longli@linuxonhyperv.com>
From:   Pavel Shilovsky <piastryyy@gmail.com>
Date:   Fri, 3 Apr 2020 10:45:35 -0700
Message-ID: <CAKywueRg8kJ+0aOehM-QKGuRwbDSb2TA5vNje8eSCdMqBT+EdQ@mail.gmail.com>
Subject: Re: [PATCH] cifs: Allocate crypto structures on the fly for
 calculating signatures of incoming packets
To:     Long Li <longli@microsoft.com>
Cc:     Steve French <sfrench@samba.org>,
        linux-cifs <linux-cifs@vger.kernel.org>,
        samba-technical <samba-technical@lists.samba.org>,
        Kernel Mailing List <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

=D0=B2=D1=82, 31 =D0=BC=D0=B0=D1=80. 2020 =D0=B3. =D0=B2 16:22, <longli@lin=
uxonhyperv.com>:
>
> From: Long Li <longli@microsoft.com>
>
> CIFS uses pre-allocated crypto structures to calculate signatures for bot=
h
> incoming and outgoing packets. In this way it doesn't need to allocate cr=
ypto
> structures for every packet, but it requires a lock to prevent concurrent
> access to crypto structures.
>
> Remove the lock by allocating crypto structures on the fly for
> incoming packets. At the same time, we can still use pre-allocated crypto
> structures for outgoing packets, as they are already protected by transpo=
rt
> lock srv_mutex.
>
> Signed-off-by: Long Li <longli@microsoft.com>
> ---
>  fs/cifs/cifsglob.h      |  3 +-
>  fs/cifs/smb2proto.h     |  6 ++-
>  fs/cifs/smb2transport.c | 87 +++++++++++++++++++++++++----------------
>  3 files changed, 60 insertions(+), 36 deletions(-)
>
> diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
> index 0d956360e984..7448e7202e7a 100644
> --- a/fs/cifs/cifsglob.h
> +++ b/fs/cifs/cifsglob.h
> @@ -426,7 +426,8 @@ struct smb_version_operations {
>         /* generate new lease key */
>         void (*new_lease_key)(struct cifs_fid *);
>         int (*generate_signingkey)(struct cifs_ses *);
> -       int (*calc_signature)(struct smb_rqst *, struct TCP_Server_Info *=
);
> +       int (*calc_signature)(struct smb_rqst *, struct TCP_Server_Info *=
,
> +                               bool allocate_crypto);
>         int (*set_integrity)(const unsigned int, struct cifs_tcon *tcon,
>                              struct cifsFileInfo *src_file);
>         int (*enum_snapshots)(const unsigned int xid, struct cifs_tcon *t=
con,
> diff --git a/fs/cifs/smb2proto.h b/fs/cifs/smb2proto.h
> index 4d1ff7b66fdc..087d5f14320b 100644
> --- a/fs/cifs/smb2proto.h
> +++ b/fs/cifs/smb2proto.h
> @@ -55,9 +55,11 @@ extern struct cifs_ses *smb2_find_smb_ses(struct TCP_S=
erver_Info *server,
>  extern struct cifs_tcon *smb2_find_smb_tcon(struct TCP_Server_Info *serv=
er,
>                                                 __u64 ses_id, __u32  tid)=
;
>  extern int smb2_calc_signature(struct smb_rqst *rqst,
> -                               struct TCP_Server_Info *server);
> +                               struct TCP_Server_Info *server,
> +                               bool allocate_crypto);
>  extern int smb3_calc_signature(struct smb_rqst *rqst,
> -                               struct TCP_Server_Info *server);
> +                               struct TCP_Server_Info *server,
> +                               bool allocate_crypto);
>  extern void smb2_echo_request(struct work_struct *work);
>  extern __le32 smb2_get_lease_state(struct cifsInodeInfo *cinode);
>  extern bool smb2_is_valid_oplock_break(char *buffer,
> diff --git a/fs/cifs/smb2transport.c b/fs/cifs/smb2transport.c
> index 08b703b7a15e..c01e19a3b112 100644
> --- a/fs/cifs/smb2transport.c
> +++ b/fs/cifs/smb2transport.c
> @@ -40,14 +40,6 @@
>  #include "smb2status.h"
>  #include "smb2glob.h"
>
> -static int
> -smb2_crypto_shash_allocate(struct TCP_Server_Info *server)
> -{
> -       return cifs_alloc_hash("hmac(sha256)",
> -                              &server->secmech.hmacsha256,
> -                              &server->secmech.sdeschmacsha256);
> -}
> -
>  static int
>  smb3_crypto_shash_allocate(struct TCP_Server_Info *server)
>  {
> @@ -219,7 +211,8 @@ smb2_find_smb_tcon(struct TCP_Server_Info *server, __=
u64 ses_id, __u32  tid)
>  }
>
>  int
> -smb2_calc_signature(struct smb_rqst *rqst, struct TCP_Server_Info *serve=
r)
> +smb2_calc_signature(struct smb_rqst *rqst, struct TCP_Server_Info *serve=
r,
> +                       bool allocate_crypto)
>  {
>         int rc;
>         unsigned char smb2_signature[SMB2_HMACSHA256_SIZE];
> @@ -228,6 +221,8 @@ smb2_calc_signature(struct smb_rqst *rqst, struct TCP=
_Server_Info *server)
>         struct smb2_sync_hdr *shdr =3D (struct smb2_sync_hdr *)iov[0].iov=
_base;
>         struct cifs_ses *ses;
>         struct shash_desc *shash;
> +       struct crypto_shash *hash;
> +       struct sdesc *sdesc =3D NULL;
>         struct smb_rqst drqst;
>
>         ses =3D smb2_find_smb_ses(server, shdr->SessionId);
> @@ -239,24 +234,32 @@ smb2_calc_signature(struct smb_rqst *rqst, struct T=
CP_Server_Info *server)
>         memset(smb2_signature, 0x0, SMB2_HMACSHA256_SIZE);
>         memset(shdr->Signature, 0x0, SMB2_SIGNATURE_SIZE);
>
> -       rc =3D smb2_crypto_shash_allocate(server);
> -       if (rc) {
> -               cifs_server_dbg(VFS, "%s: sha256 alloc failed\n", __func_=
_);
> -               return rc;
> +       if (allocate_crypto) {
> +               rc =3D cifs_alloc_hash("hmac(sha256)", &hash, &sdesc);
> +               if (rc) {
> +                       cifs_server_dbg(VFS,
> +                                       "%s: sha256 alloc failed\n", __fu=
nc__);
> +                       return rc;
> +               }
> +               shash =3D &sdesc->shash;
> +       } else {
> +               hash =3D server->secmech.hmacsha256;
> +               shash =3D &server->secmech.sdeschmacsha256->shash;
>         }

smb2_crypto_shash_allocate() unconditionally allocated
server->secmech.hmacsha256 and server->secmech.sdeschmacsha256->shash.
Now the code doesn't allocate those variables at all. Unlike SMB3
where structures are allocated in during key generation, for SMB2 we
do it on demand in smb2_calc_signature(). So, the code above should be
changed to call smb2_crypto_shash_allocate() in "else" block.

--
Best regards,
Pavel Shilovsky