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 <[email protected]> > Reported-by: Dan Carpenter <[email protected]> > > 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/[email protected] >
