| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <586F415B.7030207@caviumnetworks.com>
Date: Fri, 6 Jan 2017 12:33:55 +0530
From: George Cherian <gcherian@...iumnetworks.com>
To: Corentin Labbe <clabbe.montjoie@...il.com>,
<george.cherian@...ium.com>
CC: <herbert@...dor.apana.org.au>, <davem@...emloft.net>,
<linux-kernel@...r.kernel.org>, <linux-crypto@...r.kernel.org>,
<david.daney@...ium.com>
Subject: Re: [PATCH v3 2/3] drivers: crypto: Add the Virtual Function driver
for CPT
Hi Corentin,
On 12/21/2016 07:31 PM, Corentin Labbe wrote:
> Hello
>
> I have some comment inline
>
> On Wed, Dec 21, 2016 at 11:56:12AM +0000, george.cherian@...ium.com wrote:
>> From: George Cherian <george.cherian@...ium.com>
>>
>> Enable the CPT VF driver. CPT is the cryptographic Accelaration Unit
>
> typo acceleration
will fix
>
> [...]
>> +static inline void update_input_data(struct cpt_request_info *req_info,
>> + struct scatterlist *inp_sg,
>> + u32 nbytes, u32 *argcnt)
>> +{
>> + req_info->req.dlen += nbytes;
>> +
>> + while (nbytes) {
>> + u32 len = min(nbytes, inp_sg->length);
>> + u8 *ptr = page_address(sg_page(inp_sg)) + inp_sg->offset;
>
> You could use sg_virt instead.
Thanks for pointing it out, Yes will replace with sg_virt.
>
> But do you have tested your accelerator with user space data (via cryptodev/AF_ALG) ?
No I have tested only using in kernel applications, Not used
cryptodev/AF_ALG
> In my memory, you better use kmap() instead of this direct memory address.
>
> [...]
>> +static inline u32 cvm_enc_dec(struct ablkcipher_request *req, u32 enc,
>> + u32 cipher_type)
>> +{
>> + struct crypto_ablkcipher *tfm = crypto_ablkcipher_reqtfm(req);
>> + struct cvm_enc_ctx *ctx = crypto_ablkcipher_ctx(tfm);
>> + u32 key_type = AES_128_BIT;
>> + struct cvm_req_ctx *rctx = ablkcipher_request_ctx(req);
>> + u32 enc_iv_len = crypto_ablkcipher_ivsize(tfm);
>> + struct fc_context *fctx = &rctx->fctx;
>> + struct cpt_request_info *req_info = &rctx->cpt_req;
>> + void *cdev = NULL;
>> + u32 status = -1;
>
> Doable but dangerous
> Furthermore, cptvf_do_request return int so why use u32 ?
will fix it.
>
> [...]
>> +void cvm_enc_dec_exit(struct crypto_tfm *tfm)
>> +{
>> + return;
>> +}
>
> So you could remove all reference to this function
>
okay
> [...]
>> +static inline int cav_register_algs(void)
>> +{
>> + int err = 0;
>> +
>> + err = crypto_register_algs(algs, ARRAY_SIZE(algs));
>> + if (err) {
>> + pr_err("Error in aes module init %d\n", err);
>> + return -1;
>
> This is not a standard error code
>
okay
> [...]
>> diff --git a/drivers/crypto/cavium/cpt/cptvf_algs.h b/drivers/crypto/cavium/cpt/cptvf_algs.h
>> new file mode 100644
>> index 0000000..fcb287b
>> --- /dev/null
>> +++ b/drivers/crypto/cavium/cpt/cptvf_algs.h
> [...]
>> +
>> +u32 cptvf_do_request(void *cptvf, struct cpt_request_info *req);
>
> latter this function is set "return int"
>
> [...]
>> +static int cptvf_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>> +{
>> + struct device *dev = &pdev->dev;
>> + struct cpt_vf *cptvf;
>> + int err;
>> +
>> + cptvf = devm_kzalloc(dev, sizeof(struct cpt_vf), GFP_KERNEL);
>
> use sizeof(*cptvf) and checkpatch
>
okay
> [...]
>> +static int setup_sgio_components(struct cpt_vf *cptvf, struct buf_ptr *list,
>> + int buf_count, u8 *buffer)
>> +{
>> + int ret = 0, i, j;
>> + int components;
>> + struct sglist_component *sg_ptr = NULL;
>> + struct pci_dev *pdev = cptvf->pdev;
>> +
>> + if (unlikely(!list)) {
>> + pr_err("Input List pointer is NULL\n");
>> + ret = -EFAULT;
>> + return ret;
>
> You could directly return -EFAULT and use dev_err()
>
okay
>> + }
>> +
>> + for (i = 0; i < buf_count; i++) {
>> + if (likely(list[i].vptr)) {
>> + list[i].dma_addr = dma_map_single(&pdev->dev,
>> + list[i].vptr,
>> + list[i].size,
>> + DMA_BIDIRECTIONAL);
>> + if (unlikely(dma_mapping_error(&pdev->dev,
>> + list[i].dma_addr))) {
>> + pr_err("DMA map kernel buffer failed for component: %d\n",
>> + i);
>
> Use dev_err
>
> [...]
>> + u16 g_sz_bytes = 0, s_sz_bytes = 0;
>> + int ret = 0;
>> + struct pci_dev *pdev = cptvf->pdev;
>> +
>> + if (req->incnt > MAX_SG_IN_CNT || req->outcnt > MAX_SG_OUT_CNT) {
>> + pr_err("Requestes SG components are higher than supported\n");
>
> typo request and use dev_err
>
> In all files you have some pr_x that could be better use as dev_x
okay
>
>> + ret = -EINVAL;
>> + goto scatter_gather_clean;
>> + }
>> +
>> + /* Setup gather (input) components */
>> + g_sz_bytes = ((req->incnt + 3) / 4) * sizeof(struct sglist_component);
>> + info->gather_components = kzalloc((g_sz_bytes), GFP_KERNEL);
>
> unnecessary parenthesis
>
>> + if (!info->gather_components) {
>> + ret = -ENOMEM;
>> + goto scatter_gather_clean;
>> + }
>> +
>> + ret = setup_sgio_components(cptvf, req->in,
>> + req->incnt,
>> + info->gather_components);
>> + if (ret) {
>> + pr_err("Failed to setup gather list\n");
>> + ret = -EFAULT;
>> + goto scatter_gather_clean;
>> + }
>> +
>> + /* Setup scatter (output) components */
>> + s_sz_bytes = ((req->outcnt + 3) / 4) * sizeof(struct sglist_component);
>> + info->scatter_components = kzalloc((s_sz_bytes), GFP_KERNEL);
>
> again
>
>> + if (!info->scatter_components) {
>> + ret = -ENOMEM;
>> + goto scatter_gather_clean;
>> + }
>> +
>> + ret = setup_sgio_components(cptvf, req->out,
>> + req->outcnt,
>> + info->scatter_components);
>> + if (ret) {
>> + pr_err("Failed to setup gather list\n");
>> + ret = -EFAULT;
>> + goto scatter_gather_clean;
>
> double space
okay
>
>> + }
>> +
>> + /* Create and initialize DPTR */
>> + info->dlen = g_sz_bytes + s_sz_bytes + SG_LIST_HDR_SIZE;
>> + info->in_buffer = kzalloc((info->dlen), GFP_KERNEL);
>
> double parenthesis
> I will stop here, you have lots of that in all your alloc
>
okay
> [...]
>> +
>> + ret = send_cpt_command(cptvf, &cptinst, queue);
>> + spin_unlock_bh(&pqueue->lock);
>> + if (unlikely(ret)) {
>> + spin_unlock_bh(&pqueue->lock);
>
> Double unlock
>
Yes will fix it.
> [...]
>> diff --git a/drivers/crypto/cavium/cpt/request_manager.h b/drivers/crypto/cavium/cpt/request_manager.h
>> new file mode 100644
>> index 0000000..df6c306
>> --- /dev/null
>> +++ b/drivers/crypto/cavium/cpt/request_manager.h
>> @@ -0,0 +1,147 @@
>> +/*
>> + * Copyright (C) 2016 Cavium, Inc.
>> + *
>> + * This program is free software; you can redistribute it and/or modify it
>> + * under the terms of version 2 of the GNU General Public License
>> + * as published by the Free Software Foundation.
>> + */
>> +
>> +#ifndef __REQUEST_MANGER_H
>> +#define __REQUEST_MANGER_H
>
> typo manager
>
okay
> Thanks
> Regards
> Corentin Labbe
>
Powered by blists - more mailing lists