Commit 4f73c7d342d57d065bdbc0995cb56d8d1701b0c0
Committed by
Steve French
1 parent
e284e53fde
Exists in
ti-lsk-linux-4.1.y
and in
10 other branches
cifs: fix potential races in cifs_revalidate_mapping
The handling of the CIFS_INO_INVALID_MAPPING flag is racy. It's possible for two tasks to attempt to revalidate the mapping at the same time. The first sees that CIFS_INO_INVALID_MAPPING is set. It clears the flag and then calls invalidate_inode_pages2 to start shooting down the pagecache. While that's going on, another task checks the flag and sees that it's clear. It then ends up trusting the pagecache to satisfy a read when it shouldn't. Fix this by adding a bitlock to ensure that the clearing of the flag is atomic with respect to the actual cache invalidation. Also, move the other existing users of cifs_invalidate_mapping to use a new cifs_zap_mapping() function that just sets the INVALID_MAPPING bit and then uses the standard codepath to handle the invalidation. Signed-off-by: Jeff Layton <jlayton@poochiereds.net> Signed-off-by: Steve French <smfrench@gmail.com>
Showing 4 changed files with 49 additions and 15 deletions Side-by-side Diff
fs/cifs/cifsfs.h
... | ... | @@ -76,6 +76,7 @@ |
76 | 76 | extern int cifs_revalidate_dentry(struct dentry *); |
77 | 77 | extern int cifs_invalidate_mapping(struct inode *inode); |
78 | 78 | extern int cifs_revalidate_mapping(struct inode *inode); |
79 | +extern int cifs_zap_mapping(struct inode *inode); | |
79 | 80 | extern int cifs_getattr(struct vfsmount *, struct dentry *, struct kstat *); |
80 | 81 | extern int cifs_setattr(struct dentry *, struct iattr *); |
81 | 82 |
fs/cifs/cifsglob.h
... | ... | @@ -1118,6 +1118,7 @@ |
1118 | 1118 | #define CIFS_INODE_DOWNGRADE_OPLOCK_TO_L2 (2) /* Downgrade oplock to L2 */ |
1119 | 1119 | #define CIFS_INO_DELETE_PENDING (3) /* delete pending on server */ |
1120 | 1120 | #define CIFS_INO_INVALID_MAPPING (4) /* pagecache is invalid */ |
1121 | +#define CIFS_INO_LOCK (5) /* lock bit for synchronization */ | |
1121 | 1122 | unsigned long flags; |
1122 | 1123 | spinlock_t writers_lock; |
1123 | 1124 | unsigned int writers; /* Number of writers on this inode */ |
fs/cifs/file.c
... | ... | @@ -335,7 +335,7 @@ |
335 | 335 | spin_unlock(&cifs_file_list_lock); |
336 | 336 | |
337 | 337 | if (fid->purge_cache) |
338 | - cifs_invalidate_mapping(inode); | |
338 | + cifs_zap_mapping(inode); | |
339 | 339 | |
340 | 340 | file->private_data = cfile; |
341 | 341 | return cfile; |
... | ... | @@ -1529,7 +1529,7 @@ |
1529 | 1529 | */ |
1530 | 1530 | if (!CIFS_CACHE_WRITE(CIFS_I(inode)) && |
1531 | 1531 | CIFS_CACHE_READ(CIFS_I(inode))) { |
1532 | - cifs_invalidate_mapping(inode); | |
1532 | + cifs_zap_mapping(inode); | |
1533 | 1533 | cifs_dbg(FYI, "Set no oplock for inode=%p due to mand locks\n", |
1534 | 1534 | inode); |
1535 | 1535 | CIFS_I(inode)->oplock = 0; |
... | ... | @@ -2218,7 +2218,7 @@ |
2218 | 2218 | file->f_path.dentry->d_name.name, datasync); |
2219 | 2219 | |
2220 | 2220 | if (!CIFS_CACHE_READ(CIFS_I(inode))) { |
2221 | - rc = cifs_invalidate_mapping(inode); | |
2221 | + rc = cifs_zap_mapping(inode); | |
2222 | 2222 | if (rc) { |
2223 | 2223 | cifs_dbg(FYI, "rc: %d during invalidate phase\n", rc); |
2224 | 2224 | rc = 0; /* don't care about it in fsync */ |
... | ... | @@ -2649,7 +2649,7 @@ |
2649 | 2649 | * request comes - break it on the client to prevent reading |
2650 | 2650 | * an old data. |
2651 | 2651 | */ |
2652 | - cifs_invalidate_mapping(inode); | |
2652 | + cifs_zap_mapping(inode); | |
2653 | 2653 | cifs_dbg(FYI, "Set no oplock for inode=%p after a write operation\n", |
2654 | 2654 | inode); |
2655 | 2655 | cinode->oplock = 0; |
... | ... | @@ -3112,7 +3112,7 @@ |
3112 | 3112 | xid = get_xid(); |
3113 | 3113 | |
3114 | 3114 | if (!CIFS_CACHE_READ(CIFS_I(inode))) { |
3115 | - rc = cifs_invalidate_mapping(inode); | |
3115 | + rc = cifs_zap_mapping(inode); | |
3116 | 3116 | if (rc) |
3117 | 3117 | return rc; |
3118 | 3118 | } |
... | ... | @@ -3670,7 +3670,7 @@ |
3670 | 3670 | if (!CIFS_CACHE_READ(cinode)) { |
3671 | 3671 | rc = filemap_fdatawait(inode->i_mapping); |
3672 | 3672 | mapping_set_error(inode->i_mapping, rc); |
3673 | - cifs_invalidate_mapping(inode); | |
3673 | + cifs_zap_mapping(inode); | |
3674 | 3674 | } |
3675 | 3675 | cifs_dbg(FYI, "Oplock flush inode %p rc %d\n", inode, rc); |
3676 | 3676 | } |
fs/cifs/inode.c
... | ... | @@ -22,6 +22,7 @@ |
22 | 22 | #include <linux/stat.h> |
23 | 23 | #include <linux/slab.h> |
24 | 24 | #include <linux/pagemap.h> |
25 | +#include <linux/freezer.h> | |
25 | 26 | #include <asm/div64.h> |
26 | 27 | #include "cifsfs.h" |
27 | 28 | #include "cifspdu.h" |
28 | 29 | |
29 | 30 | |
30 | 31 | |
31 | 32 | |
32 | 33 | |
... | ... | @@ -1762,29 +1763,60 @@ |
1762 | 1763 | cifs_invalidate_mapping(struct inode *inode) |
1763 | 1764 | { |
1764 | 1765 | int rc = 0; |
1765 | - struct cifsInodeInfo *cifs_i = CIFS_I(inode); | |
1766 | 1766 | |
1767 | - clear_bit(CIFS_INO_INVALID_MAPPING, &cifs_i->flags); | |
1768 | - | |
1769 | 1767 | if (inode->i_mapping && inode->i_mapping->nrpages != 0) { |
1770 | 1768 | rc = invalidate_inode_pages2(inode->i_mapping); |
1771 | - if (rc) { | |
1769 | + if (rc) | |
1772 | 1770 | cifs_dbg(VFS, "%s: could not invalidate inode %p\n", |
1773 | 1771 | __func__, inode); |
1774 | - set_bit(CIFS_INO_INVALID_MAPPING, &cifs_i->flags); | |
1775 | - } | |
1776 | 1772 | } |
1777 | 1773 | |
1778 | 1774 | cifs_fscache_reset_inode_cookie(inode); |
1779 | 1775 | return rc; |
1780 | 1776 | } |
1781 | 1777 | |
1778 | +/** | |
1779 | + * cifs_wait_bit_killable - helper for functions that are sleeping on bit locks | |
1780 | + * @word: long word containing the bit lock | |
1781 | + */ | |
1782 | +static int | |
1783 | +cifs_wait_bit_killable(void *word) | |
1784 | +{ | |
1785 | + if (fatal_signal_pending(current)) | |
1786 | + return -ERESTARTSYS; | |
1787 | + freezable_schedule_unsafe(); | |
1788 | + return 0; | |
1789 | +} | |
1790 | + | |
1782 | 1791 | int |
1783 | 1792 | cifs_revalidate_mapping(struct inode *inode) |
1784 | 1793 | { |
1785 | - if (test_bit(CIFS_INO_INVALID_MAPPING, &CIFS_I(inode)->flags)) | |
1786 | - return cifs_invalidate_mapping(inode); | |
1787 | - return 0; | |
1794 | + int rc; | |
1795 | + unsigned long *flags = &CIFS_I(inode)->flags; | |
1796 | + | |
1797 | + rc = wait_on_bit_lock(flags, CIFS_INO_LOCK, cifs_wait_bit_killable, | |
1798 | + TASK_KILLABLE); | |
1799 | + if (rc) | |
1800 | + return rc; | |
1801 | + | |
1802 | + if (test_and_clear_bit(CIFS_INO_INVALID_MAPPING, flags)) { | |
1803 | + rc = cifs_invalidate_mapping(inode); | |
1804 | + if (rc) | |
1805 | + set_bit(CIFS_INO_INVALID_MAPPING, flags); | |
1806 | + } | |
1807 | + | |
1808 | + clear_bit_unlock(CIFS_INO_LOCK, flags); | |
1809 | + smp_mb__after_clear_bit(); | |
1810 | + wake_up_bit(flags, CIFS_INO_LOCK); | |
1811 | + | |
1812 | + return rc; | |
1813 | +} | |
1814 | + | |
1815 | +int | |
1816 | +cifs_zap_mapping(struct inode *inode) | |
1817 | +{ | |
1818 | + set_bit(CIFS_INO_INVALID_MAPPING, &CIFS_I(inode)->flags); | |
1819 | + return cifs_revalidate_mapping(inode); | |
1788 | 1820 | } |
1789 | 1821 | |
1790 | 1822 | int cifs_revalidate_file_attr(struct file *filp) |