On Thu 15-08-19 16:16:31, zhangyi (F) wrote: > Remount process will release system zone which was allocated before if > "noblock_validity" is specified. If we mount an ext4 file system to two > mountpoints with default mount options, and then remount one of them > with "noblock_validity", it may trigger a use after free problem when > someone accessing the other one. > > # mount /dev/sda foo > # mount /dev/sda bar > > User access mountpoint "foo" | Remount mountpoint "bar" > | > ext4_map_blocks() | ext4_remount() > check_block_validity() | ext4_setup_system_zone() > ext4_data_block_valid() | ext4_release_system_zone() > | free system_blks rb nodes > access system_blks rb nodes | > trigger use after free | > > This problem can also be reproduced by one mountpint, At the same time, > add_system_zone() can get called during remount as well so there can be > racing ext4_data_block_valid() reading the rbtree at the same time. > > This patch add RCU to protect system zone from releasing or building > when doing a remount which inverse current "noblock_validity" mount > option. It assign the rbtree after the whole tree was complete and > do actual freeing after rcu grace period, avoid any intermediate state. > > Signed-off-by: zhangyi (F) <yi.zhang@xxxxxxxxxx> > --- > Changes since v3: > - add comments before ext4_setup_system_zone() and > ext4_release_system_zone() to explain why we need to serializes update > sbi->system_blks pointer. > - Fix block validity checking logic changes in v3. Thanks for the patch! The patch looks good. Just some language fixes in the new comments below. You can add: Reviewed-by: Jan Kara <jack@xxxxxxx> > +/* > + * Build system zone rbtree which is used for block validity checking. > + * > + * Note that system_blks pointer should be serializes updated at remount > + * time even under sb->s_umount semaphore protection, due to it can be > + * racing with ext4_data_block_valid() reading the system_blks rbtree at > + * the same time. I'd rephrase this paragraph a bit to be easier to understand: The update of system_blks pointer in this function is protected by sb->s_umount semaphore. However we have to be careful as we can be racing with ext4_data_block_valid() calls reading system_blks rbtree protected only by RCU. That's why we first build the rbtree and then swap it in place. > -/* Called when the filesystem is unmounted */ > +/* > + * Called when the filesystem is unmounted or when remounting it with > + * noblock_validity specified. > + * > + * Note that system_blks pointer should be serializes updated and do > + * the actual freeing after the RCU grace period at remount time even > + * under sb->s_umount semaphore protection, due to it can be racing with > + * ext4_data_block_valid() reading the system_blks rbtree at the same > + * time. > + */ Similarly here I'd phrase the last paragraph as: The update of system_blks pointer in this function is protected by sb->s_umount semaphore. However we have to be careful as we can be racing with ext4_data_block_valid() calls reading system_blks rbtree protected only by RCU. So we first clear the system_blks pointer and then free the rbtree only after RCU grace period expires. Honza -- Jan Kara <jack@xxxxxxxx> SUSE Labs, CR
