Re: [PATCH v4 11/16] rpmsg: virtio: register the rpmsg

All of lore.kernel.org
 help / color / mirror / Atom feed

* Re: [PATCH v4 11/16] rpmsg: virtio: register the rpmsg_ctrl device
@ 2021-02-17 17:07 kernel test robot
  0 siblings, 0 replies; 6+ messages in thread
From: kernel test robot @ 2021-02-17 17:07 UTC (permalink / raw)
  To: kbuild

[-- Attachment #1: Type: text/plain, Size: 13301 bytes --]

CC: kbuild-all(a)lists.01.org
In-Reply-To: <20210217132905.1485-12-arnaud.pouliquen@foss.st.com>
References: <20210217132905.1485-12-arnaud.pouliquen@foss.st.com>
TO: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
TO: Bjorn Andersson <bjorn.andersson@linaro.org>
TO: "Ohad Ben-Cohen" <ohad@wizery.com>
TO: Mathieu Poirier <mathieu.poirier@linaro.org>
TO: Andy Gross <agross@kernel.org>
CC: linux-remoteproc(a)vger.kernel.org
CC: linux-kernel(a)vger.kernel.org
CC: linux-stm32(a)st-md-mailman.stormreply.com
CC: linux-arm-msm(a)vger.kernel.org
CC: arnaud.pouliquen(a)foss.st.com

Hi Arnaud,

I love your patch! Perhaps something to improve:

[auto build test WARNING on linus/master]
[also build test WARNING on v5.11 next-20210216]
[cannot apply to rpmsg/for-next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

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
:::::: branch date: 3 hours ago
:::::: commit date: 3 hours ago
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

e3bba4363fe87b Arnaud Pouliquen     2021-02-17  844  
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);
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  	}
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;
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);
e3bba4363fe87b Arnaud Pouliquen     2021-02-17  978  	kfree(to_virtio_rpmsg_channel(rpdev_ctrl));
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  }
bcabbccabffe73 Ohad Ben-Cohen       2011-10-20  987  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 31010 bytes --]

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH v4 00/16] introduce a generic IOCTL interface for RPMsg channels management
@ 2021-02-17 13:28 Arnaud Pouliquen
  2021-02-17 13:29 ` [PATCH v4 11/16] rpmsg: virtio: register the rpmsg_ctrl device Arnaud Pouliquen
  0 siblings, 1 reply; 6+ messages in thread
From: Arnaud Pouliquen @ 2021-02-17 13:28 UTC (permalink / raw)
  To: Bjorn Andersson, Ohad Ben-Cohen, Mathieu Poirier, Andy Gross
  Cc: linux-remoteproc, linux-kernel, linux-stm32, linux-arm-msm,
	arnaud.pouliquen

This series restructures the RPMsg char driver to decorrelate the control part and to
create a generic RPMsg ioctl interface compatible with other RPMsg services.

The V4 fixes compilation issue reported by the kernel test robot <lkp@intel.com>

The V3 is based on the guideline proposed by Mathieu Poirier to keep as much as possible
the legacy implementation of the rpmsg_char used by the GLINK and SMD platforms.

Objectives of the series:
- Allow to create a service from Linux user application:
  - with a specific name
  - with or without name service announcement.
- Allow to probe the same service by receiving either a NS announcement from the remote firmware
  or a Linux user application request.
- Use these services independently of the RPMsg transport implementation (e.g be able to use
  RPMSg char with the RPMsg virtio bus).

Steps in the series:
  - Extract the control part of the char dev and create the rpmsg_ctrl.c file (patches 1 to 5)
  - Enable the use of the chardev with the virtio backend (patches 6 to 10)
  - Introduce the RPMSG_CREATE_DEV_IOCTL IOCTL to instantiate RPMsg devices (patch 11)
    The application can then create or release a channel by specifying:
       - the name service of the device to instantiate.   
       - the source address.
       - the destination address.
  - Instantiate the /dev/rpmsg interface on remote NS announcement (patches 12 to 15)

In this revision, I do not divide the series into several parts in order to show a complete
picture of the proposed evolution. To simplify the review, as a next step, I can send it in
several steps listed above.

Known current Limitations:
- Tested only with virtio RPMsg bus. The glink and smd drivers adaptations have not been tested
  (not able to test it).
- For the virtio backend: No NS announcement is sent to the remote processor if the source
  address is set to RPMSG_ADDR_ANY.
- For the virtio backend: the existing RPMSG_CREATE_EPT_IOCTL is working but the endpoints are
  not attached to an exiting channel.
- to limit patches the pending RPMSG_DESTROY_DEV_IOCTL has not ben implemented. This will be
  proposed in a second step.

This series can be applied on git/andersson/remoteproc.git for-next branch (d9ff3a5789cb).

This series can be tested using rpmsgexport, rpmsgcreatedev and ping tools available here:
https://github.com/arnopo/rpmsgexport.git

Reference to the V3 discussion thread: https://lkml.org/lkml/2021/2/4/194

Arnaud Pouliquen (16):
  rpmsg: char: rename rpmsg_char_init to rpmsg_chrdev_init
  rpmsg: move RPMSG_ADDR_ANY in user API
  rpmsg: add short description of the IOCTL defined in UAPI.
  rpmsg: char: export eptdev create an destroy functions
  rpmsg: char: dissociate the control device from the rpmsg class
  rpmsg: move the rpmsg control device from rpmsg_char to rpmsg_ctrl
  rpmsg: update rpmsg_chrdev_register_device function
  rpmsg: glink: add sendto and trysendto ops
  rpmsg: smd: add sendto and trysendto ops
  rpmsg: char: use sendto to specify the message destination address
  rpmsg: virtio: register the rpmsg_ctrl device
  rpmsg: ctrl: introduce RPMSG_CREATE_DEV_IOCTL
  rpmsg: char: introduce __rpmsg_chrdev_create_eptdev function
  rpmsg: char: introduce a RPMsg driver for the RPMsg char device
  rpmsg: char: no dynamic endpoint management for the default one
  rpmsg: char: return an error if device already open

 drivers/rpmsg/Kconfig             |   9 ++
 drivers/rpmsg/Makefile            |   1 +
 drivers/rpmsg/qcom_glink_native.c |  18 ++-
 drivers/rpmsg/qcom_smd.c          |  18 ++-
 drivers/rpmsg/rpmsg_char.c        | 237 +++++++++++-------------------
 drivers/rpmsg/rpmsg_char.h        |  51 +++++++
 drivers/rpmsg/rpmsg_ctrl.c        | 229 +++++++++++++++++++++++++++++
 drivers/rpmsg/rpmsg_internal.h    |  10 +-
 drivers/rpmsg/virtio_rpmsg_bus.c  |  37 ++++-
 include/linux/rpmsg.h             |   3 +-
 include/uapi/linux/rpmsg.h        |  18 ++-
 11 files changed, 469 insertions(+), 162 deletions(-)
 create mode 100644 drivers/rpmsg/rpmsg_char.h
 create mode 100644 drivers/rpmsg/rpmsg_ctrl.c

-- 
2.17.1

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH v4 11/16] rpmsg: virtio: register the rpmsg_ctrl device
  2021-02-17 13:28 [PATCH v4 00/16] introduce a generic IOCTL interface for RPMsg channels management Arnaud Pouliquen
@ 2021-02-17 13:29 ` Arnaud Pouliquen
  2021-02-18 12:27     ` Dan Carpenter
  0 siblings, 1 reply; 6+ messages in thread
From: Arnaud Pouliquen @ 2021-02-17 13:29 UTC (permalink / raw)
  To: Bjorn Andersson, Ohad Ben-Cohen, Mathieu Poirier, Andy Gross
  Cc: linux-remoteproc, linux-kernel, linux-stm32, linux-arm-msm,
	arnaud.pouliquen

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(-)

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:
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH v4 11/16] rpmsg: virtio: register the rpmsg_ctrl device
  2021-02-17 13:29 ` [PATCH v4 11/16] rpmsg: virtio: register the rpmsg_ctrl device Arnaud Pouliquen
  2021-02-18 12:27     ` Dan Carpenter
@ 2021-02-18 12:27     ` Dan Carpenter
  0 siblings, 0 replies; 6+ messages in thread
From: Dan Carpenter @ 2021-02-18 12:27 UTC (permalink / raw)
  To: kbuild, Arnaud Pouliquen, Bjorn Andersson, Ohad Ben-Cohen,
	Mathieu Poirier, Andy Gross
  Cc: lkp, kbuild-all, linux-remoteproc, linux-kernel, linux-stm32,
	linux-arm-msm, arnaud.pouliquen

[-- Attachment #1: Type: text/plain, Size: 13919 bytes --]

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

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 31010 bytes --]

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v4 11/16] rpmsg: virtio: register the rpmsg_ctrl device
@ 2021-02-18 12:27     ` Dan Carpenter
  0 siblings, 0 replies; 6+ messages in thread
From: Dan Carpenter @ 2021-02-18 12:27 UTC (permalink / raw)
  To: kbuild-all

[-- Attachment #1: Type: text/plain, Size: 14145 bytes --]

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(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 31010 bytes --]

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v4 11/16] rpmsg: virtio: register the rpmsg_ctrl device
@ 2021-02-18 12:27     ` Dan Carpenter
  0 siblings, 0 replies; 6+ messages in thread
From: Dan Carpenter @ 2021-02-18 12:27 UTC (permalink / raw)
  To: kbuild

[-- Attachment #1: Type: text/plain, Size: 14145 bytes --]

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(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 31010 bytes --]

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v4 11/16] rpmsg: virtio: register the rpmsg_ctrl device
  2021-02-18 12:27     ` Dan Carpenter
  (?)
  (?)
@ 2021-02-18 17:14     ` Arnaud POULIQUEN
  -1 siblings, 0 replies; 6+ messages in thread
From: Arnaud POULIQUEN @ 2021-02-18 17:14 UTC (permalink / raw)
  To: Dan Carpenter, kbuild, Bjorn Andersson, Ohad Ben-Cohen,
	Mathieu Poirier, Andy Gross
  Cc: lkp, kbuild-all, linux-remoteproc, linux-kernel, linux-stm32,
	linux-arm-msm

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
> 

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2021-02-18 17:48 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-02-17 17:07 [PATCH v4 11/16] rpmsg: virtio: register the rpmsg_ctrl device kernel test robot
  -- strict thread matches above, loose matches on Subject: below --
2021-02-17 13:28 [PATCH v4 00/16] introduce a generic IOCTL interface for RPMsg channels management Arnaud Pouliquen
2021-02-17 13:29 ` [PATCH v4 11/16] rpmsg: virtio: register the rpmsg_ctrl device Arnaud Pouliquen
2021-02-18 12:27   ` Dan Carpenter
2021-02-18 12:27     ` Dan Carpenter
2021-02-18 12:27     ` Dan Carpenter
2021-02-18 17:14     ` Arnaud POULIQUEN

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.