linux-kernel - Re: [PATCH 1/1] f2fs: fix fallocate failed under pinned block situation

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:   Sat, 11 Nov 2023 12:49:08 +0800
From:   Chao Yu <chao@...nel.org>
To:     Wu Bo <wubo.oduw@...il.com>, Wu Bo <bo.wu@...o.com>,
        Jaegeuk Kim <jaegeuk@...nel.org>
Cc:     linux-f2fs-devel@...ts.sourceforge.net,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/1] f2fs: fix fallocate failed under pinned block
 situation

On 2023/11/8 21:48, Wu Bo wrote:
> On 2023/11/7 22:39, Chao Yu wrote:
>> On 2023/10/30 17:40, Wu Bo wrote:
>>> If GC victim has pinned block, it can't be recycled.
>>> And if GC is foreground running, after many failure try, the pinned file
>>> is expected to be clear pin flag. To enable the section be recycled.
>>>
>>> But when fallocate trigger FG_GC, GC can never recycle the pinned
>>> section. Because GC will go to stop before the failure try meet the
>>> threshold:
>>>      if (has_enough_free_secs(sbi, sec_freed, 0)) {
>>>          if (!gc_control->no_bg_gc &&
>>>              total_sec_freed < gc_control->nr_free_secs)
>>>              goto go_gc_more;
>>>          goto stop;
>>>      }
>>>
>>> So when fallocate trigger FG_GC, at least recycle one.
>>
>> Hmm... it may break pinfile's semantics at least on one pinned file?
>> In this case, I prefer to fail fallocate() rather than unpinning file,
>> in order to avoid leaving invalid LBA references of unpinned file held
>> by userspace.
> 
> As f2fs designed now, FG_GC is able to unpin the pinned file.
> 
> fallocate() triggered FG_GC, but can't recycle space.  It breaks the
> design logic of FG_GC.

Yes, contradictoriness exists.

IMO, unpin file by GC looks more dangerous, it may cause potential data
corruption w/ below case:
1. app pins file & holds LBAs of data blocks.
2. GC unpins file and migrates its data to new LBAs.
3. other file reuses previous LBAs.
4. app read/write data via previous LBAs.

So I suggest to normalize use of pinfile and do not add more unpin cases
in filesystem inner processes.

> 
> This issue is happened in Android OTA scenario.  fallocate() always
> return failure cause OTA fail.

Can you please check why other pinned files were so fragmented that f2fs_gc()
can not recycle one free section?

Thanks,

> 
>    And this commit changed previous behavior of fallocate():
> 
> Commit 2e42b7f817ac ("f2fs: stop allocating pinned sections if EAGAIN
> happens")
> 
> Before this commit, if fallocate() meet this situation, it will trigger
> FG_GC to recycle pinned space finally.
> 
> FG_GC is expected to recycle pinned space when there is no more free
> space.  And this is the right time to do it when fallocate() need free
> space.
> 
> It is weird when f2fs shows enough spare space but can't fallocate(). So
> I think it should be fixed.
> 
>>
>> Thoughts?
>>
>> Thanks,
>>
>>>
>>> This issue can be reproduced by filling f2fs space as following layout.
>>> Every segment has one block is pinned:
>>> +-+-+-+-+-+-+-----+-+
>>> | | |p| | | | ... | | seg_n
>>> +-+-+-+-+-+-+-----+-+
>>> +-+-+-+-+-+-+-----+-+
>>> | | |p| | | | ... | | seg_n+1
>>> +-+-+-+-+-+-+-----+-+
>>> ...
>>> +-+-+-+-+-+-+-----+-+
>>> | | |p| | | | ... | | seg_n+k
>>> +-+-+-+-+-+-+-----+-+
>>>
>>> And following are steps to reproduce this issue:
>>> dd if=/dev/zero of=./f2fs_pin.img bs=2M count=1024
>>> mkfs.f2fs f2fs_pin.img
>>> mkdir f2fs
>>> mount f2fs_pin.img ./f2fs
>>> cd f2fs
>>> dd if=/dev/zero of=./large_padding bs=1M count=1760
>>> ./pin_filling.sh
>>> rm padding*
>>> sync
>>> touch fallocate_40m
>>> f2fs_io pinfile set fallocate_40m
>>> fallocate -l 41943040 fallocate_40m
>>>
>>> fallocate always fail with EAGAIN even there has enough free space.
>>>
>>> 'pin_filling.sh' is:
>>> count=1
>>> while :
>>> do
>>>       # filling the seg space
>>>       for i in {1..511}:
>>>       do
>>>           name=padding_$count-$i
>>>           echo write $name
>>>           dd if=/dev/zero of=./$name bs=4K count=1 > /dev/null 2>&1
>>>           if [ $? -ne 0 ]; then
>>>                   exit 0
>>>           fi
>>>       done
>>>       sync
>>>
>>>       # pin one block in a segment
>>>       name=pin_file$count
>>>       dd if=/dev/zero of=./$name bs=4K count=1 > /dev/null 2>&1
>>>       sync
>>>       f2fs_io pinfile set $name
>>>       count=$(($count + 1))
>>> done
>>>
>>> Signed-off-by: Wu Bo <bo.wu@...o.com>
>>> ---
>>>    fs/f2fs/file.c | 2 +-
>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
>>> index ca5904129b16..e8a13616543f 100644
>>> --- a/fs/f2fs/file.c
>>> +++ b/fs/f2fs/file.c
>>> @@ -1690,7 +1690,7 @@ static int f2fs_expand_inode_data(struct inode
>>> *inode, loff_t offset,
>>>                .init_gc_type = FG_GC,
>>>                .should_migrate_blocks = false,
>>>                .err_gc_skipped = true,
>>> -            .nr_free_secs = 0 };
>>> +            .nr_free_secs = 1 };
>>>        pgoff_t pg_start, pg_end;
>>>        loff_t new_size;
>>>        loff_t off_end;