| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Wed, 27 Oct 2010 09:07:34 +0200
From: Christian Stroetmann <stroetmann@...olinux.com>
To: Dave Chinner <david@...morbit.com>
CC: linux-fsdevel <linux-fsdevel@...r.kernel.org>,
linux-kernel <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 1/4] fs: protect inode->i_state with inode->i_lock
Aloha;
some typos
On the 27.10.2010 06:23, Dave Chinner wrote:
> From: Dave Chinner<dchinner@...hat.com>
>
> Protect inode state transitions and validity checks with the
> inode->i_lock. This enables us to make inode state transitions
> independently of the inode_lock and is the first step to peeling
> away the inode_lock from the code.
>
> Signed-off-by: Dave Chinner<dchinner@...hat.com>
> ---
> fs/drop_caches.c | 6 +++-
> fs/fs-writeback.c | 31 ++++++++++++++--
> fs/inode.c | 95 +++++++++++++++++++++++++++++++++++++++---------
> fs/notify/inode_mark.c | 17 ++++++---
> fs/quota/dquot.c | 6 +++-
> 5 files changed, 127 insertions(+), 28 deletions(-)
>
> diff --git a/fs/drop_caches.c b/fs/drop_caches.c
> index 2195c21..c495fc3 100644
> --- a/fs/drop_caches.c
> +++ b/fs/drop_caches.c
> @@ -18,8 +18,12 @@ static void drop_pagecache_sb(struct super_block *sb, void *unused)
>
> spin_lock(&inode_lock);
> list_for_each_entry(inode,&sb->s_inodes, i_sb_list) {
> - if (inode->i_state& (I_FREEING|I_WILL_FREE|I_NEW))
> + spin_lock(&inode->i_lock);
> + if (inode->i_state& (I_FREEING|I_WILL_FREE|I_NEW)) {
> + spin_unlock(&inode->i_lock);
> continue;
> + }
> + spin_unlock(&inode->i_lock);
> if (inode->i_mapping->nrpages == 0)
> continue;
> __iget(inode);
> diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c
> index f6af81a..bd9204d 100644
> --- a/fs/fs-writeback.c
> +++ b/fs/fs-writeback.c
> @@ -293,9 +293,11 @@ static void inode_wait_for_writeback(struct inode *inode)
>
> wqh = bit_waitqueue(&inode->i_state, __I_SYNC);
> while (inode->i_state& I_SYNC) {
> + spin_unlock(&inode->i_lock);
> spin_unlock(&inode_lock);
> __wait_on_bit(wqh,&wq, inode_wait, TASK_UNINTERRUPTIBLE);
> spin_lock(&inode_lock);
> + spin_lock(&inode->i_lock);
> }
> }
>
> @@ -319,6 +321,7 @@ writeback_single_inode(struct inode *inode, struct writeback_control *wbc)
> unsigned dirty;
> int ret;
>
> + spin_lock(&inode->i_lock);
> if (!atomic_read(&inode->i_count))
> WARN_ON(!(inode->i_state& (I_WILL_FREE|I_FREEING)));
> else
> @@ -334,6 +337,7 @@ writeback_single_inode(struct inode *inode, struct writeback_control *wbc)
> * completed a full scan of b_io.
> */
> if (wbc->sync_mode != WB_SYNC_ALL) {
> + spin_unlock(&inode->i_lock);
> requeue_io(inode);
> return 0;
> }
> @@ -349,6 +353,7 @@ writeback_single_inode(struct inode *inode, struct writeback_control *wbc)
> /* Set I_SYNC, reset I_DIRTY_PAGES */
> inode->i_state |= I_SYNC;
> inode->i_state&= ~I_DIRTY_PAGES;
> + spin_unlock(&inode->i_lock);
> spin_unlock(&inode_lock);
>
> ret = do_writepages(mapping, wbc);
> @@ -370,8 +375,10 @@ writeback_single_inode(struct inode *inode, struct writeback_control *wbc)
> * write_inode()
> */
> spin_lock(&inode_lock);
> + spin_lock(&inode->i_lock);
> dirty = inode->i_state& I_DIRTY;
> inode->i_state&= ~(I_DIRTY_SYNC | I_DIRTY_DATASYNC);
> + spin_unlock(&inode->i_lock);
> spin_unlock(&inode_lock);
> /* Don't write the inode if only I_DIRTY_PAGES was set */
end point?!
> if (dirty& (I_DIRTY_SYNC | I_DIRTY_DATASYNC)) {
> @@ -381,6 +388,7 @@ writeback_single_inode(struct inode *inode, struct writeback_control *wbc)
> }
>
> spin_lock(&inode_lock);
> + spin_lock(&inode->i_lock);
> inode->i_state&= ~I_SYNC;
> if (!(inode->i_state& I_FREEING)) {
> if (mapping_tagged(mapping, PAGECACHE_TAG_DIRTY)) {
> @@ -422,6 +430,7 @@ writeback_single_inode(struct inode *inode, struct writeback_control *wbc)
> }
> }
> inode_sync_complete(inode);
> + spin_unlock(&inode->i_lock);
> return ret;
> }
>
> @@ -492,10 +501,13 @@ static int writeback_sb_inodes(struct super_block *sb, struct bdi_writeback *wb,
> * kind does not need peridic writeout yet, and for the latter
periodic
> * kind writeout is handled by the freer.
> */
> + spin_lock(&inode->i_lock);
> if (inode->i_state& (I_NEW | I_FREEING | I_WILL_FREE)) {
> + spin_unlock(&inode->i_lock);
> requeue_io(inode);
> continue;
> }
> + spin_unlock(&inode->i_lock);
>
> /*
> * Was this inode dirtied after sync_sb_inodes was called?
> @@ -681,7 +693,9 @@ static long wb_writeback(struct bdi_writeback *wb,
> if (!list_empty(&wb->b_more_io)) {
> inode = wb_inode(wb->b_more_io.prev);
> trace_wbc_writeback_wait(&wbc, wb->bdi);
> + spin_lock(&inode->i_lock);
> inode_wait_for_writeback(inode);
> + spin_unlock(&inode->i_lock);
> }
> spin_unlock(&inode_lock);
> }
> @@ -947,6 +961,7 @@ void __mark_inode_dirty(struct inode *inode, int flags)
> block_dump___mark_inode_dirty(inode);
>
> spin_lock(&inode_lock);
> + spin_lock(&inode->i_lock);
> if ((inode->i_state& flags) != flags) {
> const int was_dirty = inode->i_state& I_DIRTY;
>
> @@ -958,7 +973,7 @@ void __mark_inode_dirty(struct inode *inode, int flags)
> * superblock list, based upon its state.
> */
> if (inode->i_state& I_SYNC)
> - goto out;
> + goto out_unlock_inode;
>
> /*
> * Only add valid (hashed) inodes to the superblock's
> @@ -966,11 +981,12 @@ void __mark_inode_dirty(struct inode *inode, int flags)
> */
> if (!S_ISBLK(inode->i_mode)) {
> if (inode_unhashed(inode))
> - goto out;
> + goto out_unlock_inode;
> }
> if (inode->i_state& I_FREEING)
> - goto out;
> + goto out_unlock_inode;
>
> + spin_unlock(&inode->i_lock);
> /*
> * If the inode was already on b_dirty/b_io/b_more_io, don't
> * reposition it (that would break b_dirty time-ordering).
> @@ -995,7 +1011,10 @@ void __mark_inode_dirty(struct inode *inode, int flags)
> inode->dirtied_when = jiffies;
> list_move(&inode->i_wb_list,&bdi->wb.b_dirty);
> }
> + goto out;
> }
> +out_unlock_inode:
> + spin_unlock(&inode->i_lock);
> out:
> spin_unlock(&inode_lock);
>
> @@ -1043,8 +1062,12 @@ static void wait_sb_inodes(struct super_block *sb)
> list_for_each_entry(inode,&sb->s_inodes, i_sb_list) {
> struct address_space *mapping;
>
> - if (inode->i_state& (I_FREEING|I_WILL_FREE|I_NEW))
> + spin_lock(&inode->i_lock);
> + if (inode->i_state& (I_FREEING|I_WILL_FREE|I_NEW)) {
> + spin_unlock(&inode->i_lock);
> continue;
> + }
> + spin_unlock(&inode->i_lock);
> mapping = inode->i_mapping;
> if (mapping->nrpages == 0)
> continue;
> diff --git a/fs/inode.c b/fs/inode.c
> index a6d6068..eaba6ce 100644
> --- a/fs/inode.c
> +++ b/fs/inode.c
> @@ -26,6 +26,17 @@
> #include<linux/posix_acl.h>
>
> /*
> + * inode locking rules.
> + *
> + * inode->i_lock protects:
> + * i_state
> + *
> + * Lock ordering:
> + * inode_lock
> + * inode->i_lock
> + */
> +
> +/*
> * This is needed for the following functions:
> * - inode_has_buffers
> * - invalidate_bdev
> @@ -429,7 +440,9 @@ void end_writeback(struct inode *inode)
> BUG_ON(!(inode->i_state& I_FREEING));
> BUG_ON(inode->i_state& I_CLEAR);
> inode_sync_wait(inode);
> + spin_lock(&inode->i_lock);
> inode->i_state = I_FREEING | I_CLEAR;
> + spin_unlock(&inode->i_lock);
> }
> EXPORT_SYMBOL(end_writeback);
>
> @@ -498,12 +511,17 @@ void evict_inodes(struct super_block *sb)
> if (atomic_read(&inode->i_count))
> continue;
>
> + spin_lock(&inode->i_lock);
> if (inode->i_state& (I_NEW | I_FREEING | I_WILL_FREE)) {
> + spin_unlock(&inode->i_lock);
> WARN_ON(1);
> continue;
> }
>
> inode->i_state |= I_FREEING;
> + if (!(inode->i_state& (I_DIRTY | I_SYNC)))
> + percpu_counter_dec(&nr_inodes_unused);
> + spin_unlock(&inode->i_lock);
>
> /*
> * Move the inode off the IO lists and LRU once I_FREEING is
> @@ -511,8 +529,6 @@ void evict_inodes(struct super_block *sb)
> */
> list_move(&inode->i_lru,&dispose);
> list_del_init(&inode->i_wb_list);
> - if (!(inode->i_state& (I_DIRTY | I_SYNC)))
> - percpu_counter_dec(&nr_inodes_unused);
> }
> spin_unlock(&inode_lock);
>
> @@ -521,7 +537,7 @@ void evict_inodes(struct super_block *sb)
> }
>
> /**
> - * invalidate_inodes - attempt to free all inodes on a superblock
> + * nvalidate_inodes - attempt to free all inodes on a superblock
attempts
invalidate_inodes
> * @sb: superblock to operate on
> *
> * Attempts to free all inodes for a given superblock. If there were any
> @@ -537,14 +553,21 @@ int invalidate_inodes(struct super_block *sb)
>
> spin_lock(&inode_lock);
> list_for_each_entry_safe(inode, next,&sb->s_inodes, i_sb_list) {
> - if (inode->i_state& (I_NEW | I_FREEING | I_WILL_FREE))
> + spin_lock(&inode->i_lock);
> + if (inode->i_state& (I_NEW | I_FREEING | I_WILL_FREE)) {
> + spin_unlock(&inode->i_lock);
> continue;
> + }
> if (atomic_read(&inode->i_count)) {
> + spin_unlock(&inode->i_lock);
> busy = 1;
> continue;
> }
>
> inode->i_state |= I_FREEING;
> + if (!(inode->i_state& (I_DIRTY | I_SYNC)))
> + percpu_counter_dec(&nr_inodes_unused);
> + spin_unlock(&inode->i_lock);
>
> /*
> * Move the inode off the IO lists and LRU once I_FREEING is
> @@ -552,8 +575,6 @@ int invalidate_inodes(struct super_block *sb)
> */
> list_move(&inode->i_lru,&dispose);
> list_del_init(&inode->i_wb_list);
> - if (!(inode->i_state& (I_DIRTY | I_SYNC)))
> - percpu_counter_dec(&nr_inodes_unused);
> }
> spin_unlock(&inode_lock);
>
> @@ -612,8 +633,10 @@ static void prune_icache(int nr_to_scan)
> * Referenced or dirty inodes are still in use. Give them
> * another pass through the LRU as we canot reclaim them now.
cannot
> */
> + spin_lock(&inode->i_lock);
> if (atomic_read(&inode->i_count) ||
> (inode->i_state& ~I_REFERENCED)) {
> + spin_unlock(&inode->i_lock);
> list_del_init(&inode->i_lru);
> percpu_counter_dec(&nr_inodes_unused);
> continue;
> @@ -621,12 +644,14 @@ static void prune_icache(int nr_to_scan)
>
> /* recently referenced inodes get one more pass */
> if (inode->i_state& I_REFERENCED) {
> - list_move(&inode->i_lru,&inode_lru);
> inode->i_state&= ~I_REFERENCED;
> + spin_unlock(&inode->i_lock);
> + list_move(&inode->i_lru,&inode_lru);
> continue;
> }
> if (inode_has_buffers(inode) || inode->i_data.nrpages) {
> __iget(inode);
> + spin_unlock(&inode->i_lock);
> spin_unlock(&inode_lock);
> if (remove_inode_buffers(inode))
> reap += invalidate_mapping_pages(&inode->i_data,
> @@ -637,11 +662,15 @@ static void prune_icache(int nr_to_scan)
> if (inode != list_entry(inode_lru.next,
> struct inode, i_lru))
> continue; /* wrong inode or list_empty */
> - if (!can_unuse(inode))
> + spin_lock(&inode->i_lock);
> + if (!can_unuse(inode)) {
> + spin_unlock(&inode->i_lock);
> continue;
> + }
> }
> WARN_ON(inode->i_state& I_NEW);
> inode->i_state |= I_FREEING;
> + spin_unlock(&inode->i_lock);
>
> /*
> * Move the inode off the IO lists and LRU once I_FREEING is
> @@ -708,10 +737,12 @@ repeat:
> continue;
> if (!test(inode, data))
> continue;
> + spin_lock(&inode->i_lock);
> if (inode->i_state& (I_FREEING|I_WILL_FREE)) {
> __wait_on_freeing_inode(inode);
> goto repeat;
> }
> + spin_unlock(&inode->i_lock);
> __iget(inode);
> return inode;
> }
> @@ -734,10 +765,12 @@ repeat:
> continue;
> if (inode->i_sb != sb)
> continue;
> + spin_lock(&inode->i_lock);
> if (inode->i_state& (I_FREEING|I_WILL_FREE)) {
> __wait_on_freeing_inode(inode);
> goto repeat;
> }
> + spin_unlock(&inode->i_lock);
> __iget(inode);
> return inode;
> }
> @@ -803,8 +836,10 @@ struct inode *new_inode(struct super_block *sb)
> inode = alloc_inode(sb);
> if (inode) {
> spin_lock(&inode_lock);
> - __inode_sb_list_add(inode);
> + spin_lock(&inode->i_lock);
> inode->i_state = 0;
> + spin_unlock(&inode->i_lock);
> + __inode_sb_list_add(inode);
> spin_unlock(&inode_lock);
> }
> return inode;
> @@ -871,9 +906,11 @@ static struct inode *get_new_inode(struct super_block *sb,
> if (set(inode, data))
> goto set_failed;
>
> + spin_lock(&inode->i_lock);
> + inode->i_state = I_NEW;
> + spin_unlock(&inode->i_lock);
> hlist_add_head(&inode->i_hash, head);
> __inode_sb_list_add(inode);
> - inode->i_state = I_NEW;
> spin_unlock(&inode_lock);
>
> /* Return the locked inode with I_NEW set, the
> @@ -917,10 +954,12 @@ static struct inode *get_new_inode_fast(struct super_block *sb,
> /* We released the lock, so.. */
so what? :-)
> old = find_inode_fast(sb, head, ino);
> if (!old) {
> + spin_lock(&inode->i_lock);
> inode->i_ino = ino;
> + inode->i_state = I_NEW;
> + spin_unlock(&inode->i_lock);
> hlist_add_head(&inode->i_hash, head);
> __inode_sb_list_add(inode);
> - inode->i_state = I_NEW;
> spin_unlock(&inode_lock);
>
> /* Return the locked inode with I_NEW set, the
> @@ -1005,15 +1044,19 @@ EXPORT_SYMBOL(iunique);
> struct inode *igrab(struct inode *inode)
> {
> spin_lock(&inode_lock);
> - if (!(inode->i_state& (I_FREEING|I_WILL_FREE)))
> + spin_lock(&inode->i_lock);
> + if (!(inode->i_state& (I_FREEING|I_WILL_FREE))) {
> __iget(inode);
> - else
> + spin_unlock(&inode->i_lock);
> + } else {
> + spin_unlock(&inode->i_lock);
> /*
> * Handle the case where s_op->clear_inode is not been
> * called yet, and somebody is calling igrab
> * while the inode is getting freed.
> */
> inode = NULL;
> + }
> spin_unlock(&inode_lock);
> return inode;
> }
> @@ -1242,7 +1285,9 @@ int insert_inode_locked(struct inode *inode)
> ino_t ino = inode->i_ino;
> struct hlist_head *head = inode_hashtable + hash(sb, ino);
>
> + spin_lock(&inode->i_lock);
> inode->i_state |= I_NEW;
> + spin_unlock(&inode->i_lock);
> while (1) {
> struct hlist_node *node;
> struct inode *old = NULL;
> @@ -1252,8 +1297,11 @@ int insert_inode_locked(struct inode *inode)
> continue;
> if (old->i_sb != sb)
> continue;
> - if (old->i_state& (I_FREEING|I_WILL_FREE))
> + spin_lock(&old->i_lock);
> + if (old->i_state& (I_FREEING|I_WILL_FREE)) {
> + spin_unlock(&old->i_lock);
> continue;
> + }
> break;
> }
> if (likely(!node)) {
> @@ -1261,6 +1309,7 @@ int insert_inode_locked(struct inode *inode)
> spin_unlock(&inode_lock);
> return 0;
> }
> + spin_unlock(&old->i_lock);
> __iget(old);
> spin_unlock(&inode_lock);
> wait_on_inode(old);
> @@ -1279,7 +1328,9 @@ int insert_inode_locked4(struct inode *inode, unsigned long hashval,
> struct super_block *sb = inode->i_sb;
> struct hlist_head *head = inode_hashtable + hash(sb, hashval);
>
> + spin_lock(&inode->i_lock);
> inode->i_state |= I_NEW;
> + spin_unlock(&inode->i_lock);
>
> while (1) {
> struct hlist_node *node;
> @@ -1291,8 +1342,11 @@ int insert_inode_locked4(struct inode *inode, unsigned long hashval,
> continue;
> if (!test(old, data))
> continue;
> - if (old->i_state& (I_FREEING|I_WILL_FREE))
> + spin_lock(&old->i_lock);
> + if (old->i_state& (I_FREEING|I_WILL_FREE)) {
> + spin_unlock(&old->i_lock);
> continue;
> + }
> break;
> }
> if (likely(!node)) {
> @@ -1300,6 +1354,7 @@ int insert_inode_locked4(struct inode *inode, unsigned long hashval,
> spin_unlock(&inode_lock);
> return 0;
> }
> + spin_unlock(&old->i_lock);
> __iget(old);
> spin_unlock(&inode_lock);
> wait_on_inode(old);
> @@ -1346,6 +1401,9 @@ static void iput_final(struct inode *inode)
> const struct super_operations *op = inode->i_sb->s_op;
> int drop;
>
> + spin_lock(&inode->i_lock);
> + WARN_ON(inode->i_state& I_NEW);
> +
> if (op&& op->drop_inode)
> drop = op->drop_inode(inode);
> else
> @@ -1357,21 +1415,23 @@ static void iput_final(struct inode *inode)
> if (!(inode->i_state& (I_DIRTY|I_SYNC))) {
> inode_lru_list_add(inode);
> }
> + spin_unlock(&inode->i_lock);
> spin_unlock(&inode_lock);
> return;
> }
> - WARN_ON(inode->i_state& I_NEW);
> inode->i_state |= I_WILL_FREE;
> + spin_unlock(&inode->i_lock);
> spin_unlock(&inode_lock);
> write_inode_now(inode, 1);
> spin_lock(&inode_lock);
> + spin_lock(&inode->i_lock);
> WARN_ON(inode->i_state& I_NEW);
> inode->i_state&= ~I_WILL_FREE;
> __remove_inode_hash(inode);
> }
>
> - WARN_ON(inode->i_state& I_NEW);
> inode->i_state |= I_FREEING;
> + spin_unlock(&inode->i_lock);
>
> /*
> * Move the inode off the IO lists and LRU once I_FREEING is
> @@ -1592,6 +1652,7 @@ static void __wait_on_freeing_inode(struct inode *inode)
> DEFINE_WAIT_BIT(wait,&inode->i_state, __I_NEW);
> wq = bit_waitqueue(&inode->i_state, __I_NEW);
> prepare_to_wait(wq,&wait.wait, TASK_UNINTERRUPTIBLE);
> + spin_unlock(&inode->i_lock);
> spin_unlock(&inode_lock);
> schedule();
> finish_wait(wq,&wait.wait);
> diff --git a/fs/notify/inode_mark.c b/fs/notify/inode_mark.c
> index 21ed106..08f0d16 100644
> --- a/fs/notify/inode_mark.c
> +++ b/fs/notify/inode_mark.c
> @@ -249,8 +249,12 @@ void fsnotify_unmount_inodes(struct list_head *list)
> * I_WILL_FREE, or I_NEW which is fine because by that point
> * the inode cannot have any associated watches.
> */
> - if (inode->i_state& (I_FREEING|I_WILL_FREE|I_NEW))
> + spin_lock(&inode->i_lock);
> + if (inode->i_state& (I_FREEING|I_WILL_FREE|I_NEW)) {
> + spin_unlock(&inode->i_lock);
> continue;
> + }
> + spin_unlock(&inode->i_lock);
>
> /*
> * If i_count is zero, the inode cannot have any watches and
, then the inode
> @@ -272,10 +276,13 @@ void fsnotify_unmount_inodes(struct list_head *list)
>
> /* In case the dropping of a reference would nuke next_i. */
> if ((&next_i->i_sb_list != list)&&
> - atomic_read(&next_i->i_count)&&
> - !(next_i->i_state& (I_FREEING | I_WILL_FREE))) {
> - __iget(next_i);
> - need_iput = next_i;
> + atomic_read(&next_i->i_count)) {
> + spin_lock(&next_i->i_lock);
> + if (!(next_i->i_state& (I_FREEING | I_WILL_FREE))) {
> + __iget(next_i);
> + need_iput = next_i;
> + }
> + spin_unlock(&next_i->i_lock);
> }
>
> /*
> diff --git a/fs/quota/dquot.c b/fs/quota/dquot.c
> index aad1316..fb7c2c0 100644
> --- a/fs/quota/dquot.c
> +++ b/fs/quota/dquot.c
> @@ -898,8 +898,12 @@ static void add_dquot_ref(struct super_block *sb, int type)
>
> spin_lock(&inode_lock);
> list_for_each_entry(inode,&sb->s_inodes, i_sb_list) {
> - if (inode->i_state& (I_FREEING|I_WILL_FREE|I_NEW))
> + spin_lock(&inode->i_lock);
> + if (inode->i_state& (I_FREEING|I_WILL_FREE|I_NEW)) {
> + spin_unlock(&inode->i_lock);
> continue;
> + }
> + spin_unlock(&inode->i_lock);
> #ifdef CONFIG_QUOTA_DEBUG
> if (unlikely(inode_get_rsv_space(inode)> 0))
> reserved = 1;
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists