Messages in this thread | | | From | Mike Leach <> | Date | Wed, 17 Jun 2020 10:20:15 +0100 | Subject | Re: [kbuild] drivers/hwtracing/coresight/coresight-cti.c:862 cti_probe() error: we previously assumed 'drvdata' could be null (see line 759) |
| |
Hi Dan,
Thanks for the comprehensive review of this.
On Fri, 12 Jun 2020 at 14:47, Dan Carpenter <dan.carpenter@oracle.com> wrote: > > > Hi Mike, > > Here is the buggy line: > > 861 err_out: > 862 cti_pm_release(drvdata); > ^^^^^^^ >
Yes - caling cti_pm_release() if drvdata == NULL would result in bad things happening!
I would certainly be aiming to fix this, but as you have provided a patch on another mail - I have reviewed and compiled that, so will respond further there
Regards
Mike
> To me it's a red flag any time there is a label called "out:". The > label should say what is being released like "goto unregister_notifier;". > The style of error handling here is called a "free everything function" > and it is the most error prone style of error handling. > > A better way to write error handling is to track the most recently > allocated resource and free it with a well named goto. > > a = alloc(); > if (!a) > return -ENOMEM; > > b = alloc(); > if (!b) { > ret = -ENOMEM; > goto free_a; > } > > c = alloc(); > if (!c) { > ret = -ENOMEM; > goto free_b; > > ... > return 0; > > free_b: > free(b); > free_a: > free(a); > > The advantage of this is that 1) You only have to track the most recent > allocation. 2) You can easily verify that the most recent allocation > is freed. 3) Now you can create a free function by copy and pasting > and adding a free(c); > > void my_free(struct whatever *p) > { > free(c); > free(b); > free(a); > } > > This style uses about the same number of lines of code because although > we duplicate the free(b) and free(a), we can remove some if statements > so it ends up being about the same. > > The main problem with free everything function is that they free things > which have not been allocated. I have added more explanation at the > bottom of this bug report. > > I am also sending a patch which hopefully is clear but I can't actually > compile it. :( > > tree: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master > head: b791d1bdf9212d944d749a5c7ff6febdba241771 > commit: e9b880581d555c8f7b58c7d19cc3f8f9016a1b5f coresight: cti: Add CPU Hotplug handling to CTI driver > config: arm-randconfig-m031-20200612 (attached as .config) > compiler: arm-linux-gnueabi-gcc (GCC) 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/hwtracing/coresight/coresight-cti.c:862 cti_probe() error: we previously assumed 'drvdata' could be null (see line 759) > > # https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=e9b880581d555c8f7b58c7d19cc3f8f9016a1b5f > git remote add linus https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git > git remote update linus > git checkout e9b880581d555c8f7b58c7d19cc3f8f9016a1b5f > vim +/drvdata +862 drivers/hwtracing/coresight/coresight-cti.c > > 835d722ba10ac92 Mike Leach 2020-03-20 747 static int cti_probe(struct amba_device *adev, const struct amba_id *id) > 835d722ba10ac92 Mike Leach 2020-03-20 748 { > 835d722ba10ac92 Mike Leach 2020-03-20 749 int ret = 0; > 835d722ba10ac92 Mike Leach 2020-03-20 750 void __iomem *base; > 835d722ba10ac92 Mike Leach 2020-03-20 751 struct device *dev = &adev->dev; > 835d722ba10ac92 Mike Leach 2020-03-20 752 struct cti_drvdata *drvdata = NULL; > 835d722ba10ac92 Mike Leach 2020-03-20 753 struct coresight_desc cti_desc; > 835d722ba10ac92 Mike Leach 2020-03-20 754 struct coresight_platform_data *pdata = NULL; > 835d722ba10ac92 Mike Leach 2020-03-20 755 struct resource *res = &adev->res; > 835d722ba10ac92 Mike Leach 2020-03-20 756 > 835d722ba10ac92 Mike Leach 2020-03-20 757 /* driver data*/ > 835d722ba10ac92 Mike Leach 2020-03-20 758 drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL); > 835d722ba10ac92 Mike Leach 2020-03-20 @759 if (!drvdata) { > 835d722ba10ac92 Mike Leach 2020-03-20 760 ret = -ENOMEM; > 835d722ba10ac92 Mike Leach 2020-03-20 761 dev_info(dev, "%s, mem err\n", __func__); > > No need to print an error message for kmalloc() failures. It already > has a stack trace built in. > > 835d722ba10ac92 Mike Leach 2020-03-20 762 goto err_out; > ^^^^^^^^^^^^ > > 835d722ba10ac92 Mike Leach 2020-03-20 763 } > 835d722ba10ac92 Mike Leach 2020-03-20 764 > 835d722ba10ac92 Mike Leach 2020-03-20 765 /* Validity for the resource is already checked by the AMBA core */ > 835d722ba10ac92 Mike Leach 2020-03-20 766 base = devm_ioremap_resource(dev, res); > 835d722ba10ac92 Mike Leach 2020-03-20 767 if (IS_ERR(base)) { > 835d722ba10ac92 Mike Leach 2020-03-20 768 ret = PTR_ERR(base); > 835d722ba10ac92 Mike Leach 2020-03-20 769 dev_err(dev, "%s, remap err\n", __func__); > > At this point "drvdata->ctidev.cpu" is zero. > > 835d722ba10ac92 Mike Leach 2020-03-20 770 goto err_out; > 835d722ba10ac92 Mike Leach 2020-03-20 771 } > 835d722ba10ac92 Mike Leach 2020-03-20 772 drvdata->base = base; > 835d722ba10ac92 Mike Leach 2020-03-20 773 > 835d722ba10ac92 Mike Leach 2020-03-20 774 dev_set_drvdata(dev, drvdata); > 835d722ba10ac92 Mike Leach 2020-03-20 775 > 835d722ba10ac92 Mike Leach 2020-03-20 776 /* default CTI device info */ > 835d722ba10ac92 Mike Leach 2020-03-20 777 drvdata->ctidev.cpu = -1; > ^^^^^^^^^^^^^^^^^^^^^^^^ > > 835d722ba10ac92 Mike Leach 2020-03-20 778 drvdata->ctidev.nr_trig_con = 0; > 835d722ba10ac92 Mike Leach 2020-03-20 779 drvdata->ctidev.ctm_id = 0; > 835d722ba10ac92 Mike Leach 2020-03-20 780 INIT_LIST_HEAD(&drvdata->ctidev.trig_cons); > 835d722ba10ac92 Mike Leach 2020-03-20 781 > 835d722ba10ac92 Mike Leach 2020-03-20 782 spin_lock_init(&drvdata->spinlock); > 835d722ba10ac92 Mike Leach 2020-03-20 783 > 835d722ba10ac92 Mike Leach 2020-03-20 784 /* initialise CTI driver config values */ > 835d722ba10ac92 Mike Leach 2020-03-20 785 cti_set_default_config(dev, drvdata); > 835d722ba10ac92 Mike Leach 2020-03-20 786 > 835d722ba10ac92 Mike Leach 2020-03-20 787 pdata = coresight_cti_get_platform_data(dev); > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > This function sets drvdata->ctidev.cpu on some success paths and also > on certain failure paths. > > 835d722ba10ac92 Mike Leach 2020-03-20 788 if (IS_ERR(pdata)) { > 835d722ba10ac92 Mike Leach 2020-03-20 789 dev_err(dev, "coresight_cti_get_platform_data err\n"); > 835d722ba10ac92 Mike Leach 2020-03-20 790 ret = PTR_ERR(pdata); > 835d722ba10ac92 Mike Leach 2020-03-20 791 goto err_out; > 835d722ba10ac92 Mike Leach 2020-03-20 792 } > 835d722ba10ac92 Mike Leach 2020-03-20 793 > 835d722ba10ac92 Mike Leach 2020-03-20 794 /* default to powered - could change on PM notifications */ > 835d722ba10ac92 Mike Leach 2020-03-20 795 drvdata->config.hw_powered = true; > 835d722ba10ac92 Mike Leach 2020-03-20 796 > 835d722ba10ac92 Mike Leach 2020-03-20 797 /* set up device name - will depend if cpu bound or otherwise */ > 835d722ba10ac92 Mike Leach 2020-03-20 798 if (drvdata->ctidev.cpu >= 0) > 835d722ba10ac92 Mike Leach 2020-03-20 799 cti_desc.name = devm_kasprintf(dev, GFP_KERNEL, "cti_cpu%d", > 835d722ba10ac92 Mike Leach 2020-03-20 800 drvdata->ctidev.cpu); > 835d722ba10ac92 Mike Leach 2020-03-20 801 else > 835d722ba10ac92 Mike Leach 2020-03-20 802 cti_desc.name = coresight_alloc_device_name(&cti_sys_devs, dev); > 835d722ba10ac92 Mike Leach 2020-03-20 803 if (!cti_desc.name) { > 835d722ba10ac92 Mike Leach 2020-03-20 804 ret = -ENOMEM; > 835d722ba10ac92 Mike Leach 2020-03-20 805 goto err_out; > 835d722ba10ac92 Mike Leach 2020-03-20 806 } > 835d722ba10ac92 Mike Leach 2020-03-20 807 > e9b880581d555c8 Mike Leach 2020-05-18 808 /* setup CPU power management handling for CPU bound CTI devices. */ > e9b880581d555c8 Mike Leach 2020-05-18 809 if (drvdata->ctidev.cpu >= 0) { > e9b880581d555c8 Mike Leach 2020-05-18 810 cti_cpu_drvdata[drvdata->ctidev.cpu] = drvdata; > e9b880581d555c8 Mike Leach 2020-05-18 811 if (!nr_cti_cpu++) { > ^^^^^^^^^^^^ > > e9b880581d555c8 Mike Leach 2020-05-18 812 cpus_read_lock(); > e9b880581d555c8 Mike Leach 2020-05-18 813 ret = cpuhp_setup_state_nocalls_cpuslocked( > e9b880581d555c8 Mike Leach 2020-05-18 814 CPUHP_AP_ARM_CORESIGHT_CTI_STARTING, > e9b880581d555c8 Mike Leach 2020-05-18 815 "arm/coresight_cti:starting", > e9b880581d555c8 Mike Leach 2020-05-18 816 cti_starting_cpu, cti_dying_cpu); > e9b880581d555c8 Mike Leach 2020-05-18 817 > e9b880581d555c8 Mike Leach 2020-05-18 818 cpus_read_unlock(); > e9b880581d555c8 Mike Leach 2020-05-18 819 if (ret) > e9b880581d555c8 Mike Leach 2020-05-18 820 goto err_out; > e9b880581d555c8 Mike Leach 2020-05-18 821 } > e9b880581d555c8 Mike Leach 2020-05-18 822 } > e9b880581d555c8 Mike Leach 2020-05-18 823 > 3c5597e398124e6 Mike Leach 2020-03-20 824 /* create dynamic attributes for connections */ > 3c5597e398124e6 Mike Leach 2020-03-20 825 ret = cti_create_cons_sysfs(dev, drvdata); > 3c5597e398124e6 Mike Leach 2020-03-20 826 if (ret) { > 3c5597e398124e6 Mike Leach 2020-03-20 827 dev_err(dev, "%s: create dynamic sysfs entries failed\n", > 3c5597e398124e6 Mike Leach 2020-03-20 828 cti_desc.name); > 3c5597e398124e6 Mike Leach 2020-03-20 829 goto err_out; > 3c5597e398124e6 Mike Leach 2020-03-20 830 } > 3c5597e398124e6 Mike Leach 2020-03-20 831 > 835d722ba10ac92 Mike Leach 2020-03-20 832 /* set up coresight component description */ > 835d722ba10ac92 Mike Leach 2020-03-20 833 cti_desc.pdata = pdata; > 835d722ba10ac92 Mike Leach 2020-03-20 834 cti_desc.type = CORESIGHT_DEV_TYPE_ECT; > 835d722ba10ac92 Mike Leach 2020-03-20 835 cti_desc.subtype.ect_subtype = CORESIGHT_DEV_SUBTYPE_ECT_CTI; > 835d722ba10ac92 Mike Leach 2020-03-20 836 cti_desc.ops = &cti_ops; > 3c5597e398124e6 Mike Leach 2020-03-20 837 cti_desc.groups = drvdata->ctidev.con_groups; > 835d722ba10ac92 Mike Leach 2020-03-20 838 cti_desc.dev = dev; > 835d722ba10ac92 Mike Leach 2020-03-20 839 drvdata->csdev = coresight_register(&cti_desc); > 835d722ba10ac92 Mike Leach 2020-03-20 840 if (IS_ERR(drvdata->csdev)) { > 835d722ba10ac92 Mike Leach 2020-03-20 841 ret = PTR_ERR(drvdata->csdev); > 835d722ba10ac92 Mike Leach 2020-03-20 842 goto err_out; > 835d722ba10ac92 Mike Leach 2020-03-20 843 } > 835d722ba10ac92 Mike Leach 2020-03-20 844 > 835d722ba10ac92 Mike Leach 2020-03-20 845 /* add to list of CTI devices */ > 835d722ba10ac92 Mike Leach 2020-03-20 846 mutex_lock(&ect_mutex); > 835d722ba10ac92 Mike Leach 2020-03-20 847 list_add(&drvdata->node, &ect_net); > 177af8285b59a38 Mike Leach 2020-03-20 848 /* set any cross references */ > 177af8285b59a38 Mike Leach 2020-03-20 849 cti_update_conn_xrefs(drvdata); > 835d722ba10ac92 Mike Leach 2020-03-20 850 mutex_unlock(&ect_mutex); > 835d722ba10ac92 Mike Leach 2020-03-20 851 > 835d722ba10ac92 Mike Leach 2020-03-20 852 /* set up release chain */ > 835d722ba10ac92 Mike Leach 2020-03-20 853 drvdata->csdev_release = drvdata->csdev->dev.release; > 835d722ba10ac92 Mike Leach 2020-03-20 854 drvdata->csdev->dev.release = cti_device_release; > 835d722ba10ac92 Mike Leach 2020-03-20 855 > 835d722ba10ac92 Mike Leach 2020-03-20 856 /* all done - dec pm refcount */ > 835d722ba10ac92 Mike Leach 2020-03-20 857 pm_runtime_put(&adev->dev); > 835d722ba10ac92 Mike Leach 2020-03-20 858 dev_info(&drvdata->csdev->dev, "CTI initialized\n"); > 835d722ba10ac92 Mike Leach 2020-03-20 859 return 0; > 835d722ba10ac92 Mike Leach 2020-03-20 860 > 835d722ba10ac92 Mike Leach 2020-03-20 861 err_out: > e9b880581d555c8 Mike Leach 2020-05-18 @862 cti_pm_release(drvdata); > ^^^^^^^ > 835d722ba10ac92 Mike Leach 2020-03-20 863 return ret; > 835d722ba10ac92 Mike Leach 2020-03-20 864 } > > > 750 /* release PM registrations */ > 751 static void cti_pm_release(struct cti_drvdata *drvdata) > 752 { > 753 if (drvdata->ctidev.cpu >= 0) { > ^^^^^^^ > We are dereferencing this when it wasn't allocated. > > 754 if (--nr_cti_cpu == 0) { > ^^^^^^^^^^^^ > If devm_kasprintf() fails then we are decrementing this when it wasn't > incremented so now it can be negative. > > 755 cpu_pm_unregister_notifier(&cti_cpu_pm_nb); > > If the cpu_pm_register_notifier() fails then we are unregistering this > when it wasn't registered. It turns out this is harmless but if we > only free things which have been allocated then it becomes a lot > easier to audit the code. > > 756 > 757 cpuhp_remove_state_nocalls( > 758 CPUHP_AP_ARM_CORESIGHT_CTI_STARTING); > > If cpuhp_setup_state_nocalls_cpuslocked() failed then this wasn't > allocated. I believe this is harmless. > > 759 } > 760 cti_cpu_drvdata[drvdata->ctidev.cpu] = NULL; > 761 } > 762 } > > --- > 0-DAY CI Kernel Test Service, Intel Corporation > https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org > _______________________________________________ > kbuild mailing list -- kbuild@lists.01.org > To unsubscribe send an email to kbuild-leave@lists.01.org
-- Mike Leach Principal Engineer, ARM Ltd. Manchester Design Centre. UK
| |