netdev - Re: [PATCH v4 05/16] IB/pvrdma: Add functions for Verbs support

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 14 Sep 2016 15:28:12 +0300
From:   Yuval Shaia <yuval.shaia@...cle.com>
To:     Adit Ranadive <aditr@...are.com>
Cc:     dledford@...hat.com, linux-rdma@...r.kernel.org,
        pv-drivers@...are.com, netdev@...r.kernel.org,
        linux-pci@...r.kernel.org, jhansen@...are.com, asarwade@...are.com,
        georgezhang@...are.com, bryantan@...are.com
Subject: Re: [PATCH v4 05/16] IB/pvrdma: Add functions for Verbs support

On Sun, Sep 11, 2016 at 09:49:15PM -0700, Adit Ranadive wrote:
> +
> +/**
> + * pvrdma_alloc_pd - allocate protection domain
> + * @ibdev: the IB device
> + * @context: user context
> + * @udata: user data
> + *
> + * @return: the ib_pd protection domain pointer on success, otherwise errno.
> + */
> +struct ib_pd *pvrdma_alloc_pd(struct ib_device *ibdev,
> +			      struct ib_ucontext *context,
> +			      struct ib_udata *udata)
> +{
> +	struct pvrdma_pd *pd;
> +	struct pvrdma_dev *dev = to_vdev(ibdev);
> +	union pvrdma_cmd_req req;
> +	union pvrdma_cmd_resp rsp;
> +	struct pvrdma_cmd_create_pd *cmd = &req.create_pd;
> +	struct pvrdma_cmd_create_pd_resp *resp = &rsp.create_pd_resp;
> +	int ret;
> +	void *ptr;
> +
> +	/* Check allowed max pds */
> +	if (!atomic_add_unless(&dev->num_pds, 1, dev->dsr->caps.max_pd))
> +		return ERR_PTR(-EINVAL);
> +
> +	memset(cmd, 0, sizeof(*cmd));
> +	cmd->hdr.cmd = PVRDMA_CMD_CREATE_PD;
> +	cmd->ctx_handle = (context) ? to_vucontext(context)->ctx_handle : 0;
> +	ret = pvrdma_cmd_post(dev, &req, &rsp);
> +	if (ret < 0) {
> +		dev_warn(&dev->pdev->dev,
> +			 "failed to allocate protection domain, error: %d\n",
> +			 ret);
> +		ptr = ERR_PTR(ret);
> +		goto err;
> +	} else if (resp->hdr.ack != PVRDMA_CMD_CREATE_PD_RESP) {
> +		dev_warn(&dev->pdev->dev,
> +			 "unknown response for allocate protection domain\n");
> +		ptr = ERR_PTR(-EFAULT);
> +		goto err;
> +	}
> +
> +	pd = kmalloc(sizeof(*pd), GFP_KERNEL);
> +	if (!pd) {
> +		ptr = ERR_PTR(-ENOMEM);
> +		goto err;
> +	}

I know that this was my suggestion but also remember that you raised a
(correct) argument that it is preferred to first do other allocation and
free them if command fails then the other way around where failure of
memory allocation (like here) will force us to do the opposite command
(pvrdma_dealloc_pd in this case).

So either accept your way (better) or call pvrdma_dealloc_pd when kmalloc
fails.

> +
> +	pd->privileged = !context;
> +	pd->pd_handle = resp->pd_handle;
> +	pd->pdn = resp->pd_handle;
> +
> +	if (context) {
> +		if (ib_copy_to_udata(udata, &pd->pdn, sizeof(__u32))) {
> +			dev_warn(&dev->pdev->dev,
> +				 "failed to copy back protection domain\n");
> +			pvrdma_dealloc_pd(&pd->ibpd);
> +			return ERR_PTR(-EFAULT);
> +		}
> +	}
> +
> +	/* u32 pd handle */
> +	return  &pd->ibpd;
> +
> +err:
> +	atomic_dec(&dev->num_pds);
> +	return ptr;
> +}