Commit 2f9092e1020246168b1309b35e085ecd7ff9ff72
Committed by
Al Viro
1 parent
1ba0c7dbbb
Exists in
master
and in
4 other branches
Fix i_mutex vs. readdir handling in nfsd
Commit 14f7dd63 ("Copy XFS readdir hack into nfsd code") introduced a bug to generic code which had been extant for a long time in the XFS version -- it started to call through into lookup_one_len() and hence into the file systems' ->lookup() methods without i_mutex held on the directory. This patch fixes it by locking the directory's i_mutex again before calling the filldir functions. The original deadlocks which commit 14f7dd63 was designed to avoid are still avoided, because they were due to fs-internal locking, not i_mutex. While we're at it, fix the return type of nfsd_buffered_readdir() which should be a __be32 not an int -- it's an NFS errno, not a Linux errno. And return nfserrno(-ENOMEM) when allocation fails, not just -ENOMEM. Sparse would have caught that, if it wasn't so busy bitching about __cold__. Commit 05f4f678 ("nfsd4: don't do lookup within readdir in recovery code") introduced a similar problem with calling lookup_one_len() without i_mutex, which this patch also addresses. To fix that, it was necessary to fix the called functions so that they expect i_mutex to be held; that part was done by J. Bruce Fields. Signed-off-by: David Woodhouse <David.Woodhouse@intel.com> Umm-I-can-live-with-that-by: Al Viro <viro@zeniv.linux.org.uk> Reported-by: J. R. Okajima <hooanon05@yahoo.co.jp> Tested-by: J. Bruce Fields <bfields@citi.umich.edu> LKML-Reference: <8036.1237474444@jrobl> Cc: stable@kernel.org Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
Showing 3 changed files with 30 additions and 43 deletions Side-by-side Diff
fs/namei.c
fs/nfsd/nfs4recover.c
... | ... | @@ -229,21 +229,23 @@ |
229 | 229 | goto out; |
230 | 230 | status = vfs_readdir(filp, nfsd4_build_namelist, &names); |
231 | 231 | fput(filp); |
232 | + mutex_lock(&dir->d_inode->i_mutex); | |
232 | 233 | while (!list_empty(&names)) { |
233 | 234 | entry = list_entry(names.next, struct name_list, list); |
234 | 235 | |
235 | 236 | dentry = lookup_one_len(entry->name, dir, HEXDIR_LEN-1); |
236 | 237 | if (IS_ERR(dentry)) { |
237 | 238 | status = PTR_ERR(dentry); |
238 | - goto out; | |
239 | + break; | |
239 | 240 | } |
240 | 241 | status = f(dir, dentry); |
241 | 242 | dput(dentry); |
242 | 243 | if (status) |
243 | - goto out; | |
244 | + break; | |
244 | 245 | list_del(&entry->list); |
245 | 246 | kfree(entry); |
246 | 247 | } |
248 | + mutex_unlock(&dir->d_inode->i_mutex); | |
247 | 249 | out: |
248 | 250 | while (!list_empty(&names)) { |
249 | 251 | entry = list_entry(names.next, struct name_list, list); |
... | ... | @@ -255,36 +257,6 @@ |
255 | 257 | } |
256 | 258 | |
257 | 259 | static int |
258 | -nfsd4_remove_clid_file(struct dentry *dir, struct dentry *dentry) | |
259 | -{ | |
260 | - int status; | |
261 | - | |
262 | - if (!S_ISREG(dir->d_inode->i_mode)) { | |
263 | - printk("nfsd4: non-file found in client recovery directory\n"); | |
264 | - return -EINVAL; | |
265 | - } | |
266 | - mutex_lock_nested(&dir->d_inode->i_mutex, I_MUTEX_PARENT); | |
267 | - status = vfs_unlink(dir->d_inode, dentry); | |
268 | - mutex_unlock(&dir->d_inode->i_mutex); | |
269 | - return status; | |
270 | -} | |
271 | - | |
272 | -static int | |
273 | -nfsd4_clear_clid_dir(struct dentry *dir, struct dentry *dentry) | |
274 | -{ | |
275 | - int status; | |
276 | - | |
277 | - /* For now this directory should already be empty, but we empty it of | |
278 | - * any regular files anyway, just in case the directory was created by | |
279 | - * a kernel from the future.... */ | |
280 | - nfsd4_list_rec_dir(dentry, nfsd4_remove_clid_file); | |
281 | - mutex_lock_nested(&dir->d_inode->i_mutex, I_MUTEX_PARENT); | |
282 | - status = vfs_rmdir(dir->d_inode, dentry); | |
283 | - mutex_unlock(&dir->d_inode->i_mutex); | |
284 | - return status; | |
285 | -} | |
286 | - | |
287 | -static int | |
288 | 260 | nfsd4_unlink_clid_dir(char *name, int namlen) |
289 | 261 | { |
290 | 262 | struct dentry *dentry; |
291 | 263 | |
292 | 264 | |
293 | 265 | |
... | ... | @@ -294,18 +266,18 @@ |
294 | 266 | |
295 | 267 | mutex_lock(&rec_dir.dentry->d_inode->i_mutex); |
296 | 268 | dentry = lookup_one_len(name, rec_dir.dentry, namlen); |
297 | - mutex_unlock(&rec_dir.dentry->d_inode->i_mutex); | |
298 | 269 | if (IS_ERR(dentry)) { |
299 | 270 | status = PTR_ERR(dentry); |
300 | - return status; | |
271 | + goto out_unlock; | |
301 | 272 | } |
302 | 273 | status = -ENOENT; |
303 | 274 | if (!dentry->d_inode) |
304 | 275 | goto out; |
305 | - | |
306 | - status = nfsd4_clear_clid_dir(rec_dir.dentry, dentry); | |
276 | + status = vfs_rmdir(rec_dir.dentry->d_inode, dentry); | |
307 | 277 | out: |
308 | 278 | dput(dentry); |
279 | +out_unlock: | |
280 | + mutex_unlock(&rec_dir.dentry->d_inode->i_mutex); | |
309 | 281 | return status; |
310 | 282 | } |
311 | 283 | |
... | ... | @@ -348,7 +320,7 @@ |
348 | 320 | if (nfs4_has_reclaimed_state(child->d_name.name, false)) |
349 | 321 | return 0; |
350 | 322 | |
351 | - status = nfsd4_clear_clid_dir(parent, child); | |
323 | + status = vfs_rmdir(parent->d_inode, child); | |
352 | 324 | if (status) |
353 | 325 | printk("failed to remove client recovery directory %s\n", |
354 | 326 | child->d_name.name); |
fs/nfsd/vfs.c
... | ... | @@ -1890,8 +1890,8 @@ |
1890 | 1890 | return 0; |
1891 | 1891 | } |
1892 | 1892 | |
1893 | -static int nfsd_buffered_readdir(struct file *file, filldir_t func, | |
1894 | - struct readdir_cd *cdp, loff_t *offsetp) | |
1893 | +static __be32 nfsd_buffered_readdir(struct file *file, filldir_t func, | |
1894 | + struct readdir_cd *cdp, loff_t *offsetp) | |
1895 | 1895 | { |
1896 | 1896 | struct readdir_data buf; |
1897 | 1897 | struct buffered_dirent *de; |
1898 | 1898 | |
... | ... | @@ -1901,11 +1901,12 @@ |
1901 | 1901 | |
1902 | 1902 | buf.dirent = (void *)__get_free_page(GFP_KERNEL); |
1903 | 1903 | if (!buf.dirent) |
1904 | - return -ENOMEM; | |
1904 | + return nfserrno(-ENOMEM); | |
1905 | 1905 | |
1906 | 1906 | offset = *offsetp; |
1907 | 1907 | |
1908 | 1908 | while (1) { |
1909 | + struct inode *dir_inode = file->f_path.dentry->d_inode; | |
1909 | 1910 | unsigned int reclen; |
1910 | 1911 | |
1911 | 1912 | cdp->err = nfserr_eof; /* will be cleared on successful read */ |
1912 | 1913 | |
1913 | 1914 | |
1914 | 1915 | |
1915 | 1916 | |
... | ... | @@ -1924,26 +1925,38 @@ |
1924 | 1925 | if (!size) |
1925 | 1926 | break; |
1926 | 1927 | |
1928 | + /* | |
1929 | + * Various filldir functions may end up calling back into | |
1930 | + * lookup_one_len() and the file system's ->lookup() method. | |
1931 | + * These expect i_mutex to be held, as it would within readdir. | |
1932 | + */ | |
1933 | + host_err = mutex_lock_killable(&dir_inode->i_mutex); | |
1934 | + if (host_err) | |
1935 | + break; | |
1936 | + | |
1927 | 1937 | de = (struct buffered_dirent *)buf.dirent; |
1928 | 1938 | while (size > 0) { |
1929 | 1939 | offset = de->offset; |
1930 | 1940 | |
1931 | 1941 | if (func(cdp, de->name, de->namlen, de->offset, |
1932 | 1942 | de->ino, de->d_type)) |
1933 | - goto done; | |
1943 | + break; | |
1934 | 1944 | |
1935 | 1945 | if (cdp->err != nfs_ok) |
1936 | - goto done; | |
1946 | + break; | |
1937 | 1947 | |
1938 | 1948 | reclen = ALIGN(sizeof(*de) + de->namlen, |
1939 | 1949 | sizeof(u64)); |
1940 | 1950 | size -= reclen; |
1941 | 1951 | de = (struct buffered_dirent *)((char *)de + reclen); |
1942 | 1952 | } |
1953 | + mutex_unlock(&dir_inode->i_mutex); | |
1954 | + if (size > 0) /* We bailed out early */ | |
1955 | + break; | |
1956 | + | |
1943 | 1957 | offset = vfs_llseek(file, 0, SEEK_CUR); |
1944 | 1958 | } |
1945 | 1959 | |
1946 | - done: | |
1947 | 1960 | free_page((unsigned long)(buf.dirent)); |
1948 | 1961 | |
1949 | 1962 | if (host_err) |