linux-kernel - Re: [PATCH v8 2/6] virtio-pmem: Add virtio pmem driver

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:   Tue, 14 May 2019 11:34:05 +0200
From:   David Hildenbrand <david@...hat.com>
To:     Pankaj Gupta <pagupta@...hat.com>
Cc:     linux-nvdimm@...ts.01.org, linux-kernel@...r.kernel.org,
        virtualization@...ts.linux-foundation.org, kvm@...r.kernel.org,
        linux-fsdevel@...r.kernel.org, linux-acpi@...r.kernel.org,
        qemu-devel@...gnu.org, linux-ext4@...r.kernel.org,
        linux-xfs@...r.kernel.org,
        dan j williams <dan.j.williams@...el.com>,
        zwisler@...nel.org, vishal l verma <vishal.l.verma@...el.com>,
        dave jiang <dave.jiang@...el.com>, mst@...hat.com,
        jasowang@...hat.com, willy@...radead.org, rjw@...ysocki.net,
        hch@...radead.org, lenb@...nel.org, jack@...e.cz, tytso@....edu,
        adilger kernel <adilger.kernel@...ger.ca>,
        darrick wong <darrick.wong@...cle.com>, lcapitulino@...hat.com,
        kwolf@...hat.com, imammedo@...hat.com, jmoyer@...hat.com,
        nilal@...hat.com, riel@...riel.com, stefanha@...hat.com,
        aarcange@...hat.com, david@...morbit.com, cohuck@...hat.com,
        xiaoguangrong eric <xiaoguangrong.eric@...il.com>,
        pbonzini@...hat.com, kilobyte@...band.pl,
        yuval shaia <yuval.shaia@...cle.com>, jstaron@...gle.com
Subject: Re: [PATCH v8 2/6] virtio-pmem: Add virtio pmem driver

>>
>>> +	}
>>> +
>>> +	/* When host has read buffer, this completes via host_ack */
>>
>> "A host repsonse results in "host_ack" getting called" ... ?
>>
>>> +	wait_event(req->host_acked, req->done);
>>> +	err = req->ret;
>>> +ret:
>>> +	kfree(req);
>>> +	return err;
>>> +};
>>> +
>>> +/* The asynchronous flush callback function */
>>> +int async_pmem_flush(struct nd_region *nd_region, struct bio *bio)
>>> +{
>>> +	int rc = 0;
>>> +
>>> +	/* Create child bio for asynchronous flush and chain with
>>> +	 * parent bio. Otherwise directly call nd_region flush.
>>> +	 */
>>> +	if (bio && bio->bi_iter.bi_sector != -1) {
>>> +		struct bio *child = bio_alloc(GFP_ATOMIC, 0);
>>> +
>>> +		if (!child)
>>> +			return -ENOMEM;
>>> +		bio_copy_dev(child, bio);
>>> +		child->bi_opf = REQ_PREFLUSH;
>>> +		child->bi_iter.bi_sector = -1;
>>> +		bio_chain(child, bio);
>>> +		submit_bio(child);
>>
>> return 0;
>>
>> Then, drop the "else" case and "int rc" and do directly
>>
>> if (virtio_pmem_flush(nd_region))
>> 	return -EIO;
> 
> and another 'return 0' here :)
> 
> I don't like return from multiple places instead I prefer
> single exit point from function.

Makes this function more complicated than necessary. I agree when there
are locks involved.

>  
>>
>>> +
>>> +	return 0;
>>> +};
>>> +
>>> +static int virtio_pmem_probe(struct virtio_device *vdev)
>>> +{
>>> +	int err = 0;
>>> +	struct resource res;
>>> +	struct virtio_pmem *vpmem;
>>> +	struct nd_region_desc ndr_desc = {};
>>> +	int nid = dev_to_node(&vdev->dev);
>>> +	struct nd_region *nd_region;
>>
>> Nit: use reverse Christmas tree layout :)
> 
> Done.
> 
>>
>>> +
>>> +	if (!vdev->config->get) {
>>> +		dev_err(&vdev->dev, "%s failure: config access disabled\n",
>>> +			__func__);
>>> +		return -EINVAL;
>>> +	}
>>> +
>>> +	vpmem = devm_kzalloc(&vdev->dev, sizeof(*vpmem), GFP_KERNEL);
>>> +	if (!vpmem) {
>>> +		err = -ENOMEM;
>>> +		goto out_err;
>>> +	}
>>> +
>>> +	vpmem->vdev = vdev;
>>> +	vdev->priv = vpmem;
>>> +	err = init_vq(vpmem);
>>> +	if (err)
>>> +		goto out_err;
>>> +
>>> +	virtio_cread(vpmem->vdev, struct virtio_pmem_config,
>>> +			start, &vpmem->start);
>>> +	virtio_cread(vpmem->vdev, struct virtio_pmem_config,
>>> +			size, &vpmem->size);
>>> +
>>> +	res.start = vpmem->start;
>>> +	res.end   = vpmem->start + vpmem->size-1;
>>> +	vpmem->nd_desc.provider_name = "virtio-pmem";
>>> +	vpmem->nd_desc.module = THIS_MODULE;
>>> +
>>> +	vpmem->nvdimm_bus = nvdimm_bus_register(&vdev->dev,
>>> +						&vpmem->nd_desc);
>>> +	if (!vpmem->nvdimm_bus)
>>> +		goto out_vq;
>>> +
>>> +	dev_set_drvdata(&vdev->dev, vpmem->nvdimm_bus);
>>> +
>>> +	ndr_desc.res = &res;
>>> +	ndr_desc.numa_node = nid;
>>> +	ndr_desc.flush = async_pmem_flush;
>>> +	set_bit(ND_REGION_PAGEMAP, &ndr_desc.flags);
>>> +	set_bit(ND_REGION_ASYNC, &ndr_desc.flags);
>>> +	nd_region = nvdimm_pmem_region_create(vpmem->nvdimm_bus, &ndr_desc);
>>> +
>>
>> I'd drop this empty line.
> 
> hmm.
> 

The common pattern after allocating something, immediately check for it
in the next line (like you do throughout this patch ;) )

...
>> You are not freeing "vdev->priv".
> 
> vdev->priv is vpmem which is allocated using devm API.

I'm confused. Looking at drivers/virtio/virtio_balloon.c:

static void virtballoon_remove(struct virtio_device *vdev)
{
	struct virtio_balloon *vb = vdev->priv;

	...

	kfree(vb);
}

I think you should do the same here, vdev->priv is allocated in
virtio_pmem_probe.

But maybe I am missing something important here :)

>>
>>> +	nvdimm_bus_unregister(nvdimm_bus);
>>> +	vdev->config->del_vqs(vdev);
>>> +	vdev->config->reset(vdev);
>>> +}
>>> +

-- 

Thanks,

David / dhildenb