Re: [PATCH 2/2] ext4: fix potential use after free issue while fsresize

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Theodore Ts'o <tytso@xxxxxxx> writes:

> On Fri, Nov 14, 2014 at 01:40:40PM +0400, Dmitry Monakhov wrote:
>> We need some sort of synchronization while updating ->s_group_desc because there
>> are a lot of users which can access old ->s_group_desc array after it was released.
>> It is reasonable to use lightweight seqcount_t here instead of RCU.
>> 
>> Signed-off-by: Dmitry Monakhov <dmonakhov@xxxxxxxxxx>
>
> I'm curious --- under what circumstances did you manage to hit this,
> or was this something that you noticed?
>
>> +	do {
>> +		seq = read_seqcount_begin(&sbi->s_group_desc_seq);
>> +		gd_bh = sbi->s_group_desc[group_desc];
>> +	} while (unlikely(read_seqcount_retry(&sbi->s_group_desc_seq, seq)));
>
> The problem is that s_group_desc is allocated using ext4_kvmalloc(),
> which means it's possible that it was allocated using vmalloc().
> Which means that it is possible (although unlikely) that the old
> s_group_desc address could become invalidated after the call to
> ext4_kvfree(o_group_desc).
>
> This will only happen on 32-bit platforms if we get unlucky and the
> kmap region gets recycled right after the vfree(); but we would only
> see the bug in practice if the memory gets kfree'ed gets reused
> immeidately afterwards, and we've been living with this with ext3 for
> a long time.  Don't get me wrong; I'm not saying we should ignore this
> bug, since I certainly agree with you that it is truly a bug.  But if
> we are going to fix it, we should probably fix it all the way, which I
> suspect means we may have to use RCU here.
>
> OTOH, if you are hitting this in live production workloads, then we
> can do the partial fix now, and then fix it all the way up later.  Is
> the RCU mechanism really going to be that ugly?  It's not like we are
> resizing all the time, so we don't need to worry about it being
> heavyweight from a performance point of view, no?
I use seqcount_t because Doc/RCU/array.txt state that it is lighter,
but indeed I've missed vmalloc case so I'll rewrite it to RCU.
>
> 	    	   	       	     	- Ted

Attachment: signature.asc
Description: PGP signature


[Index of Archives]     [Reiser Filesystem Development]     [Ceph FS]     [Kernel Newbies]     [Security]     [Netfilter]     [Bugtraq]     [Linux FS]     [Yosemite National Park]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Device Mapper]     [Linux Media]

  Powered by Linux