diff mbox series

[v4,11/16] rpmsg: virtio: register the rpmsg_ctrl device

Message ID 20210217132905.1485-12-arnaud.pouliquen@foss.st.com (mailing list archive)
State Superseded
Headers show
Series introduce a generic IOCTL interface for RPMsg channels management | expand

Commit Message

Arnaud POULIQUEN Feb. 17, 2021, 1:29 p.m. UTC
Instantiate the rpmsg_ioctl device on virtio RPMsg bus creation.
This provides the possibility to expose the RPMSG_CREATE_EPT_IOCTL
to create RPMsg chdev endpoints.

Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>

---
V3:
Fix compilation issue
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>

warnings:
drivers/rpmsg/virtio_rpmsg_bus.c:978 rpmsg_probe() error: uninitialized symbol 'vch'.
drivers/rpmsg/virtio_rpmsg_bus.c:979 rpmsg_probe() error: uninitialized symbol 'rpdev_ctrl'.
---
 drivers/rpmsg/virtio_rpmsg_bus.c | 37 +++++++++++++++++++++++++++++++-
 1 file changed, 36 insertions(+), 1 deletion(-)

Comments

Dan Carpenter Feb. 18, 2021, 12:27 p.m. UTC | #1
Hi Arnaud,

url:    https://github.com/0day-ci/linux/commits/Arnaud-Pouliquen/introduce-a-generic-IOCTL-interface-for-RPMsg-channels-management/20210217-214044
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git f40ddce88593482919761f74910f42f4b84c004b
config: x86_64-randconfig-m001-20210215 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-15) 9.3.0

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>

smatch warnings:
drivers/rpmsg/virtio_rpmsg_bus.c:977 rpmsg_probe() error: uninitialized symbol 'vch'.

vim +/vch +977 drivers/rpmsg/virtio_rpmsg_bus.c

bcabbccabffe73 Ohad Ben-Cohen       2011-10-20  845  static int rpmsg_probe(struct virtio_device *vdev)
bcabbccabffe73 Ohad Ben-Cohen       2011-10-20  846  {
bcabbccabffe73 Ohad Ben-Cohen       2011-10-20  847  	vq_callback_t *vq_cbs[] = { rpmsg_recv_done, rpmsg_xmit_done };
f7ad26ff952b3c Stefan Hajnoczi      2015-12-17  848  	static const char * const names[] = { "input", "output" };
bcabbccabffe73 Ohad Ben-Cohen       2011-10-20  849  	struct virtqueue *vqs[2];
bcabbccabffe73 Ohad Ben-Cohen       2011-10-20  850  	struct virtproc_info *vrp;
950a7388f02bf7 Arnaud Pouliquen     2020-11-20  851  	struct virtio_rpmsg_channel *vch;
e3bba4363fe87b Arnaud Pouliquen     2021-02-17  852  	struct rpmsg_device *rpdev_ns = NULL, *rpdev_ctrl = NULL;
bcabbccabffe73 Ohad Ben-Cohen       2011-10-20  853  	void *bufs_va;
bcabbccabffe73 Ohad Ben-Cohen       2011-10-20  854  	int err = 0, i;
b1b9891441fa33 Suman Anna           2014-09-16  855  	size_t total_buf_space;
71e4b8bf0482fc Michael S. Tsirkin   2015-03-12  856  	bool notify;
bcabbccabffe73 Ohad Ben-Cohen       2011-10-20  857  
bcabbccabffe73 Ohad Ben-Cohen       2011-10-20  858  	vrp = kzalloc(sizeof(*vrp), GFP_KERNEL);

You might want to consider updating this stuff to devm_kzalloc() which
is trendy with the kids these days.  It's cleaned up automatically when
the module is released.

bcabbccabffe73 Ohad Ben-Cohen       2011-10-20  859  	if (!vrp)
bcabbccabffe73 Ohad Ben-Cohen       2011-10-20  860  		return -ENOMEM;
bcabbccabffe73 Ohad Ben-Cohen       2011-10-20  861  
bcabbccabffe73 Ohad Ben-Cohen       2011-10-20  862  	vrp->vdev = vdev;
bcabbccabffe73 Ohad Ben-Cohen       2011-10-20  863  
bcabbccabffe73 Ohad Ben-Cohen       2011-10-20  864  	idr_init(&vrp->endpoints);
bcabbccabffe73 Ohad Ben-Cohen       2011-10-20  865  	mutex_init(&vrp->endpoints_lock);
bcabbccabffe73 Ohad Ben-Cohen       2011-10-20  866  	mutex_init(&vrp->tx_lock);
bcabbccabffe73 Ohad Ben-Cohen       2011-10-20  867  	init_waitqueue_head(&vrp->sendq);
bcabbccabffe73 Ohad Ben-Cohen       2011-10-20  868  
bcabbccabffe73 Ohad Ben-Cohen       2011-10-20  869  	/* We expect two virtqueues, rx and tx (and in this order) */
9b2bbdb2275884 Michael S. Tsirkin   2017-03-06  870  	err = virtio_find_vqs(vdev, 2, vqs, vq_cbs, names, NULL);
bcabbccabffe73 Ohad Ben-Cohen       2011-10-20  871  	if (err)
bcabbccabffe73 Ohad Ben-Cohen       2011-10-20  872  		goto free_vrp;
bcabbccabffe73 Ohad Ben-Cohen       2011-10-20  873  
bcabbccabffe73 Ohad Ben-Cohen       2011-10-20  874  	vrp->rvq = vqs[0];
bcabbccabffe73 Ohad Ben-Cohen       2011-10-20  875  	vrp->svq = vqs[1];
bcabbccabffe73 Ohad Ben-Cohen       2011-10-20  876  
b1b9891441fa33 Suman Anna           2014-09-16  877  	/* we expect symmetric tx/rx vrings */
b1b9891441fa33 Suman Anna           2014-09-16  878  	WARN_ON(virtqueue_get_vring_size(vrp->rvq) !=
b1b9891441fa33 Suman Anna           2014-09-16  879  		virtqueue_get_vring_size(vrp->svq));
b1b9891441fa33 Suman Anna           2014-09-16  880  
b1b9891441fa33 Suman Anna           2014-09-16  881  	/* we need less buffers if vrings are small */
b1b9891441fa33 Suman Anna           2014-09-16  882  	if (virtqueue_get_vring_size(vrp->rvq) < MAX_RPMSG_NUM_BUFS / 2)
b1b9891441fa33 Suman Anna           2014-09-16  883  		vrp->num_bufs = virtqueue_get_vring_size(vrp->rvq) * 2;
b1b9891441fa33 Suman Anna           2014-09-16  884  	else
b1b9891441fa33 Suman Anna           2014-09-16  885  		vrp->num_bufs = MAX_RPMSG_NUM_BUFS;
b1b9891441fa33 Suman Anna           2014-09-16  886  
f93848f9eeb0f8 Loic Pallardy        2017-03-28  887  	vrp->buf_size = MAX_RPMSG_BUF_SIZE;
f93848f9eeb0f8 Loic Pallardy        2017-03-28  888  
f93848f9eeb0f8 Loic Pallardy        2017-03-28  889  	total_buf_space = vrp->num_bufs * vrp->buf_size;
b1b9891441fa33 Suman Anna           2014-09-16  890  
bcabbccabffe73 Ohad Ben-Cohen       2011-10-20  891  	/* allocate coherent memory for the buffers */
d999b622fcfb39 Loic Pallardy        2019-01-10  892  	bufs_va = dma_alloc_coherent(vdev->dev.parent,
b1b9891441fa33 Suman Anna           2014-09-16  893  				     total_buf_space, &vrp->bufs_dma,
b1b9891441fa33 Suman Anna           2014-09-16  894  				     GFP_KERNEL);
3119b487e03650 Wei Yongjun          2013-04-29  895  	if (!bufs_va) {
3119b487e03650 Wei Yongjun          2013-04-29  896  		err = -ENOMEM;
bcabbccabffe73 Ohad Ben-Cohen       2011-10-20  897  		goto vqs_del;
3119b487e03650 Wei Yongjun          2013-04-29  898  	}
bcabbccabffe73 Ohad Ben-Cohen       2011-10-20  899  
de4064af76537f Suman Anna           2018-10-23  900  	dev_dbg(&vdev->dev, "buffers: va %pK, dma %pad\n",
8d95b322ba34b1 Anna, Suman          2016-08-12  901  		bufs_va, &vrp->bufs_dma);
bcabbccabffe73 Ohad Ben-Cohen       2011-10-20  902  
bcabbccabffe73 Ohad Ben-Cohen       2011-10-20  903  	/* half of the buffers is dedicated for RX */
bcabbccabffe73 Ohad Ben-Cohen       2011-10-20  904  	vrp->rbufs = bufs_va;
bcabbccabffe73 Ohad Ben-Cohen       2011-10-20  905  
bcabbccabffe73 Ohad Ben-Cohen       2011-10-20  906  	/* and half is dedicated for TX */
b1b9891441fa33 Suman Anna           2014-09-16  907  	vrp->sbufs = bufs_va + total_buf_space / 2;
bcabbccabffe73 Ohad Ben-Cohen       2011-10-20  908  
bcabbccabffe73 Ohad Ben-Cohen       2011-10-20  909  	/* set up the receive buffers */
b1b9891441fa33 Suman Anna           2014-09-16  910  	for (i = 0; i < vrp->num_bufs / 2; i++) {
bcabbccabffe73 Ohad Ben-Cohen       2011-10-20  911  		struct scatterlist sg;
f93848f9eeb0f8 Loic Pallardy        2017-03-28  912  		void *cpu_addr = vrp->rbufs + i * vrp->buf_size;
bcabbccabffe73 Ohad Ben-Cohen       2011-10-20  913  
9dd87c2af651b0 Loic Pallardy        2017-03-28  914  		rpmsg_sg_init(&sg, cpu_addr, vrp->buf_size);
bcabbccabffe73 Ohad Ben-Cohen       2011-10-20  915  
cee51d69a45b6c Rusty Russell        2013-03-20  916  		err = virtqueue_add_inbuf(vrp->rvq, &sg, 1, cpu_addr,
bcabbccabffe73 Ohad Ben-Cohen       2011-10-20  917  					  GFP_KERNEL);
57e1a37347d31c Rusty Russell        2012-10-16  918  		WARN_ON(err); /* sanity check; this can't really happen */
bcabbccabffe73 Ohad Ben-Cohen       2011-10-20  919  	}
bcabbccabffe73 Ohad Ben-Cohen       2011-10-20  920  
bcabbccabffe73 Ohad Ben-Cohen       2011-10-20  921  	/* suppress "tx-complete" interrupts */
bcabbccabffe73 Ohad Ben-Cohen       2011-10-20  922  	virtqueue_disable_cb(vrp->svq);
bcabbccabffe73 Ohad Ben-Cohen       2011-10-20  923  
bcabbccabffe73 Ohad Ben-Cohen       2011-10-20  924  	vdev->priv = vrp;
bcabbccabffe73 Ohad Ben-Cohen       2011-10-20  925  
bcabbccabffe73 Ohad Ben-Cohen       2011-10-20  926  	/* if supported by the remote processor, enable the name service */
bcabbccabffe73 Ohad Ben-Cohen       2011-10-20  927  	if (virtio_has_feature(vdev, VIRTIO_RPMSG_F_NS)) {
950a7388f02bf7 Arnaud Pouliquen     2020-11-20  928  		vch = kzalloc(sizeof(*vch), GFP_KERNEL);
950a7388f02bf7 Arnaud Pouliquen     2020-11-20  929  		if (!vch) {
bcabbccabffe73 Ohad Ben-Cohen       2011-10-20  930  			err = -ENOMEM;
bcabbccabffe73 Ohad Ben-Cohen       2011-10-20  931  			goto free_coherent;
bcabbccabffe73 Ohad Ben-Cohen       2011-10-20  932  		}
950a7388f02bf7 Arnaud Pouliquen     2020-11-20  933  
950a7388f02bf7 Arnaud Pouliquen     2020-11-20  934  		/* Link the channel to our vrp */
950a7388f02bf7 Arnaud Pouliquen     2020-11-20  935  		vch->vrp = vrp;
950a7388f02bf7 Arnaud Pouliquen     2020-11-20  936  
950a7388f02bf7 Arnaud Pouliquen     2020-11-20  937  		/* Assign public information to the rpmsg_device */
950a7388f02bf7 Arnaud Pouliquen     2020-11-20  938  		rpdev_ns = &vch->rpdev;
950a7388f02bf7 Arnaud Pouliquen     2020-11-20  939  		rpdev_ns->ops = &virtio_rpmsg_ops;
950a7388f02bf7 Arnaud Pouliquen     2020-11-20  940  		rpdev_ns->little_endian = virtio_is_little_endian(vrp->vdev);
950a7388f02bf7 Arnaud Pouliquen     2020-11-20  941  
950a7388f02bf7 Arnaud Pouliquen     2020-11-20  942  		rpdev_ns->dev.parent = &vrp->vdev->dev;
950a7388f02bf7 Arnaud Pouliquen     2020-11-20  943  		rpdev_ns->dev.release = virtio_rpmsg_release_device;
950a7388f02bf7 Arnaud Pouliquen     2020-11-20  944  
950a7388f02bf7 Arnaud Pouliquen     2020-11-20  945  		err = rpmsg_ns_register_device(rpdev_ns);
950a7388f02bf7 Arnaud Pouliquen     2020-11-20  946  		if (err)
950a7388f02bf7 Arnaud Pouliquen     2020-11-20  947  			goto free_coherent;
bcabbccabffe73 Ohad Ben-Cohen       2011-10-20  948  	}

"vch" not initialized on else path.  Also keep in mind that "rpdev_ctrl"
is NULL at this point.

bcabbccabffe73 Ohad Ben-Cohen       2011-10-20  949  
e3bba4363fe87b Arnaud Pouliquen     2021-02-17  950  	rpdev_ctrl = rpmsg_virtio_add_char_dev(vdev);
                                                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

e3bba4363fe87b Arnaud Pouliquen     2021-02-17  951  	if (IS_ERR(rpdev_ctrl)) {
e3bba4363fe87b Arnaud Pouliquen     2021-02-17  952  		err = PTR_ERR(rpdev_ctrl);
e3bba4363fe87b Arnaud Pouliquen     2021-02-17  953  		goto free_coherent;

I should create a Smatch warning for this as well.

e3bba4363fe87b Arnaud Pouliquen     2021-02-17  954  	}
71e4b8bf0482fc Michael S. Tsirkin   2015-03-12  955  	/*
71e4b8bf0482fc Michael S. Tsirkin   2015-03-12  956  	 * Prepare to kick but don't notify yet - we can't do this before
71e4b8bf0482fc Michael S. Tsirkin   2015-03-12  957  	 * device is ready.
71e4b8bf0482fc Michael S. Tsirkin   2015-03-12  958  	 */
71e4b8bf0482fc Michael S. Tsirkin   2015-03-12  959  	notify = virtqueue_kick_prepare(vrp->rvq);
71e4b8bf0482fc Michael S. Tsirkin   2015-03-12  960  
71e4b8bf0482fc Michael S. Tsirkin   2015-03-12  961  	/* From this point on, we can notify and get callbacks. */
71e4b8bf0482fc Michael S. Tsirkin   2015-03-12  962  	virtio_device_ready(vdev);
71e4b8bf0482fc Michael S. Tsirkin   2015-03-12  963  
bcabbccabffe73 Ohad Ben-Cohen       2011-10-20  964  	/* tell the remote processor it can start sending messages */
71e4b8bf0482fc Michael S. Tsirkin   2015-03-12  965  	/*
71e4b8bf0482fc Michael S. Tsirkin   2015-03-12  966  	 * this might be concurrent with callbacks, but we are only
71e4b8bf0482fc Michael S. Tsirkin   2015-03-12  967  	 * doing notify, not a full kick here, so that's ok.
71e4b8bf0482fc Michael S. Tsirkin   2015-03-12  968  	 */
71e4b8bf0482fc Michael S. Tsirkin   2015-03-12  969  	if (notify)
71e4b8bf0482fc Michael S. Tsirkin   2015-03-12  970  		virtqueue_notify(vrp->rvq);
bcabbccabffe73 Ohad Ben-Cohen       2011-10-20  971  
bcabbccabffe73 Ohad Ben-Cohen       2011-10-20  972  	dev_info(&vdev->dev, "rpmsg host is online\n");
bcabbccabffe73 Ohad Ben-Cohen       2011-10-20  973  
bcabbccabffe73 Ohad Ben-Cohen       2011-10-20  974  	return 0;
bcabbccabffe73 Ohad Ben-Cohen       2011-10-20  975  
bcabbccabffe73 Ohad Ben-Cohen       2011-10-20  976  free_coherent:
950a7388f02bf7 Arnaud Pouliquen     2020-11-20 @977  	kfree(vch);
                                                        ^^^^^^^^^^
Uninitalized.

e3bba4363fe87b Arnaud Pouliquen     2021-02-17  978  	kfree(to_virtio_rpmsg_channel(rpdev_ctrl));

Now, let's say that "rpdev_ctrl" is NULL.  That's fine because
to_virtio_rpmsg_channel() is a no-op so it becomes kfree(NULL) which
is a no-op.  But why even have the to_virtio_rpmsg_channel() function
if we are going to rely on it being a no-op?

If "rpdev_ctrl" is an error pointer this will crash.  The problem is we
are freeing stuff that was not allocated.  The fix for this is:

1) Free the most recent successful allocation.
2) The unwind code should mirror the allocation code.
3) Choose label names which say what the goto does.  If the most recent
   allocation was "vch" the goto should be "goto free_vch;"
4) Every allocation function should have a matching free function.  It's
   a layering violation to have to know that the internals of
   rpmsg_virtio_add_char_dev().

So create function:

void rpmsg_virtio_del_char_dev(struct rpmsg_device *rpdev_ctrl)
{
	if (!rpdev_ctrl)
		return;
	kfree(to_virtio_rpmsg_channel(rpdev_ctrl));
}

The clean up code looks like this:

	return 0;

free_vch:
	if (virtio_has_feature(vdev, VIRTIO_RPMSG_F_NS))
		kfree(vch);
free_ctrl:
	rpmsg_virtio_del_char_dev(rpdev_ctrl);
free_coherent:
	dma_free_coherent(vdev->dev.parent, total_buf_space,
			  bufs_va, vrp->bufs_dma);
vqs_del:
	vdev->config->del_vqs(vrp->vdev);

Then just go through the function and as you read down the code keep
track of the most recent successful allocation and goto the correct
free label.

d999b622fcfb39 Loic Pallardy        2019-01-10  979  	dma_free_coherent(vdev->dev.parent, total_buf_space,
eeb0074f36d1ab Fernando Guzman Lugo 2012-08-29  980  			  bufs_va, vrp->bufs_dma);
bcabbccabffe73 Ohad Ben-Cohen       2011-10-20  981  vqs_del:
bcabbccabffe73 Ohad Ben-Cohen       2011-10-20  982  	vdev->config->del_vqs(vrp->vdev);
bcabbccabffe73 Ohad Ben-Cohen       2011-10-20  983  free_vrp:
bcabbccabffe73 Ohad Ben-Cohen       2011-10-20  984  	kfree(vrp);
bcabbccabffe73 Ohad Ben-Cohen       2011-10-20  985  	return err;
bcabbccabffe73 Ohad Ben-Cohen       2011-10-20  986  }

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Arnaud POULIQUEN Feb. 18, 2021, 5:14 p.m. UTC | #2
Hi Dan,

On 2/18/21 1:27 PM, Dan Carpenter wrote:
> Hi Arnaud,
> 
> url:    https://github.com/0day-ci/linux/commits/Arnaud-Pouliquen/introduce-a-generic-IOCTL-interface-for-RPMsg-channels-management/20210217-214044
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git f40ddce88593482919761f74910f42f4b84c004b
> config: x86_64-randconfig-m001-20210215 (attached as .config)
> compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
> 
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot <lkp@intel.com>
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> 
> smatch warnings:
> drivers/rpmsg/virtio_rpmsg_bus.c:977 rpmsg_probe() error: uninitialized symbol 'vch'.
> 
> vim +/vch +977 drivers/rpmsg/virtio_rpmsg_bus.c
> 
> bcabbccabffe73 Ohad Ben-Cohen       2011-10-20  845  static int rpmsg_probe(struct virtio_device *vdev)
> bcabbccabffe73 Ohad Ben-Cohen       2011-10-20  846  {
> bcabbccabffe73 Ohad Ben-Cohen       2011-10-20  847  	vq_callback_t *vq_cbs[] = { rpmsg_recv_done, rpmsg_xmit_done };
> f7ad26ff952b3c Stefan Hajnoczi      2015-12-17  848  	static const char * const names[] = { "input", "output" };
> bcabbccabffe73 Ohad Ben-Cohen       2011-10-20  849  	struct virtqueue *vqs[2];
> bcabbccabffe73 Ohad Ben-Cohen       2011-10-20  850  	struct virtproc_info *vrp;
> 950a7388f02bf7 Arnaud Pouliquen     2020-11-20  851  	struct virtio_rpmsg_channel *vch;
> e3bba4363fe87b Arnaud Pouliquen     2021-02-17  852  	struct rpmsg_device *rpdev_ns = NULL, *rpdev_ctrl = NULL;
> bcabbccabffe73 Ohad Ben-Cohen       2011-10-20  853  	void *bufs_va;
> bcabbccabffe73 Ohad Ben-Cohen       2011-10-20  854  	int err = 0, i;
> b1b9891441fa33 Suman Anna           2014-09-16  855  	size_t total_buf_space;
> 71e4b8bf0482fc Michael S. Tsirkin   2015-03-12  856  	bool notify;
> bcabbccabffe73 Ohad Ben-Cohen       2011-10-20  857  
> bcabbccabffe73 Ohad Ben-Cohen       2011-10-20  858  	vrp = kzalloc(sizeof(*vrp), GFP_KERNEL);
> 
> You might want to consider updating this stuff to devm_kzalloc() which
> is trendy with the kids these days.  It's cleaned up automatically when
> the module is released.
> 
> bcabbccabffe73 Ohad Ben-Cohen       2011-10-20  859  	if (!vrp)
> bcabbccabffe73 Ohad Ben-Cohen       2011-10-20  860  		return -ENOMEM;
> bcabbccabffe73 Ohad Ben-Cohen       2011-10-20  861  
> bcabbccabffe73 Ohad Ben-Cohen       2011-10-20  862  	vrp->vdev = vdev;
> bcabbccabffe73 Ohad Ben-Cohen       2011-10-20  863  
> bcabbccabffe73 Ohad Ben-Cohen       2011-10-20  864  	idr_init(&vrp->endpoints);
> bcabbccabffe73 Ohad Ben-Cohen       2011-10-20  865  	mutex_init(&vrp->endpoints_lock);
> bcabbccabffe73 Ohad Ben-Cohen       2011-10-20  866  	mutex_init(&vrp->tx_lock);
> bcabbccabffe73 Ohad Ben-Cohen       2011-10-20  867  	init_waitqueue_head(&vrp->sendq);
> bcabbccabffe73 Ohad Ben-Cohen       2011-10-20  868  
> bcabbccabffe73 Ohad Ben-Cohen       2011-10-20  869  	/* We expect two virtqueues, rx and tx (and in this order) */
> 9b2bbdb2275884 Michael S. Tsirkin   2017-03-06  870  	err = virtio_find_vqs(vdev, 2, vqs, vq_cbs, names, NULL);
> bcabbccabffe73 Ohad Ben-Cohen       2011-10-20  871  	if (err)
> bcabbccabffe73 Ohad Ben-Cohen       2011-10-20  872  		goto free_vrp;
> bcabbccabffe73 Ohad Ben-Cohen       2011-10-20  873  
> bcabbccabffe73 Ohad Ben-Cohen       2011-10-20  874  	vrp->rvq = vqs[0];
> bcabbccabffe73 Ohad Ben-Cohen       2011-10-20  875  	vrp->svq = vqs[1];
> bcabbccabffe73 Ohad Ben-Cohen       2011-10-20  876  
> b1b9891441fa33 Suman Anna           2014-09-16  877  	/* we expect symmetric tx/rx vrings */
> b1b9891441fa33 Suman Anna           2014-09-16  878  	WARN_ON(virtqueue_get_vring_size(vrp->rvq) !=
> b1b9891441fa33 Suman Anna           2014-09-16  879  		virtqueue_get_vring_size(vrp->svq));
> b1b9891441fa33 Suman Anna           2014-09-16  880  
> b1b9891441fa33 Suman Anna           2014-09-16  881  	/* we need less buffers if vrings are small */
> b1b9891441fa33 Suman Anna           2014-09-16  882  	if (virtqueue_get_vring_size(vrp->rvq) < MAX_RPMSG_NUM_BUFS / 2)
> b1b9891441fa33 Suman Anna           2014-09-16  883  		vrp->num_bufs = virtqueue_get_vring_size(vrp->rvq) * 2;
> b1b9891441fa33 Suman Anna           2014-09-16  884  	else
> b1b9891441fa33 Suman Anna           2014-09-16  885  		vrp->num_bufs = MAX_RPMSG_NUM_BUFS;
> b1b9891441fa33 Suman Anna           2014-09-16  886  
> f93848f9eeb0f8 Loic Pallardy        2017-03-28  887  	vrp->buf_size = MAX_RPMSG_BUF_SIZE;
> f93848f9eeb0f8 Loic Pallardy        2017-03-28  888  
> f93848f9eeb0f8 Loic Pallardy        2017-03-28  889  	total_buf_space = vrp->num_bufs * vrp->buf_size;
> b1b9891441fa33 Suman Anna           2014-09-16  890  
> bcabbccabffe73 Ohad Ben-Cohen       2011-10-20  891  	/* allocate coherent memory for the buffers */
> d999b622fcfb39 Loic Pallardy        2019-01-10  892  	bufs_va = dma_alloc_coherent(vdev->dev.parent,
> b1b9891441fa33 Suman Anna           2014-09-16  893  				     total_buf_space, &vrp->bufs_dma,
> b1b9891441fa33 Suman Anna           2014-09-16  894  				     GFP_KERNEL);
> 3119b487e03650 Wei Yongjun          2013-04-29  895  	if (!bufs_va) {
> 3119b487e03650 Wei Yongjun          2013-04-29  896  		err = -ENOMEM;
> bcabbccabffe73 Ohad Ben-Cohen       2011-10-20  897  		goto vqs_del;
> 3119b487e03650 Wei Yongjun          2013-04-29  898  	}
> bcabbccabffe73 Ohad Ben-Cohen       2011-10-20  899  
> de4064af76537f Suman Anna           2018-10-23  900  	dev_dbg(&vdev->dev, "buffers: va %pK, dma %pad\n",
> 8d95b322ba34b1 Anna, Suman          2016-08-12  901  		bufs_va, &vrp->bufs_dma);
> bcabbccabffe73 Ohad Ben-Cohen       2011-10-20  902  
> bcabbccabffe73 Ohad Ben-Cohen       2011-10-20  903  	/* half of the buffers is dedicated for RX */
> bcabbccabffe73 Ohad Ben-Cohen       2011-10-20  904  	vrp->rbufs = bufs_va;
> bcabbccabffe73 Ohad Ben-Cohen       2011-10-20  905  
> bcabbccabffe73 Ohad Ben-Cohen       2011-10-20  906  	/* and half is dedicated for TX */
> b1b9891441fa33 Suman Anna           2014-09-16  907  	vrp->sbufs = bufs_va + total_buf_space / 2;
> bcabbccabffe73 Ohad Ben-Cohen       2011-10-20  908  
> bcabbccabffe73 Ohad Ben-Cohen       2011-10-20  909  	/* set up the receive buffers */
> b1b9891441fa33 Suman Anna           2014-09-16  910  	for (i = 0; i < vrp->num_bufs / 2; i++) {
> bcabbccabffe73 Ohad Ben-Cohen       2011-10-20  911  		struct scatterlist sg;
> f93848f9eeb0f8 Loic Pallardy        2017-03-28  912  		void *cpu_addr = vrp->rbufs + i * vrp->buf_size;
> bcabbccabffe73 Ohad Ben-Cohen       2011-10-20  913  
> 9dd87c2af651b0 Loic Pallardy        2017-03-28  914  		rpmsg_sg_init(&sg, cpu_addr, vrp->buf_size);
> bcabbccabffe73 Ohad Ben-Cohen       2011-10-20  915  
> cee51d69a45b6c Rusty Russell        2013-03-20  916  		err = virtqueue_add_inbuf(vrp->rvq, &sg, 1, cpu_addr,
> bcabbccabffe73 Ohad Ben-Cohen       2011-10-20  917  					  GFP_KERNEL);
> 57e1a37347d31c Rusty Russell        2012-10-16  918  		WARN_ON(err); /* sanity check; this can't really happen */
> bcabbccabffe73 Ohad Ben-Cohen       2011-10-20  919  	}
> bcabbccabffe73 Ohad Ben-Cohen       2011-10-20  920  
> bcabbccabffe73 Ohad Ben-Cohen       2011-10-20  921  	/* suppress "tx-complete" interrupts */
> bcabbccabffe73 Ohad Ben-Cohen       2011-10-20  922  	virtqueue_disable_cb(vrp->svq);
> bcabbccabffe73 Ohad Ben-Cohen       2011-10-20  923  
> bcabbccabffe73 Ohad Ben-Cohen       2011-10-20  924  	vdev->priv = vrp;
> bcabbccabffe73 Ohad Ben-Cohen       2011-10-20  925  
> bcabbccabffe73 Ohad Ben-Cohen       2011-10-20  926  	/* if supported by the remote processor, enable the name service */
> bcabbccabffe73 Ohad Ben-Cohen       2011-10-20  927  	if (virtio_has_feature(vdev, VIRTIO_RPMSG_F_NS)) {
> 950a7388f02bf7 Arnaud Pouliquen     2020-11-20  928  		vch = kzalloc(sizeof(*vch), GFP_KERNEL);
> 950a7388f02bf7 Arnaud Pouliquen     2020-11-20  929  		if (!vch) {
> bcabbccabffe73 Ohad Ben-Cohen       2011-10-20  930  			err = -ENOMEM;
> bcabbccabffe73 Ohad Ben-Cohen       2011-10-20  931  			goto free_coherent;
> bcabbccabffe73 Ohad Ben-Cohen       2011-10-20  932  		}
> 950a7388f02bf7 Arnaud Pouliquen     2020-11-20  933  
> 950a7388f02bf7 Arnaud Pouliquen     2020-11-20  934  		/* Link the channel to our vrp */
> 950a7388f02bf7 Arnaud Pouliquen     2020-11-20  935  		vch->vrp = vrp;
> 950a7388f02bf7 Arnaud Pouliquen     2020-11-20  936  
> 950a7388f02bf7 Arnaud Pouliquen     2020-11-20  937  		/* Assign public information to the rpmsg_device */
> 950a7388f02bf7 Arnaud Pouliquen     2020-11-20  938  		rpdev_ns = &vch->rpdev;
> 950a7388f02bf7 Arnaud Pouliquen     2020-11-20  939  		rpdev_ns->ops = &virtio_rpmsg_ops;
> 950a7388f02bf7 Arnaud Pouliquen     2020-11-20  940  		rpdev_ns->little_endian = virtio_is_little_endian(vrp->vdev);
> 950a7388f02bf7 Arnaud Pouliquen     2020-11-20  941  
> 950a7388f02bf7 Arnaud Pouliquen     2020-11-20  942  		rpdev_ns->dev.parent = &vrp->vdev->dev;
> 950a7388f02bf7 Arnaud Pouliquen     2020-11-20  943  		rpdev_ns->dev.release = virtio_rpmsg_release_device;
> 950a7388f02bf7 Arnaud Pouliquen     2020-11-20  944  
> 950a7388f02bf7 Arnaud Pouliquen     2020-11-20  945  		err = rpmsg_ns_register_device(rpdev_ns);
> 950a7388f02bf7 Arnaud Pouliquen     2020-11-20  946  		if (err)
> 950a7388f02bf7 Arnaud Pouliquen     2020-11-20  947  			goto free_coherent;
> bcabbccabffe73 Ohad Ben-Cohen       2011-10-20  948  	}
> 
> "vch" not initialized on else path.  Also keep in mind that "rpdev_ctrl"
> is NULL at this point.
> 
> bcabbccabffe73 Ohad Ben-Cohen       2011-10-20  949  
> e3bba4363fe87b Arnaud Pouliquen     2021-02-17  950  	rpdev_ctrl = rpmsg_virtio_add_char_dev(vdev);
>                                                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> 
> e3bba4363fe87b Arnaud Pouliquen     2021-02-17  951  	if (IS_ERR(rpdev_ctrl)) {
> e3bba4363fe87b Arnaud Pouliquen     2021-02-17  952  		err = PTR_ERR(rpdev_ctrl);
> e3bba4363fe87b Arnaud Pouliquen     2021-02-17  953  		goto free_coherent;
> 
> I should create a Smatch warning for this as well.
> 
> e3bba4363fe87b Arnaud Pouliquen     2021-02-17  954  	}
> 71e4b8bf0482fc Michael S. Tsirkin   2015-03-12  955  	/*
> 71e4b8bf0482fc Michael S. Tsirkin   2015-03-12  956  	 * Prepare to kick but don't notify yet - we can't do this before
> 71e4b8bf0482fc Michael S. Tsirkin   2015-03-12  957  	 * device is ready.
> 71e4b8bf0482fc Michael S. Tsirkin   2015-03-12  958  	 */
> 71e4b8bf0482fc Michael S. Tsirkin   2015-03-12  959  	notify = virtqueue_kick_prepare(vrp->rvq);
> 71e4b8bf0482fc Michael S. Tsirkin   2015-03-12  960  
> 71e4b8bf0482fc Michael S. Tsirkin   2015-03-12  961  	/* From this point on, we can notify and get callbacks. */
> 71e4b8bf0482fc Michael S. Tsirkin   2015-03-12  962  	virtio_device_ready(vdev);
> 71e4b8bf0482fc Michael S. Tsirkin   2015-03-12  963  
> bcabbccabffe73 Ohad Ben-Cohen       2011-10-20  964  	/* tell the remote processor it can start sending messages */
> 71e4b8bf0482fc Michael S. Tsirkin   2015-03-12  965  	/*
> 71e4b8bf0482fc Michael S. Tsirkin   2015-03-12  966  	 * this might be concurrent with callbacks, but we are only
> 71e4b8bf0482fc Michael S. Tsirkin   2015-03-12  967  	 * doing notify, not a full kick here, so that's ok.
> 71e4b8bf0482fc Michael S. Tsirkin   2015-03-12  968  	 */
> 71e4b8bf0482fc Michael S. Tsirkin   2015-03-12  969  	if (notify)
> 71e4b8bf0482fc Michael S. Tsirkin   2015-03-12  970  		virtqueue_notify(vrp->rvq);
> bcabbccabffe73 Ohad Ben-Cohen       2011-10-20  971  
> bcabbccabffe73 Ohad Ben-Cohen       2011-10-20  972  	dev_info(&vdev->dev, "rpmsg host is online\n");
> bcabbccabffe73 Ohad Ben-Cohen       2011-10-20  973  
> bcabbccabffe73 Ohad Ben-Cohen       2011-10-20  974  	return 0;
> bcabbccabffe73 Ohad Ben-Cohen       2011-10-20  975  
> bcabbccabffe73 Ohad Ben-Cohen       2011-10-20  976  free_coherent:
> 950a7388f02bf7 Arnaud Pouliquen     2020-11-20 @977  	kfree(vch);
>                                                         ^^^^^^^^^^
> Uninitalized.
> 
> e3bba4363fe87b Arnaud Pouliquen     2021-02-17  978  	kfree(to_virtio_rpmsg_channel(rpdev_ctrl));
> 
> Now, let's say that "rpdev_ctrl" is NULL.  That's fine because
> to_virtio_rpmsg_channel() is a no-op so it becomes kfree(NULL) which
> is a no-op.  But why even have the to_virtio_rpmsg_channel() function
> if we are going to rely on it being a no-op?
> 
> If "rpdev_ctrl" is an error pointer this will crash.  The problem is we
> are freeing stuff that was not allocated.  The fix for this is:
> 
> 1) Free the most recent successful allocation.
> 2) The unwind code should mirror the allocation code.
> 3) Choose label names which say what the goto does.  If the most recent
>    allocation was "vch" the goto should be "goto free_vch;"
> 4) Every allocation function should have a matching free function.  It's
>    a layering violation to have to know that the internals of
>    rpmsg_virtio_add_char_dev().
> 
> So create function:
> 
> void rpmsg_virtio_del_char_dev(struct rpmsg_device *rpdev_ctrl)
> {
> 	if (!rpdev_ctrl)
> 		return;
> 	kfree(to_virtio_rpmsg_channel(rpdev_ctrl));
> }
> 
> The clean up code looks like this:
> 
> 	return 0;
> 
> free_vch:
> 	if (virtio_has_feature(vdev, VIRTIO_RPMSG_F_NS))
> 		kfree(vch);
> free_ctrl:
> 	rpmsg_virtio_del_char_dev(rpdev_ctrl);
> free_coherent:
> 	dma_free_coherent(vdev->dev.parent, total_buf_space,
> 			  bufs_va, vrp->bufs_dma);
> vqs_del:
> 	vdev->config->del_vqs(vrp->vdev);
> 
> Then just go through the function and as you read down the code keep
> track of the most recent successful allocation and goto the correct
> free label.

You're right, my error management is very bad here. I will fix this based on
your recommandation.

Thanks,
Arnaud

> 
> d999b622fcfb39 Loic Pallardy        2019-01-10  979  	dma_free_coherent(vdev->dev.parent, total_buf_space,
> eeb0074f36d1ab Fernando Guzman Lugo 2012-08-29  980  			  bufs_va, vrp->bufs_dma);
> bcabbccabffe73 Ohad Ben-Cohen       2011-10-20  981  vqs_del:
> bcabbccabffe73 Ohad Ben-Cohen       2011-10-20  982  	vdev->config->del_vqs(vrp->vdev);
> bcabbccabffe73 Ohad Ben-Cohen       2011-10-20  983  free_vrp:
> bcabbccabffe73 Ohad Ben-Cohen       2011-10-20  984  	kfree(vrp);
> bcabbccabffe73 Ohad Ben-Cohen       2011-10-20  985  	return err;
> bcabbccabffe73 Ohad Ben-Cohen       2011-10-20  986  }
> 
> ---
> 0-DAY CI Kernel Test Service, Intel Corporation
> https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
>
diff mbox series

Patch

diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
index e87d4cf926eb..5143fdeca306 100644
--- a/drivers/rpmsg/virtio_rpmsg_bus.c
+++ b/drivers/rpmsg/virtio_rpmsg_bus.c
@@ -813,6 +813,35 @@  static void rpmsg_xmit_done(struct virtqueue *svq)
 	wake_up_interruptible(&vrp->sendq);
 }
 
+static struct rpmsg_device *rpmsg_virtio_add_char_dev(struct virtio_device *vdev)
+{
+	struct virtproc_info *vrp = vdev->priv;
+	struct virtio_rpmsg_channel *vch;
+	struct rpmsg_device *rpdev_ctrl;
+	int err = 0;
+
+	vch = kzalloc(sizeof(*vch), GFP_KERNEL);
+	if (!vch)
+		return ERR_PTR(-ENOMEM);
+
+	/* Link the channel to the vrp */
+	vch->vrp = vrp;
+
+	/* Assign public information to the rpmsg_device */
+	rpdev_ctrl = &vch->rpdev;
+	rpdev_ctrl->ops = &virtio_rpmsg_ops;
+
+	rpdev_ctrl->dev.parent = &vrp->vdev->dev;
+	rpdev_ctrl->dev.release = virtio_rpmsg_release_device;
+	rpdev_ctrl->little_endian = virtio_is_little_endian(vrp->vdev);
+
+	err = rpmsg_ctrl_register_device(rpdev_ctrl);
+	if (err)
+		return ERR_PTR(err);
+
+	return rpdev_ctrl;
+}
+
 static int rpmsg_probe(struct virtio_device *vdev)
 {
 	vq_callback_t *vq_cbs[] = { rpmsg_recv_done, rpmsg_xmit_done };
@@ -820,7 +849,7 @@  static int rpmsg_probe(struct virtio_device *vdev)
 	struct virtqueue *vqs[2];
 	struct virtproc_info *vrp;
 	struct virtio_rpmsg_channel *vch;
-	struct rpmsg_device *rpdev_ns;
+	struct rpmsg_device *rpdev_ns = NULL, *rpdev_ctrl = NULL;
 	void *bufs_va;
 	int err = 0, i;
 	size_t total_buf_space;
@@ -918,6 +947,11 @@  static int rpmsg_probe(struct virtio_device *vdev)
 			goto free_coherent;
 	}
 
+	rpdev_ctrl = rpmsg_virtio_add_char_dev(vdev);
+	if (IS_ERR(rpdev_ctrl)) {
+		err = PTR_ERR(rpdev_ctrl);
+		goto free_coherent;
+	}
 	/*
 	 * Prepare to kick but don't notify yet - we can't do this before
 	 * device is ready.
@@ -941,6 +975,7 @@  static int rpmsg_probe(struct virtio_device *vdev)
 
 free_coherent:
 	kfree(vch);
+	kfree(to_virtio_rpmsg_channel(rpdev_ctrl));
 	dma_free_coherent(vdev->dev.parent, total_buf_space,
 			  bufs_va, vrp->bufs_dma);
 vqs_del: