Received-SPF: pass (google.com: domain of linux-kernel+bounces-10419-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Message-ID: <bca8d526-2397-4ca5-b1d6-5758c9334a81@I-love.SAKURA.ne.jp>
Date: Sat, 23 Dec 2023 22:33:11 +0900
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH 5/8] fs/ntfs3: Use kvmalloc instead of kmalloc(...
 __GFP_NOWARN)
Content-Language: en-US
To: Matthew Wilcox <willy@infradead.org>,
        Konstantin Komarov <almaz.alexandrovich@paragon-software.com>
Cc: ntfs3@lists.linux.dev,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        linux-fsdevel@vger.kernel.org
References: <e41f6717-7c70-edf2-2d3a-8034840d14c5@paragon-software.com>
 <890222ac-1bd2-6817-7873-390801c5a172@paragon-software.com>
 <ZYZmFPnJAM3aJLlF@casper.infradead.org>
From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
In-Reply-To: <ZYZmFPnJAM3aJLlF@casper.infradead.org>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit

On 2023/12/23 13:46, Matthew Wilcox wrote:
> On Mon, Jul 03, 2023 at 11:26:36AM +0400, Konstantin Komarov wrote:
>>
>> Signed-off-by: Konstantin Komarov <almaz.alexandrovich@paragon-software.com>
> 
> So, um, why?  I mean, I know what kvmalloc does, but why do you want to
> do it?  Also, this is missing changes from kfree() to kvfree() so if
> you do end up falling back to vmalloc, you'll hit a bug in kfree().

Right. The first thing we should do is to revert commit fc471e39e38f("fs/ntfs3:
Use kvmalloc instead of kmalloc(... __GFP_NOWARN)"), for that commit is horrible.

> 
>> +++ b/fs/ntfs3/attrlist.c
>> @@ -52,7 +52,8 @@ int ntfs_load_attr_list(struct ntfs_inode *ni, struct
>> ATTRIB *attr)
>>
>>      if (!attr->non_res) {
>>          lsize = le32_to_cpu(attr->res.data_size);
>> -        le = kmalloc(al_aligned(lsize), GFP_NOFS | __GFP_NOWARN);
>> +        /* attr is resident: lsize < record_size (1K or 4K) */
>> +        le = kvmalloc(al_aligned(lsize), GFP_KERNEL);

syzbot is reporting that

  /* attr is resident: lsize < record_size (1K or 4K) */

was false at https://syzkaller.appspot.com/bug?extid=f987ceaddc6bcc334cde .
Maybe you can fix this report by adding more validations. Please be
sure to take into account that the filesystem image is corrupted/crafted.

But you can't replace GFP_NOFS with GFP_KERNEL anyway, for syzbot is also
reporting GFP_KERNEL allocation with filesystem lock held
at https://syzkaller.appspot.com/bug?extid=18f543fc90dd1194c616 .

By the way, you might want to add __GFP_RETRY_MAYFAIL when you can't use
GFP_KERNEL, for GFP_NOFS memory allocation request cannot trigger OOM
killer in order to allocate requested amount of memory, leading to
possibility of out-of-memory deadlock. Especially when there is possibility
that kvmalloc() needs to allocate so many pages...
See https://elixir.bootlin.com/linux/v6.7-rc6/source/mm/page_alloc.c#L3458 .