On Thu, Aug 29, 2024 at 2:22 PM Dan Carpenter <dan.carpenter@xxxxxxxxxx> wrote:
>
> Hello Frank Rowand,
Frank has retired... Stephen is who got this upstream.
> Commit d1eabd218ede ("of: unittest: treat missing of_root as error
> instead of fixing up") from Feb 16, 2024 (linux-next), leads to the
> following Smatch static checker warning:
>
> drivers/of/unittest.c:1934 unittest_data_add()
> warn: inconsistent returns '&of_overlay_phandle_mutex'.
>
> drivers/of/unittest.c
> 1857 static int __init unittest_data_add(void)
> 1858 {
> 1859 void *unittest_data;
> 1860 void *unittest_data_align;
> 1861 struct device_node *unittest_data_node = NULL, *np;
> 1862 /*
> 1863 * __dtbo_testcases_begin[] and __dtbo_testcases_end[] are magically
> 1864 * created by cmd_dt_S_dtbo in scripts/Makefile.lib
> 1865 */
> 1866 extern uint8_t __dtbo_testcases_begin[];
> 1867 extern uint8_t __dtbo_testcases_end[];
> 1868 const int size = __dtbo_testcases_end - __dtbo_testcases_begin;
> 1869 int rc;
> 1870 void *ret;
> 1871
> 1872 if (!size) {
> 1873 pr_warn("%s: testcases is empty\n", __func__);
> 1874 return -ENODATA;
> 1875 }
> 1876
> 1877 /* creating copy */
> 1878 unittest_data = kmalloc(size + FDT_ALIGN_SIZE, GFP_KERNEL);
> 1879 if (!unittest_data)
> 1880 return -ENOMEM;
> 1881
> 1882 unittest_data_align = PTR_ALIGN(unittest_data, FDT_ALIGN_SIZE);
>
> We never save unittest_data anywhere so how is this freed on the success path?
We can't because the tree continues to be accessed. Also, if we run
the unittest, the kernel is tainted.
> 1883 memcpy(unittest_data_align, __dtbo_testcases_begin, size);
> 1884
> 1885 ret = of_fdt_unflatten_tree(unittest_data_align, NULL, &unittest_data_node);
> 1886 if (!ret) {
> 1887 pr_warn("%s: unflatten testcases tree failed\n", __func__);
> 1888 kfree(unittest_data);
> 1889 return -ENODATA;
> 1890 }
> 1891 if (!unittest_data_node) {
> 1892 pr_warn("%s: testcases tree is empty\n", __func__);
> 1893 kfree(unittest_data);
> 1894 return -ENODATA;
> 1895 }
> 1896
> 1897 /*
> 1898 * This lock normally encloses of_resolve_phandles()
> 1899 */
> 1900 of_overlay_mutex_lock();
> ^^^^^^^^^^^^^^^^^^^^^^^^
>
> 1901
> 1902 rc = of_resolve_phandles(unittest_data_node);
> 1903 if (rc) {
> 1904 pr_err("%s: Failed to resolve phandles (rc=%i)\n", __func__, rc);
> 1905 of_overlay_mutex_unlock();
>
> kfree(unittest_data);?
Yes, we should.
>
>
> 1906 return -EINVAL;
> 1907 }
> 1908
> 1909 /* attach the sub-tree to live tree */
> 1910 if (!of_root) {
> 1911 pr_warn("%s: no live tree to attach sub-tree\n", __func__);
> 1912 kfree(unittest_data);
>
> This used to call of_overlay_mutex_unlock(). I think that was deleted
> accidentally.
Agreed. But as nothing is done for this condition any more, we could
either just drop it (of_root should never be NULL) or move the check
to the beginning of the function.
Rob
