linux-ext4 - Re: [PATCH v3 3/8] ext4: use __GFP_NOFAIL if allocating extents

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] [day] [month] [year] [list]

Date:   Mon, 24 Apr 2023 11:45:22 +0800
From:   Baokun Li <libaokun1@...wei.com>
To:     Jan Kara <jack@...e.cz>
CC:     <linux-ext4@...r.kernel.org>, <tytso@....edu>,
        <adilger.kernel@...ger.ca>, <ritesh.list@...il.com>,
        <linux-kernel@...r.kernel.org>, <yi.zhang@...wei.com>,
        <yangerkun@...wei.com>, <yukuai3@...wei.com>,
        Baokun Li <libaokun1@...wei.com>
Subject: Re: [PATCH v3 3/8] ext4: use __GFP_NOFAIL if allocating
 extents_status cannot fail

On 2023/4/13 18:30, Jan Kara wrote:
> On Wed 12-04-23 20:41:21, Baokun Li wrote:
>> If extent status tree update fails, we have inconsistency between what is
>> stored in the extent status tree and what is stored on disk. And that can
>> cause even data corruption issues in some cases.
>>
>> For extents that cannot be dropped we use __GFP_NOFAIL to allocate memory.
>> And with the above logic, the undo operation in __es_remove_extent that
>> may cause inconsistency if the split extent fails is unnecessary, so we
>> remove it as well.
>>
>> Suggested-by: Jan Kara<jack@...e.cz>
>> Signed-off-by: Baokun Li<libaokun1@...wei.com>
> When I was looking through this patch, I've realized there's a problem with
> my plan :-|. See below...
>
>>   static struct extent_status *
>>   ext4_es_alloc_extent(struct inode *inode, ext4_lblk_t lblk, ext4_lblk_t len,
>> -		     ext4_fsblk_t pblk)
>> +		     ext4_fsblk_t pblk, int nofail)
>>   {
>>   	struct extent_status *es;
>> -	es = kmem_cache_alloc(ext4_es_cachep, GFP_ATOMIC);
>> +	gfp_t gfp_flags = GFP_ATOMIC;
>> +
>> +	if (nofail)
>> +		gfp_flags |= __GFP_NOFAIL;
>> +
>> +	es = kmem_cache_alloc(ext4_es_cachep, gfp_flags);
>>   	if (es == NULL)
>>   		return NULL;
> I have remembered that the combination of GFP_ATOMIC and GFP_NOFAIL is
> discouraged because the kernel has no sane way of refilling reserves for
> atomic allocations when in atomic context. So this combination can result
> in lockups.

Indeed. GFP_NOFAIL is only applicable to sleepable allocations,

GFP_ATOMIC will ignore it. I didn't notice that.

> So what I think we'll have to do is that we'll just have to return error
> from __es_insert_extent() and __es_remove_extent() and in the callers we
> drop the i_es_lock, allocate needed status entries (one or two depending on
> the desired operation) with GFP_KERNEL | GFP_NOFAIL, get the lock again and
> pass the preallocated entries into __es_insert_extent /
> __es_remove_extent(). It's a bit ugly but we can at least remove those
> __es_shrink() calls which are not pretty either.
>
> 								Honza

Yes, there's really no better way, thank you very much for your review!
I've sent a patch for v4 as you suggested.
Thanks again!

-- 
With Best Regards,
Baokun Li
.