>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... >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