This should be the simpler way to fix this, which matches the unwind ladder. I moved the goto label to the
correct place so the frees are in the right order and switched the two gotos which were backwards.
Let me know if any objections or additional cleanup you think is important.
On Fri, Jul 19, 2024 at 5:05 PM Dan Carpenter <dan.carpenter@xxxxxxxxxx> wrote:
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
Thanks,
Steve
Steve
From 8e2a1da3227a59fdd2ec6fc2dd347561b144327a Mon Sep 17 00:00:00 2001
From: Steve French <stfrench@xxxxxxxxxxxxx>
Date: Sun, 21 Jul 2024 15:45:56 -0500
Subject: [PATCH] cifs: fix potential null pointer use in destroy_workqueue in
init_cifs error path
Dan Carpenter reported a Smack static checker warning:
fs/smb/client/cifsfs.c:1981 init_cifs()
error: we previously assumed 'serverclose_wq' could be null (see line 1895)
The patch which introduced the serverclose workqueue used the wrong
oredering in error paths in init_cifs() for freeing it on errors.
Reported-by: Dan Carpenter <dan.carpenter@xxxxxxxxxx>
Fixes: 173217bd7336 ("smb3: retrying on failed server close")
Cc: stable@xxxxxxxxxxxxxxx
Cc: Ritvik Budhiraja <rbudhiraja@xxxxxxxxxxxxx>
Signed-off-by: Steve French <stfrench@xxxxxxxxxxxxx>
---
fs/smb/client/cifsfs.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/fs/smb/client/cifsfs.c b/fs/smb/client/cifsfs.c
index c92937bed133..2c4b357d85e2 100644
--- a/fs/smb/client/cifsfs.c
+++ b/fs/smb/client/cifsfs.c
@@ -1894,12 +1894,12 @@ init_cifs(void)
WQ_FREEZABLE|WQ_MEM_RECLAIM, 0);
if (!serverclose_wq) {
rc = -ENOMEM;
- goto out_destroy_serverclose_wq;
+ goto out_destroy_deferredclose_wq;
}
rc = cifs_init_inodecache();
if (rc)
- goto out_destroy_deferredclose_wq;
+ goto out_destroy_serverclose_wq;
rc = cifs_init_netfs();
if (rc)
@@ -1967,6 +1967,8 @@ init_cifs(void)
cifs_destroy_netfs();
out_destroy_inodecache:
cifs_destroy_inodecache();
+out_destroy_serverclose_wq:
+ destroy_workqueue(serverclose_wq);
out_destroy_deferredclose_wq:
destroy_workqueue(deferredclose_wq);
out_destroy_cifsoplockd_wq:
@@ -1977,8 +1979,6 @@ init_cifs(void)
destroy_workqueue(decrypt_wq);
out_destroy_cifsiod_wq:
destroy_workqueue(cifsiod_wq);
-out_destroy_serverclose_wq:
- destroy_workqueue(serverclose_wq);
out_clean_proc:
cifs_proc_clean();
return rc;
--
2.43.0
