Re: [PATCH] cifs: Allocate crypto structures on the fly for calculating signatures of incoming packets
From: Pavel Shilovsky
Date: Fri Apr 03 2020 - 20:05:04 EST
ÐÑ, 3 ÐÐÑ. 2020 Ð. Ð 16:11, Long Li <longli@xxxxxxxxxxxxx>:
>
> >Subject: Re: [PATCH] cifs: Allocate crypto structures on the fly for calculating
> >signatures of incoming packets
> >
> >ÐÑ, 31 ÐÐÑ. 2020 Ð. Ð 16:22, <longli@xxxxxxxxxxxxxxxxx>:
> >>
> >> From: Long Li <longli@xxxxxxxxxxxxx>
> >>
> >> CIFS uses pre-allocated crypto structures to calculate signatures for
> >> both incoming and outgoing packets. In this way it doesn't need to
> >> allocate crypto 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 transport lock srv_mutex.
> >>
> >> Signed-off-by: Long Li <longli@xxxxxxxxxxxxx>
> >> ---
> >> 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
> >> *tcon, 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_Server_Info *server, extern struct cifs_tcon
> >*smb2_find_smb_tcon(struct TCP_Server_Info *server,
> >> __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
> >> *server)
> >> +smb2_calc_signature(struct smb_rqst *rqst, struct TCP_Server_Info
> >*server,
> >> + 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 = (struct smb2_sync_hdr *)iov[0].iov_base;
> >> struct cifs_ses *ses;
> >> struct shash_desc *shash;
> >> + struct crypto_shash *hash;
> >> + struct sdesc *sdesc = NULL;
> >> struct smb_rqst drqst;
> >>
> >> ses = smb2_find_smb_ses(server, shdr->SessionId); @@ -239,24
> >> +234,32 @@ smb2_calc_signature(struct smb_rqst *rqst, struct
> >TCP_Server_Info *server)
> >> memset(smb2_signature, 0x0, SMB2_HMACSHA256_SIZE);
> >> memset(shdr->Signature, 0x0, SMB2_SIGNATURE_SIZE);
> >>
> >> - rc = smb2_crypto_shash_allocate(server);
> >> - if (rc) {
> >> - cifs_server_dbg(VFS, "%s: sha256 alloc failed\n", __func__);
> >> - return rc;
> >> + if (allocate_crypto) {
> >> + rc = cifs_alloc_hash("hmac(sha256)", &hash, &sdesc);
> >> + if (rc) {
> >> + cifs_server_dbg(VFS,
> >> + "%s: sha256 alloc failed\n", __func__);
> >> + return rc;
> >> + }
> >> + shash = &sdesc->shash;
> >> + } else {
> >> + hash = server->secmech.hmacsha256;
> >> + shash = &server->secmech.sdeschmacsha256->shash;
> >> }
> >
> >smb2_crypto_shash_allocate() unconditionally allocated
> >server->secmech.hmacsha256 and server->secmech.sdeschmacsha256-
> >>shash.
>
> I think they are allocated in smb311_crypto_shash_allocate(), through
> => smb311_crypto_shash_allocate
> => smb311_update_preauth_hash
> => compound_send_recv
> => cifs_send_recv
> => SMB2_negotiate
>
> The function names are a little misleading...
smb311_crypto_shash_allocate() only allocate those structures for SMB
3.1.1 protocol version, see the code below:
797 int
798 smb311_update_preauth_hash(struct cifs_ses *ses, struct kvec *iov, int nvec)
799 {
800 >-------int i, rc;
801 >-------struct sdesc *d;
802 >-------struct smb2_sync_hdr *hdr;
803
804 >-------if (ses->server->tcpStatus == CifsGood) {
805 >------->-------/* skip non smb311 connections */
806 >------->-------if (ses->server->dialect != SMB311_PROT_ID)
807 >------->------->-------return 0;
--
Best regards,
Pavel Shilovsky