linux-ext4 - Re: [PATCH 2/4] ext4: fix reserved space transferring on chown() [V2]

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Mon, 7 Dec 2009 22:48:35 +0530
From:	"Aneesh Kumar K.V" <aneesh.kumar@...ux.vnet.ibm.com>
To:	Dmitry Monakhov <dmonakhov@...nvz.org>
Cc:	linux-ext4@...r.kernel.org
Subject: Re: [PATCH 2/4] ext4: fix reserved space transferring on chown()
 [V2]

On Wed, Nov 25, 2009 at 09:57:39AM +0300, Dmitry Monakhov wrote:
> Currently all quota's functions except vfs_dq_reserve_block()
> called without i_block_reservation_lock. This result in
> ext4_reservation vs quota_reservation inconsistency which provoke
> incorrect reservation transfer ==> incorrect quota after transfer.
> 
> Race (1)
> | Task 1 (chown)                 | Task 2 (truncate)                    |
> | dquot_transfer                 |                                      |
> | ->down_write(dqptr_sem)        | ext4_da_release_spac                 |
> | -->dquot_get_reserved_space    | ->lock(i_block_reservation_lock)     |
> | --->get_reserved_space         | /* decrement reservation */          |
> | ---->ext4_get_reserved_space   | ->unlock(i_block_reservation_lock)   |
> | ----->lock(i_block_rsv_lock)   | /* During this time window           |
> | /* Read ext4_rsv from inode */ | * fs's reservation not equals        |
> | /* transfer it to new quota */ | * to quota's */                      |
> | ->up_write(dqptr_sem)          | ->vfs_dq_release_reservation_block() |
> |                                | /* quota_rsv goes negative here */   |
> |                                |                                      |
> 
> Race (2)
> | Task 1 (chown)                | Task 2 (flush-8:16)             |
> | dquot_transfer()              | ext4_mb_mark_diskspace_used()   |
> | ->down_write(dqptr_sem)       | ->vfs_dq_claim_block()          |
> | --->get_reserved_space()      | /* After this moment         */ |
> | --->ext4_get_reserved_space() | /* ext4_rsv != quota_ino_rsv */ |
> | /* Read rsv from inode which  |                                 |
> | ->dquot_free_reserved_space() |                                 |
> | /* quota_rsv goes negative */ |                                 |
> |                               |                                 |
> |                               | dquot_free_reserved_space()     |
> |                               | /* finally dec ext4_ino_rsv */  |
> 
> So, in order to protect us from this type of races we always have to
> provides ext4_ino_rsv == quot_ino_rsv guarantee. And this is only
> possible then i_block_reservation_lock is taken before entering any
> quota operations.
> 
> In fact i_block_reservation_lock is held by ext4_da_reserve_space()
> while calling vfs_dq_reserve_block(). Lock are held in following order
> i_block_reservation_lock > dqptr_sem
> 
> This may result in deadlock because of different lock ordering:
> ext4_da_reserve_space()             dquot_transfer()
> lock(i_block_reservation_lock)	     down_write(dqptr_sem)
>   down_write(dqptr_sem)		     lock(i_block_reservation_lock)
> 
> But this not happen only because both callers must have i_mutex so
> serialization happens on i_mutex.


But that down_write can sleep right ? 

For example:
http://bugzilla.kernel.org/show_bug.cgi?id=14739

-aneesh
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html