Hello Ritvik Budhiraja,
Commit 173217bd7336 ("smb3: retrying on failed server close") from
Apr 2, 2024 (linux-next), leads to the following Smatch static
checker warning:
fs/smb/client/cifsfs.c:1981 init_cifs()
error: we previously assumed 'serverclose_wq' could be null (see line 1895)
I've written a blog about best practices for how kernel unwind ladders are
normally written.
https://staticthinking.wordpress.com/2022/04/28/free-the-last-thing-style/
fs/smb/client/cifsfs.c
1800 static int __init
1801 init_cifs(void)
1802 {
1803 int rc = 0;
1804 cifs_proc_init();
This creates proc files.
1805 INIT_LIST_HEAD(&cifs_tcp_ses_list);
1806 /*
1807 * Initialize Global counters
1808 */
1809 atomic_set(&sesInfoAllocCount, 0);
1810 atomic_set(&tconInfoAllocCount, 0);
1811 atomic_set(&tcpSesNextId, 0);
1812 atomic_set(&tcpSesAllocCount, 0);
1813 atomic_set(&tcpSesReconnectCount, 0);
1814 atomic_set(&tconInfoReconnectCount, 0);
1815
1816 atomic_set(&buf_alloc_count, 0);
1817 atomic_set(&small_buf_alloc_count, 0);
1818 #ifdef CONFIG_CIFS_STATS2
1819 atomic_set(&total_buf_alloc_count, 0);
1820 atomic_set(&total_small_buf_alloc_count, 0);
1821 if (slow_rsp_threshold < 1)
1822 cifs_dbg(FYI, "slow_response_threshold msgs disabled\n");
1823 else if (slow_rsp_threshold > 32767)
1824 cifs_dbg(VFS,
1825 "slow response threshold set higher than recommended (0 to 32767)\n");
1826 #endif /* CONFIG_CIFS_STATS2 */
1827
1828 atomic_set(&mid_count, 0);
1829 GlobalCurrentXid = 0;
1830 GlobalTotalActiveXid = 0;
1831 GlobalMaxActiveXid = 0;
1832 spin_lock_init(&cifs_tcp_ses_lock);
1833 spin_lock_init(&GlobalMid_Lock);
1834
1835 cifs_lock_secret = get_random_u32();
1836
1837 if (cifs_max_pending < 2) {
1838 cifs_max_pending = 2;
1839 cifs_dbg(FYI, "cifs_max_pending set to min of 2\n");
1840 } else if (cifs_max_pending > CIFS_MAX_REQ) {
1841 cifs_max_pending = CIFS_MAX_REQ;
1842 cifs_dbg(FYI, "cifs_max_pending set to max of %u\n",
1843 CIFS_MAX_REQ);
1844 }
1845
1846 /* Limit max to about 18 hours, and setting to zero disables directory entry caching */
1847 if (dir_cache_timeout > 65000) {
1848 dir_cache_timeout = 65000;
1849 cifs_dbg(VFS, "dir_cache_timeout set to max of 65000 seconds\n");
1850 }
1851
1852 cifsiod_wq = alloc_workqueue("cifsiod", WQ_FREEZABLE|WQ_MEM_RECLAIM, 0);
1853 if (!cifsiod_wq) {
1854 rc = -ENOMEM;
1855 goto out_clean_proc;
Generally I can tell from these gotos that they free the last resource. This
one deletes the proc files.
1856 }
1857
1858 /*
1859 * Consider in future setting limit!=0 maybe to min(num_of_cores - 1, 3)
1860 * so that we don't launch too many worker threads but
1861 * Documentation/core-api/workqueue.rst recommends setting it to 0
1862 */
1863
1864 /* WQ_UNBOUND allows decrypt tasks to run on any CPU */
1865 decrypt_wq = alloc_workqueue("smb3decryptd",
1866 WQ_UNBOUND|WQ_FREEZABLE|WQ_MEM_RECLAIM, 0);
1867 if (!decrypt_wq) {
1868 rc = -ENOMEM;
1869 goto out_destroy_cifsiod_wq;
cifsiod_wq alloc and destroyed. Fine.
1870 }
1871
1872 fileinfo_put_wq = alloc_workqueue("cifsfileinfoput",
1873 WQ_UNBOUND|WQ_FREEZABLE|WQ_MEM_RECLAIM, 0);
1874 if (!fileinfo_put_wq) {
1875 rc = -ENOMEM;
1876 goto out_destroy_decrypt_wq;
1877 }
1878
1879 cifsoplockd_wq = alloc_workqueue("cifsoplockd",
1880 WQ_FREEZABLE|WQ_MEM_RECLAIM, 0);
1881 if (!cifsoplockd_wq) {
1882 rc = -ENOMEM;
1883 goto out_destroy_fileinfo_put_wq;
1884 }
1885
1886 deferredclose_wq = alloc_workqueue("deferredclose",
1887 WQ_FREEZABLE|WQ_MEM_RECLAIM, 0);
1888 if (!deferredclose_wq) {
1889 rc = -ENOMEM;
1890 goto out_destroy_cifsoplockd_wq;
1891 }
1892
1893 serverclose_wq = alloc_workqueue("serverclose",
1894 WQ_FREEZABLE|WQ_MEM_RECLAIM, 0);
1895 if (!serverclose_wq) {
1896 rc = -ENOMEM;
1897 goto out_destroy_serverclose_wq;
This should be goto out_destroy_deferredclose_wq.
1898 }
1899
1900 rc = cifs_init_inodecache();
1901 if (rc)
1902 goto out_destroy_deferredclose_wq;
This should be out_destroy_serverclose_wq.
1903
1904 rc = cifs_init_netfs();
1905 if (rc)
1906 goto out_destroy_inodecache;
1907
1908 rc = init_mids();
1909 if (rc)
1910 goto out_destroy_netfs;
1911
1912 rc = cifs_init_request_bufs();
1913 if (rc)
1914 goto out_destroy_mids;
1915
1916 #ifdef CONFIG_CIFS_DFS_UPCALL
1917 rc = dfs_cache_init();
1918 if (rc)
1919 goto out_destroy_request_bufs;
1920 #endif /* CONFIG_CIFS_DFS_UPCALL */
1921 #ifdef CONFIG_CIFS_UPCALL
1922 rc = init_cifs_spnego();
1923 if (rc)
1924 goto out_destroy_dfs_cache;
1925 #endif /* CONFIG_CIFS_UPCALL */
1926 #ifdef CONFIG_CIFS_SWN_UPCALL
1927 rc = cifs_genl_init();
1928 if (rc)
1929 goto out_register_key_type;
This goto has a confusing name. It calls exit_cifs_spnego(). It used to
unregister the spegno key type. #HistoricalReasons
1930 #endif /* CONFIG_CIFS_SWN_UPCALL */
1931
1932 rc = init_cifs_idmap();
1933 if (rc)
1934 goto out_cifs_swn_init;
1935
1936 rc = register_filesystem(&cifs_fs_type);
1937 if (rc)
1938 goto out_init_cifs_idmap;
1939
1940 rc = register_filesystem(&smb3_fs_type);
1941 if (rc) {
1942 unregister_filesystem(&cifs_fs_type);
Do this in the unwind ladder, not before the goto.
1943 goto out_init_cifs_idmap;
1944 }
1945
1946 return 0;
1947
1948 out_init_cifs_idmap:
1949 exit_cifs_idmap();
1950 out_cifs_swn_init:
1951 #ifdef CONFIG_CIFS_SWN_UPCALL
1952 cifs_genl_exit();
1953 out_register_key_type:
1954 #endif
1955 #ifdef CONFIG_CIFS_UPCALL
1956 exit_cifs_spnego();
1957 out_destroy_dfs_cache:
1958 #endif
1959 #ifdef CONFIG_CIFS_DFS_UPCALL
1960 dfs_cache_destroy();
1961 out_destroy_request_bufs:
1962 #endif
These ifdef are a bit complicated. It might be nicer to write it as:
out_cifs_swn_init:
if (IS_ENABLED(CONFIG_CIFS_SWN_UPCALL))
cifs_genl_exit();
1963 cifs_destroy_request_bufs();
1964 out_destroy_mids:
1965 destroy_mids();
1966 out_destroy_netfs:
1967 cifs_destroy_netfs();
1968 out_destroy_inodecache:
1969 cifs_destroy_inodecache();
1970 out_destroy_deferredclose_wq:
1971 destroy_workqueue(deferredclose_wq);
1972 out_destroy_cifsoplockd_wq:
1973 destroy_workqueue(cifsoplockd_wq);
1974 out_destroy_fileinfo_put_wq:
1975 destroy_workqueue(fileinfo_put_wq);
1976 out_destroy_decrypt_wq:
1977 destroy_workqueue(decrypt_wq);
1978 out_destroy_cifsiod_wq:
1979 destroy_workqueue(cifsiod_wq);
1980 out_destroy_serverclose_wq:
1981 destroy_workqueue(serverclose_wq);
1982 out_clean_proc:
1983 cifs_proc_clean();
These need to be in mirror order from how they are allocated. The last step is
to cut and paste the cleanup into exit_cifs(), add an
unregister_filesystem(&smb3_fs_type); and delete the labels. There is an extra
step of cifs_release_automount_timer() which kills the timer. I would have
expected that to be the first thing in the function, but maybe the ordering
doesn't matter or maybe I haven't understood how the ordering works.
1984 return rc;
1985 }
regards,
dan carpenter
