Hi Matthew,
FYI, the error/warning still remains.
tree: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master
head: f6eb0fed6a3957c0b93e3a00c1ffaad84d4ffc31
commit: 933906f8e8e4110c56db9bddd1281e4e4983a2bb ntfs: Convert ntfs to read_folio
date: 3 months ago
config: hexagon-buildonly-randconfig-r001-20220814 (https://download.01.org/0day-ci/archive/20220814/[email protected]/config)
compiler: clang version 16.0.0 (https://github.com/llvm/llvm-project 3329cec2f79185bafd678f310fafadba2a8c76d2)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=933906f8e8e4110c56db9bddd1281e4e4983a2bb
git remote add linus https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
git fetch --no-tags linus master
git checkout 933906f8e8e4110c56db9bddd1281e4e4983a2bb
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=hexagon SHELL=/bin/bash fs/ntfs/
If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <[email protected]>
All warnings (new ones prefixed by >>):
>> fs/ntfs/aops.c:378:12: warning: stack frame size (2216) exceeds limit (1024) in 'ntfs_read_folio' [-Wframe-larger-than]
static int ntfs_read_folio(struct file *file, struct folio *folio)
^
1 warning generated.
vim +/ntfs_read_folio +378 fs/ntfs/aops.c
359
360 /**
361 * ntfs_read_folio - fill a @folio of a @file with data from the device
362 * @file: open file to which the folio @folio belongs or NULL
363 * @folio: page cache folio to fill with data
364 *
365 * For non-resident attributes, ntfs_read_folio() fills the @folio of the open
366 * file @file by calling the ntfs version of the generic block_read_full_folio()
367 * function, ntfs_read_block(), which in turn creates and reads in the buffers
368 * associated with the folio asynchronously.
369 *
370 * For resident attributes, OTOH, ntfs_read_folio() fills @folio by copying the
371 * data from the mft record (which at this stage is most likely in memory) and
372 * fills the remainder with zeroes. Thus, in this case, I/O is synchronous, as
373 * even if the mft record is not cached at this point in time, we need to wait
374 * for it to be read in before we can do the copy.
375 *
376 * Return 0 on success and -errno on error.
377 */
> 378 static int ntfs_read_folio(struct file *file, struct folio *folio)
379 {
380 struct page *page = &folio->page;
381 loff_t i_size;
382 struct inode *vi;
383 ntfs_inode *ni, *base_ni;
384 u8 *addr;
385 ntfs_attr_search_ctx *ctx;
386 MFT_RECORD *mrec;
387 unsigned long flags;
388 u32 attr_len;
389 int err = 0;
390
391 retry_readpage:
392 BUG_ON(!PageLocked(page));
393 vi = page->mapping->host;
394 i_size = i_size_read(vi);
395 /* Is the page fully outside i_size? (truncate in progress) */
396 if (unlikely(page->index >= (i_size + PAGE_SIZE - 1) >>
397 PAGE_SHIFT)) {
398 zero_user(page, 0, PAGE_SIZE);
399 ntfs_debug("Read outside i_size - truncated?");
400 goto done;
401 }
402 /*
403 * This can potentially happen because we clear PageUptodate() during
404 * ntfs_writepage() of MstProtected() attributes.
405 */
406 if (PageUptodate(page)) {
407 unlock_page(page);
408 return 0;
409 }
410 ni = NTFS_I(vi);
411 /*
412 * Only $DATA attributes can be encrypted and only unnamed $DATA
413 * attributes can be compressed. Index root can have the flags set but
414 * this means to create compressed/encrypted files, not that the
415 * attribute is compressed/encrypted. Note we need to check for
416 * AT_INDEX_ALLOCATION since this is the type of both directory and
417 * index inodes.
418 */
419 if (ni->type != AT_INDEX_ALLOCATION) {
420 /* If attribute is encrypted, deny access, just like NT4. */
421 if (NInoEncrypted(ni)) {
422 BUG_ON(ni->type != AT_DATA);
423 err = -EACCES;
424 goto err_out;
425 }
426 /* Compressed data streams are handled in compress.c. */
427 if (NInoNonResident(ni) && NInoCompressed(ni)) {
428 BUG_ON(ni->type != AT_DATA);
429 BUG_ON(ni->name_len);
430 return ntfs_read_compressed_block(page);
431 }
432 }
433 /* NInoNonResident() == NInoIndexAllocPresent() */
434 if (NInoNonResident(ni)) {
435 /* Normal, non-resident data stream. */
436 return ntfs_read_block(page);
437 }
438 /*
439 * Attribute is resident, implying it is not compressed or encrypted.
440 * This also means the attribute is smaller than an mft record and
441 * hence smaller than a page, so can simply zero out any pages with
442 * index above 0. Note the attribute can actually be marked compressed
443 * but if it is resident the actual data is not compressed so we are
444 * ok to ignore the compressed flag here.
445 */
446 if (unlikely(page->index > 0)) {
447 zero_user(page, 0, PAGE_SIZE);
448 goto done;
449 }
450 if (!NInoAttr(ni))
451 base_ni = ni;
452 else
453 base_ni = ni->ext.base_ntfs_ino;
454 /* Map, pin, and lock the mft record. */
455 mrec = map_mft_record(base_ni);
456 if (IS_ERR(mrec)) {
457 err = PTR_ERR(mrec);
458 goto err_out;
459 }
460 /*
461 * If a parallel write made the attribute non-resident, drop the mft
462 * record and retry the read_folio.
463 */
464 if (unlikely(NInoNonResident(ni))) {
465 unmap_mft_record(base_ni);
466 goto retry_readpage;
467 }
468 ctx = ntfs_attr_get_search_ctx(base_ni, mrec);
469 if (unlikely(!ctx)) {
470 err = -ENOMEM;
471 goto unm_err_out;
472 }
473 err = ntfs_attr_lookup(ni->type, ni->name, ni->name_len,
474 CASE_SENSITIVE, 0, NULL, 0, ctx);
475 if (unlikely(err))
476 goto put_unm_err_out;
477 attr_len = le32_to_cpu(ctx->attr->data.resident.value_length);
478 read_lock_irqsave(&ni->size_lock, flags);
479 if (unlikely(attr_len > ni->initialized_size))
480 attr_len = ni->initialized_size;
481 i_size = i_size_read(vi);
482 read_unlock_irqrestore(&ni->size_lock, flags);
483 if (unlikely(attr_len > i_size)) {
484 /* Race with shrinking truncate. */
485 attr_len = i_size;
486 }
487 addr = kmap_atomic(page);
488 /* Copy the data to the page. */
489 memcpy(addr, (u8*)ctx->attr +
490 le16_to_cpu(ctx->attr->data.resident.value_offset),
491 attr_len);
492 /* Zero the remainder of the page. */
493 memset(addr + attr_len, 0, PAGE_SIZE - attr_len);
494 flush_dcache_page(page);
495 kunmap_atomic(addr);
496 put_unm_err_out:
497 ntfs_attr_put_search_ctx(ctx);
498 unm_err_out:
499 unmap_mft_record(base_ni);
500 done:
501 SetPageUptodate(page);
502 err_out:
503 unlock_page(page);
504 return err;
505 }
506
--
0-DAY CI Kernel Test Service
https://01.org/lkp
On Sun, Aug 14, 2022 at 08:21:36AM +0800, kernel test robot wrote:
> Hi Matthew,
>
> FYI, the error/warning still remains.
FYI, this is still not interesting.
This is a hexagon 256kB PAGE_SIZE config, and so the amount of stack
space is correspondingly larger. The frame size warning should be
increased to allow for this.
> >> fs/ntfs/aops.c:378:12: warning: stack frame size (2216) exceeds limit (1024) in 'ntfs_read_folio' [-Wframe-larger-than]
On Mon, Aug 15, 2022 at 2:29 PM Matthew Wilcox <[email protected]> wrote:
>
> On Sun, Aug 14, 2022 at 08:21:36AM +0800, kernel test robot wrote:
> > Hi Matthew,
> >
> > FYI, the error/warning still remains.
>
> FYI, this is still not interesting.
> This is a hexagon 256kB PAGE_SIZE config, and so the amount of stack
> space is correspondingly larger. The frame size warning should be
> increased to allow for this.
>
> > >> fs/ntfs/aops.c:378:12: warning: stack frame size (2216) exceeds limit (1024) in 'ntfs_read_folio' [-Wframe-larger-than]
I don't think we should change the frame size warning for this, there is not
generally any correlation between page size and stack usage, so that would
just hide bugs elsewhere.
NTFS has had problems with stack usage on 64K+ pages before, the last
time we addressed this using 4eec7faf6775 ("fs: ntfs: Limit NTFS_RW to
page sizes smaller than 64k"), but it looks like this time it affects both
write and read support.
I checked hexagon ntfs with 64KB pages, and that stays below the 1024 byte
limit, so we could add another dependency
--- a/fs/ntfs/Kconfig
+++ b/fs/ntfs/Kconfig
@@ -1,6 +1,7 @@
# SPDX-License-Identifier: GPL-2.0-only
config NTFS_FS
tristate "NTFS file system support"
+ depends on PAGE_SIZE_LESS_THAN_256KB
select NLS
help
NTFS is the file system of Microsoft Windows NT, 2000, XP and 2003.
to rule out the broken case but still allow powerpc64 and arm64 with
64KB to use NTFS.
Arnd
On Mon, Aug 15, 2022 at 02:56:11PM +0200, Arnd Bergmann wrote:
> On Mon, Aug 15, 2022 at 2:29 PM Matthew Wilcox <[email protected]> wrote:
> >
> > On Sun, Aug 14, 2022 at 08:21:36AM +0800, kernel test robot wrote:
> > > Hi Matthew,
> > >
> > > FYI, the error/warning still remains.
> >
> > FYI, this is still not interesting.
> > This is a hexagon 256kB PAGE_SIZE config, and so the amount of stack
> > space is correspondingly larger. The frame size warning should be
> > increased to allow for this.
> >
> > > >> fs/ntfs/aops.c:378:12: warning: stack frame size (2216) exceeds limit (1024) in 'ntfs_read_folio' [-Wframe-larger-than]
>
> I don't think we should change the frame size warning for this, there is not
> generally any correlation between page size and stack usage, so that would
> just hide bugs elsewhere.
In this specific case, there is. It's a stack allocation of an array
that depends on the number of 512-byte blocks per page. With 4k pages,
that's only 8. With 256k pages, that's 512. With an 8-byte pointer,
that's a 4kB allocation, and even with a 4-byte pointer, that's a 2kB
stack allocation, which is still going to blow the prescribed stack
limit.
This is not unique to NTFS! An NTFS-specific "fix" is inappropriate.
It's just that nobody's paying attention to the warnings coming from
fs/buffer.c:
include/linux/buffer_head.h:#define MAX_BUF_PER_PAGE (PAGE_SIZE / 512)
int block_read_full_folio(struct folio *folio, get_block_t *get_block)
{
...
struct buffer_head *bh, *head, *arr[MAX_BUF_PER_PAGE];
I don't know why I'm not getting a nastygram about that one, but it's
all bufferhead based filesystems.
> NTFS has had problems with stack usage on 64K+ pages before, the last
> time we addressed this using 4eec7faf6775 ("fs: ntfs: Limit NTFS_RW to
> page sizes smaller than 64k"), but it looks like this time it affects both
> write and read support.
The reasoning there is faulty. If you have a 64k (or 256k) page size,
your stack is correspondingly huge and can handle these kinds of
allocations.
On Mon, Aug 15, 2022 at 3:05 PM Matthew Wilcox <[email protected]> wrote:
> On Mon, Aug 15, 2022 at 02:56:11PM +0200, Arnd Bergmann wrote:
> > On Mon, Aug 15, 2022 at 2:29 PM Matthew Wilcox <[email protected]> wrote:
> > >
> > > On Sun, Aug 14, 2022 at 08:21:36AM +0800, kernel test robot wrote:
> > > > Hi Matthew,
> > > >
> > > > FYI, the error/warning still remains.
> > >
> > > FYI, this is still not interesting.
> > > This is a hexagon 256kB PAGE_SIZE config, and so the amount of stack
> > > space is correspondingly larger. The frame size warning should be
> > > increased to allow for this.
> > >
> > > > >> fs/ntfs/aops.c:378:12: warning: stack frame size (2216) exceeds limit (1024) in 'ntfs_read_folio' [-Wframe-larger-than]
> >
> > I don't think we should change the frame size warning for this, there is not
> > generally any correlation between page size and stack usage, so that would
> > just hide bugs elsewhere.
>
> In this specific case, there is. It's a stack allocation of an array
> that depends on the number of 512-byte blocks per page. With 4k pages,
> that's only 8. With 256k pages, that's 512. With an 8-byte pointer,
> that's a 4kB allocation, and even with a 4-byte pointer, that's a 2kB
> stack allocation, which is still going to blow the prescribed stack
> limit.
>
> This is not unique to NTFS! An NTFS-specific "fix" is inappropriate.
> It's just that nobody's paying attention to the warnings coming from
> fs/buffer.c:
>
> include/linux/buffer_head.h:#define MAX_BUF_PER_PAGE (PAGE_SIZE / 512)
>
> int block_read_full_folio(struct folio *folio, get_block_t *get_block)
> {
> ...
> struct buffer_head *bh, *head, *arr[MAX_BUF_PER_PAGE];
>
> I don't know why I'm not getting a nastygram about that one, but it's
> all bufferhead based filesystems.
I can confirm I see this warning with 256KB pages, in block_read_full_folio()
and others:
fs/mpage.c:131:20: error: stack frame size (4200) exceeds limit (1024)
in 'do_mpage_readpage' [-Werror,-Wframe-larger-than]
fs/mpage.c:447:12: error: stack frame size (4336) exceeds limit (1024)
in '__mpage_writepage' [-Werror,-Wframe-larger-than]
fs/buffer.c:2254:5: error: stack frame size (2152) exceeds limit
(1024) in 'block_read_full_folio' [-Werror,-Wframe-larger-than]
fs/fat/dir.c:1133:5: error: stack frame size (2104) exceeds limit
(1024) in 'fat_alloc_new_dir' [-Werror,-Wframe-larger-than]
fs/fat/fatent.c:466:5: error: stack frame size (2216) exceeds limit
(1024) in 'fat_alloc_clusters' [-Werror,-Wframe-larger-than]
fs/fat/fatent.c:554:5: error: stack frame size (2168) exceeds limit
(1024) in 'fat_free_clusters' [-Werror,-Wframe-larger-than]
fs/fat/dir.c:1281:5: error: stack frame size (2232) exceeds limit
(1024) in 'fat_add_entries' [-Werror,-Wframe-larger-than]
fs/ntfs3/fsntfs.c:738:5: error: stack frame size (2112) exceeds limit
(1024) in 'ntfs_clear_mft_tail' [-Werror,-Wframe-larger-than]
fs/ext4/move_extent.c:252:1: error: stack frame size (2272) exceeds
limit (1024) in 'move_extent_per_page' [-Werror,-Wframe-larger-than]
fs/ext4/readpage.c:223:5: error: stack frame size (4280) exceeds limit
(1024) in 'ext4_mpage_readpages' [-Werror,-Wframe-larger-than]
I still think that raising the warning limit here is not appropriate, having
a 512 element array of pointers on the stack is just not appropriate anywhere
IMHO.
> > NTFS has had problems with stack usage on 64K+ pages before, the last
> > time we addressed this using 4eec7faf6775 ("fs: ntfs: Limit NTFS_RW to
> > page sizes smaller than 64k"), but it looks like this time it affects both
> > write and read support.
>
> The reasoning there is faulty. If you have a 64k (or 256k) page size,
> your stack is correspondingly huge and can handle these kinds of
> allocations.
I think that is only the case for VMAP stacks, which require full pages,
but configurations without that use the "thread_stack" kmem_cache
for allocating stacks when THREAD_SIZE is smaller than PAGE_SIZE.
The THREAD_SIZE on Hexagon is 4KB, so do_mpage_readpage()
with 4200 bytes would immediately overflow that. Obviously 4KB stacks
are problematic already and only supported as options on sh and m68k
otherwise, but raising it to the usual 8KB would likely still cause the same
problem.
I have no problems with a patch removing support for 256KB pages if that
helps, as Hexagon is the only architecture to support this and there are close
to zero Linux users anway. This would leave only three warnings for 64KB
pages in allmodconfig:
fs/mpage.c:131:20: error: stack frame size (1128) exceeds limit (1024)
in 'do_mpage_readpage' [-Werror,-Wframe-larger-than]
fs/mpage.c:447:12: error: stack frame size (1264) exceeds limit (1024)
in '__mpage_writepage' [-Werror,-Wframe-larger-than]
fs/ext4/readpage.c:223:5: error: stack frame size (1208) exceeds limit
(1024) in 'ext4_mpage_readpages' [-Werror,-Wframe-larger-than]
Arnd
On Mon, Aug 15, 2022 at 3:48 PM Arnd Bergmann <[email protected]> wrote:
> I have no problems with a patch removing support for 256KB pages if that
> helps, as Hexagon is the only architecture to support this and there are close
> to zero Linux users anway. This would leave only three warnings for 64KB
> pages in allmodconfig:
>
> fs/mpage.c:131:20: error: stack frame size (1128) exceeds limit (1024)
> in 'do_mpage_readpage' [-Werror,-Wframe-larger-than]
> fs/mpage.c:447:12: error: stack frame size (1264) exceeds limit (1024)
> in '__mpage_writepage' [-Werror,-Wframe-larger-than]
> fs/ext4/readpage.c:223:5: error: stack frame size (1208) exceeds limit
> (1024) in 'ext4_mpage_readpages' [-Werror,-Wframe-larger-than]
I looked into these a bit more and found that these are arrays of sector_t,
which could be either 32-bit or 64-bit wide before 72deb455b5ec
("block: remove CONFIG_LBDAF"), but is now always 64-bit, so having
an array of 128 of these (65536/512) adds a 1KB to the stack and will
cause a warning. It's only slightly over the limit, and there are very few
32-bit systems that allow 64KB pages to trigger that warning.
I see now that ppc440 also supports 256KB pages and has the same
problem as hexagon, but also has been broken since the start of the
git history in this regard:
fs/mpage.c:638:1: error: the frame size of 4280 bytes is larger than
2048 bytes [-Werror=frame-larger-than=]
I don't know if anyone strongly cares about 256KB pages on
ppc44x any more, but given this, I'm fairly sure that they are
not using block based file systems. So we could just make
CONFIG_BLOCK depend on PAGE_SIZE_LESS_THAN_256KB
globally instead of dropping 256KB pages everywhere.
Arnd
On Mon, Aug 15, 2022 at 04:37:09PM +0200, Arnd Bergmann wrote:
> On Mon, Aug 15, 2022 at 3:48 PM Arnd Bergmann <[email protected]> wrote:
>
> > I have no problems with a patch removing support for 256KB pages if that
> > helps, as Hexagon is the only architecture to support this and there are close
> > to zero Linux users anway. This would leave only three warnings for 64KB
Right, I had brought up at least adjusting the dependencies of 256KB
pages so that it could not be selected with CONFIG_COMPILE_TEST to
reduce the number of warnings that would appear in randconfigs.
https://lore.kernel.org/[email protected]/
I suspect removing it outright would be fine too.
> > pages in allmodconfig:
> >
> > fs/mpage.c:131:20: error: stack frame size (1128) exceeds limit (1024)
> > in 'do_mpage_readpage' [-Werror,-Wframe-larger-than]
> > fs/mpage.c:447:12: error: stack frame size (1264) exceeds limit (1024)
> > in '__mpage_writepage' [-Werror,-Wframe-larger-than]
> > fs/ext4/readpage.c:223:5: error: stack frame size (1208) exceeds limit
> > (1024) in 'ext4_mpage_readpages' [-Werror,-Wframe-larger-than]
>
> I looked into these a bit more and found that these are arrays of sector_t,
> which could be either 32-bit or 64-bit wide before 72deb455b5ec
> ("block: remove CONFIG_LBDAF"), but is now always 64-bit, so having
> an array of 128 of these (65536/512) adds a 1KB to the stack and will
> cause a warning. It's only slightly over the limit, and there are very few
> 32-bit systems that allow 64KB pages to trigger that warning.
>
> I see now that ppc440 also supports 256KB pages and has the same
> problem as hexagon, but also has been broken since the start of the
> git history in this regard:
>
> fs/mpage.c:638:1: error: the frame size of 4280 bytes is larger than
> 2048 bytes [-Werror=frame-larger-than=]
>
> I don't know if anyone strongly cares about 256KB pages on
> ppc44x any more, but given this, I'm fairly sure that they are
> not using block based file systems. So we could just make
> CONFIG_BLOCK depend on PAGE_SIZE_LESS_THAN_256KB
> globally instead of dropping 256KB pages everywhere.
That doesn't sound like an unreasonable solution.
Cheers,
Nathan