From: 刘通 <lyutoon@gmail.com>
To: Christoph Hellwig <hch@lst.de>
Cc: Chandan Babu R <chandan.babu@oracle.com>,
"Darrick J. Wong" <djwong@kernel.org>,
Dave Chinner <david@fromorbit.com>,
linux-xfs@vger.kernel.org
Subject: Re: [PATCH 1/9] xfs: fix error returns from xfs_bmapi_write
Date: Mon, 6 May 2024 20:39:30 +0800 [thread overview]
Message-ID: <CAEJPjCuj2AH2iUh4fcL9Ux-D52utREpXp7XGVdhGwYnDNuDMSA@mail.gmail.com> (raw)
In-Reply-To: <CAEJPjCu5CWEMHHpLS2yB7tk9Hh52EsQ5npifKiw--U-50PLEng@mail.gmail.com>
Hello!
Thank you for promptly fixing this issue. May I ask if it's possible
to assign a CVE to this vulnerability?
Once again, thank you for your efforts in fixing this vulnerability!
Wish you all the best.
Tong
刘通 <lyutoon@gmail.com> 于2024年5月6日周一 20:24写道：
>
> Hello!
>
> Thank you for promptly fixing this issue. May I ask if it's possible to assign a CVE to this vulnerability?
> Once again, thank you for your efforts in fixing this vulnerability!
>
> Wish you all the best.
>
> Tong
>
> Christoph Hellwig <hch@lst.de> 于2024年4月29日周一 14:15写道：
>>
>> xfs_bmapi_write can return 0 without actually returning a mapping in
>> mval in two different cases:
>>
>> 1) when there is absolutely no space available to do an allocation
>> 2) when converting delalloc space, and the allocation is so small
>> that it only covers parts of the delalloc extent before the
>> range requested by the caller
>>
>> Callers at best can handle one of these cases, but in many cases can't
>> cope with either one. Switch xfs_bmapi_write to always return a
>> mapping or return an error code instead. For case 1) above ENOSPC is
>> the obvious choice which is very much what the callers expect anyway.
>> For case 2) there is no really good error code, so pick a funky one
>> from the SysV streams portfolio.
>>
>> This fixes the reproducer here:
>>
>> https://lore.kernel.org/linux-xfs/CAEJPjCvT3Uag-pMTYuigEjWZHn1sGMZ0GCjVVCv29tNHK76Cgg@mail.gmail.com0/
>>
>> which uses reserved blocks to create file systems that are gravely
>> out of space and thus cause at least xfs_file_alloc_space to hang
>> and trigger the lack of ENOSPC handling in xfs_dquot_disk_alloc.
>>
>> Note that this patch does not actually make any caller but
>> xfs_alloc_file_space deal intelligently with case 2) above.
>>
>> Signed-off-by: Christoph Hellwig <hch@lst.de>
>> Reported-by: 刘通 <lyutoon@gmail.com>
>> Reviewed-by: Darrick J. Wong <djwong@kernel.org>
>> ---
>> fs/xfs/libxfs/xfs_attr_remote.c | 1 -
>> fs/xfs/libxfs/xfs_bmap.c | 46 ++++++++++++++++++++++++++-------
>> fs/xfs/libxfs/xfs_da_btree.c | 20 ++++----------
>> fs/xfs/scrub/quota_repair.c | 6 -----
>> fs/xfs/scrub/rtbitmap_repair.c | 2 --
>> fs/xfs/xfs_bmap_util.c | 31 +++++++++++-----------
>> fs/xfs/xfs_dquot.c | 1 -
>> fs/xfs/xfs_iomap.c | 8 ------
>> fs/xfs/xfs_reflink.c | 14 ----------
>> fs/xfs/xfs_rtalloc.c | 2 --
>> 10 files changed, 57 insertions(+), 74 deletions(-)
>>
>> diff --git a/fs/xfs/libxfs/xfs_attr_remote.c b/fs/xfs/libxfs/xfs_attr_remote.c
>> index a8de9dc1e998a3..beb0efdd8f6b83 100644
>> --- a/fs/xfs/libxfs/xfs_attr_remote.c
>> +++ b/fs/xfs/libxfs/xfs_attr_remote.c
>> @@ -625,7 +625,6 @@ xfs_attr_rmtval_set_blk(
>> if (error)
>> return error;
>>
>> - ASSERT(nmap == 1);
>> ASSERT((map->br_startblock != DELAYSTARTBLOCK) &&
>> (map->br_startblock != HOLESTARTBLOCK));
>>
>> diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
>> index 6053f5e5c71eec..f19191d6eade7e 100644
>> --- a/fs/xfs/libxfs/xfs_bmap.c
>> +++ b/fs/xfs/libxfs/xfs_bmap.c
>> @@ -4217,8 +4217,10 @@ xfs_bmapi_allocate(
>> } else {
>> error = xfs_bmap_alloc_userdata(bma);
>> }
>> - if (error || bma->blkno == NULLFSBLOCK)
>> + if (error)
>> return error;
>> + if (bma->blkno == NULLFSBLOCK)
>> + return -ENOSPC;
>>
>> if (bma->flags & XFS_BMAPI_ZERO) {
>> error = xfs_zero_extent(bma->ip, bma->blkno, bma->length);
>> @@ -4397,6 +4399,15 @@ xfs_bmapi_finish(
>> * extent state if necessary. Details behaviour is controlled by the flags
>> * parameter. Only allocates blocks from a single allocation group, to avoid
>> * locking problems.
>> + *
>> + * Returns 0 on success and places the extent mappings in mval. nmaps is used
>> + * as an input/output parameter where the caller specifies the maximum number
>> + * of mappings that may be returned and xfs_bmapi_write passes back the number
>> + * of mappings (including existing mappings) it found.
>> + *
>> + * Returns a negative error code on failure, including -ENOSPC when it could not
>> + * allocate any blocks and -ENOSR when it did allocate blocks to convert a
>> + * delalloc range, but those blocks were before the passed in range.
>> */
>> int
>> xfs_bmapi_write(
>> @@ -4525,10 +4536,16 @@ xfs_bmapi_write(
>> ASSERT(len > 0);
>> ASSERT(bma.length > 0);
>> error = xfs_bmapi_allocate(&bma);
>> - if (error)
>> + if (error) {
>> + /*
>> + * If we already allocated space in a previous
>> + * iteration return what we go so far when
>> + * running out of space.
>> + */
>> + if (error == -ENOSPC && bma.nallocs)
>> + break;
>> goto error0;
>> - if (bma.blkno == NULLFSBLOCK)
>> - break;
>> + }
>>
>> /*
>> * If this is a CoW allocation, record the data in
>> @@ -4566,7 +4583,6 @@ xfs_bmapi_write(
>> if (!xfs_iext_next_extent(ifp, &bma.icur, &bma.got))
>> eof = true;
>> }
>> - *nmap = n;
>>
>> error = xfs_bmap_btree_to_extents(tp, ip, bma.cur, &bma.logflags,
>> whichfork);
>> @@ -4577,7 +4593,22 @@ xfs_bmapi_write(
>> ifp->if_nextents > XFS_IFORK_MAXEXT(ip, whichfork));
>> xfs_bmapi_finish(&bma, whichfork, 0);
>> xfs_bmap_validate_ret(orig_bno, orig_len, orig_flags, orig_mval,
>> - orig_nmap, *nmap);
>> + orig_nmap, n);
>> +
>> + /*
>> + * When converting delayed allocations, xfs_bmapi_allocate ignores
>> + * the passed in bno and always converts from the start of the found
>> + * delalloc extent.
>> + *
>> + * To avoid a successful return with *nmap set to 0, return the magic
>> + * -ENOSR error code for this particular case so that the caller can
>> + * handle it.
>> + */
>> + if (!n) {
>> + ASSERT(bma.nallocs >= *nmap);
>> + return -ENOSR;
>> + }
>> + *nmap = n;
>> return 0;
>> error0:
>> xfs_bmapi_finish(&bma, whichfork, error);
>> @@ -4684,9 +4715,6 @@ xfs_bmapi_convert_delalloc(
>> if (error)
>> goto out_finish;
>>
>> - error = -ENOSPC;
>> - if (WARN_ON_ONCE(bma.blkno == NULLFSBLOCK))
>> - goto out_finish;
>> if (WARN_ON_ONCE(!xfs_valid_startblock(ip, bma.got.br_startblock))) {
>> xfs_bmap_mark_sick(ip, whichfork);
>> error = -EFSCORRUPTED;
>> diff --git a/fs/xfs/libxfs/xfs_da_btree.c b/fs/xfs/libxfs/xfs_da_btree.c
>> index b13796629e2213..16a529a8878083 100644
>> --- a/fs/xfs/libxfs/xfs_da_btree.c
>> +++ b/fs/xfs/libxfs/xfs_da_btree.c
>> @@ -2297,8 +2297,8 @@ xfs_da_grow_inode_int(
>> struct xfs_inode *dp = args->dp;
>> int w = args->whichfork;
>> xfs_rfsblock_t nblks = dp->i_nblocks;
>> - struct xfs_bmbt_irec map, *mapp;
>> - int nmap, error, got, i, mapi;
>> + struct xfs_bmbt_irec map, *mapp = ↦
>> + int nmap, error, got, i, mapi = 1;
>>
>> /*
>> * Find a spot in the file space to put the new block.
>> @@ -2314,14 +2314,7 @@ xfs_da_grow_inode_int(
>> error = xfs_bmapi_write(tp, dp, *bno, count,
>> xfs_bmapi_aflag(w)|XFS_BMAPI_METADATA|XFS_BMAPI_CONTIG,
>> args->total, &map, &nmap);
>> - if (error)
>> - return error;
>> -
>> - ASSERT(nmap <= 1);
>> - if (nmap == 1) {
>> - mapp = ↦
>> - mapi = 1;
>> - } else if (nmap == 0 && count > 1) {
>> + if (error == -ENOSPC && count > 1) {
>> xfs_fileoff_t b;
>> int c;
>>
>> @@ -2339,16 +2332,13 @@ xfs_da_grow_inode_int(
>> args->total, &mapp[mapi], &nmap);
>> if (error)
>> goto out_free_map;
>> - if (nmap < 1)
>> - break;
>> mapi += nmap;
>> b = mapp[mapi - 1].br_startoff +
>> mapp[mapi - 1].br_blockcount;
>> }
>> - } else {
>> - mapi = 0;
>> - mapp = NULL;
>> }
>> + if (error)
>> + goto out_free_map;
>>
>> /*
>> * Count the blocks we got, make sure it matches the total.
>> diff --git a/fs/xfs/scrub/quota_repair.c b/fs/xfs/scrub/quota_repair.c
>> index 0bab4c30cb85ab..90cd1512bba961 100644
>> --- a/fs/xfs/scrub/quota_repair.c
>> +++ b/fs/xfs/scrub/quota_repair.c
>> @@ -77,8 +77,6 @@ xrep_quota_item_fill_bmap_hole(
>> irec, &nmaps);
>> if (error)
>> return error;
>> - if (nmaps != 1)
>> - return -ENOSPC;
>>
>> dq->q_blkno = XFS_FSB_TO_DADDR(mp, irec->br_startblock);
>>
>> @@ -444,10 +442,6 @@ xrep_quota_data_fork(
>> XFS_BMAPI_CONVERT, 0, &nrec, &nmap);
>> if (error)
>> goto out;
>> - if (nmap != 1) {
>> - error = -ENOSPC;
>> - goto out;
>> - }
>> ASSERT(nrec.br_startoff == irec.br_startoff);
>> ASSERT(nrec.br_blockcount == irec.br_blockcount);
>>
>> diff --git a/fs/xfs/scrub/rtbitmap_repair.c b/fs/xfs/scrub/rtbitmap_repair.c
>> index 46f5d5f605c915..0fef98e9f83409 100644
>> --- a/fs/xfs/scrub/rtbitmap_repair.c
>> +++ b/fs/xfs/scrub/rtbitmap_repair.c
>> @@ -108,8 +108,6 @@ xrep_rtbitmap_data_mappings(
>> 0, &map, &nmaps);
>> if (error)
>> return error;
>> - if (nmaps != 1)
>> - return -EFSCORRUPTED;
>>
>> /* Commit new extent and all deferred work. */
>> error = xrep_defer_finish(sc);
>> diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
>> index 53aa90a0ee3a85..2e6f08198c0719 100644
>> --- a/fs/xfs/xfs_bmap_util.c
>> +++ b/fs/xfs/xfs_bmap_util.c
>> @@ -721,33 +721,32 @@ xfs_alloc_file_space(
>> if (error)
>> goto error;
>>
>> - error = xfs_bmapi_write(tp, ip, startoffset_fsb,
>> - allocatesize_fsb, XFS_BMAPI_PREALLOC, 0, imapp,
>> - &nimaps);
>> - if (error)
>> - goto error;
>> -
>> - ip->i_diflags |= XFS_DIFLAG_PREALLOC;
>> - xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
>> -
>> - error = xfs_trans_commit(tp);
>> - xfs_iunlock(ip, XFS_ILOCK_EXCL);
>> - if (error)
>> - break;
>> -
>> /*
>> * If the allocator cannot find a single free extent large
>> * enough to cover the start block of the requested range,
>> - * xfs_bmapi_write will return 0 but leave *nimaps set to 0.
>> + * xfs_bmapi_write will return -ENOSR.
>> *
>> * In that case we simply need to keep looping with the same
>> * startoffset_fsb so that one of the following allocations
>> * will eventually reach the requested range.
>> */
>> - if (nimaps) {
>> + error = xfs_bmapi_write(tp, ip, startoffset_fsb,
>> + allocatesize_fsb, XFS_BMAPI_PREALLOC, 0, imapp,
>> + &nimaps);
>> + if (error) {
>> + if (error != -ENOSR)
>> + goto error;
>> + error = 0;
>> + } else {
>> startoffset_fsb += imapp->br_blockcount;
>> allocatesize_fsb -= imapp->br_blockcount;
>> }
>> +
>> + ip->i_diflags |= XFS_DIFLAG_PREALLOC;
>> + xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
>> +
>> + error = xfs_trans_commit(tp);
>> + xfs_iunlock(ip, XFS_ILOCK_EXCL);
>> }
>>
>> return error;
>> diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c
>> index 13aba84bd64afb..43acb4f0d17433 100644
>> --- a/fs/xfs/xfs_dquot.c
>> +++ b/fs/xfs/xfs_dquot.c
>> @@ -357,7 +357,6 @@ xfs_dquot_disk_alloc(
>> goto err_cancel;
>>
>> ASSERT(map.br_blockcount == XFS_DQUOT_CLUSTER_SIZE_FSB);
>> - ASSERT(nmaps == 1);
>> ASSERT((map.br_startblock != DELAYSTARTBLOCK) &&
>> (map.br_startblock != HOLESTARTBLOCK));
>>
>> diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
>> index 9ce0f6b9df93e6..60463160820b62 100644
>> --- a/fs/xfs/xfs_iomap.c
>> +++ b/fs/xfs/xfs_iomap.c
>> @@ -322,14 +322,6 @@ xfs_iomap_write_direct(
>> if (error)
>> goto out_unlock;
>>
>> - /*
>> - * Copy any maps to caller's array and return any error.
>> - */
>> - if (nimaps == 0) {
>> - error = -ENOSPC;
>> - goto out_unlock;
>> - }
>> -
>> if (unlikely(!xfs_valid_startblock(ip, imap->br_startblock))) {
>> xfs_bmap_mark_sick(ip, XFS_DATA_FORK);
>> error = xfs_alert_fsblock_zero(ip, imap);
>> diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
>> index 7da0e8f961d351..5ecb52a234becc 100644
>> --- a/fs/xfs/xfs_reflink.c
>> +++ b/fs/xfs/xfs_reflink.c
>> @@ -430,13 +430,6 @@ xfs_reflink_fill_cow_hole(
>> if (error)
>> return error;
>>
>> - /*
>> - * Allocation succeeded but the requested range was not even partially
>> - * satisfied? Bail out!
>> - */
>> - if (nimaps == 0)
>> - return -ENOSPC;
>> -
>> convert:
>> return xfs_reflink_convert_unwritten(ip, imap, cmap, convert_now);
>>
>> @@ -499,13 +492,6 @@ xfs_reflink_fill_delalloc(
>> error = xfs_trans_commit(tp);
>> if (error)
>> return error;
>> -
>> - /*
>> - * Allocation succeeded but the requested range was not even
>> - * partially satisfied? Bail out!
>> - */
>> - if (nimaps == 0)
>> - return -ENOSPC;
>> } while (cmap->br_startoff + cmap->br_blockcount <= imap->br_startoff);
>>
>> return xfs_reflink_convert_unwritten(ip, imap, cmap, convert_now);
>> diff --git a/fs/xfs/xfs_rtalloc.c b/fs/xfs/xfs_rtalloc.c
>> index b476a876478d93..150f544445ca82 100644
>> --- a/fs/xfs/xfs_rtalloc.c
>> +++ b/fs/xfs/xfs_rtalloc.c
>> @@ -709,8 +709,6 @@ xfs_growfs_rt_alloc(
>> nmap = 1;
>> error = xfs_bmapi_write(tp, ip, oblocks, nblocks - oblocks,
>> XFS_BMAPI_METADATA, 0, &map, &nmap);
>> - if (!error && nmap < 1)
>> - error = -ENOSPC;
>> if (error)
>> goto out_trans_cancel;
>> /*
>> --
>> 2.39.2
>>
next prev parent reply other threads:[~2024-05-06 12:39 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-29 6:15 xfs_bmapi_write retval fix v2 Christoph Hellwig
2024-04-29 6:15 ` [PATCH 1/9] xfs: fix error returns from xfs_bmapi_write Christoph Hellwig
[not found] ` <CAEJPjCu5CWEMHHpLS2yB7tk9Hh52EsQ5npifKiw--U-50PLEng@mail.gmail.com>
2024-05-06 12:39 ` 刘通 [this message]
2024-04-29 6:15 ` [PATCH 2/9] xfs: remove the unusued tmp_logflags variable in xfs_bmapi_allocate Christoph Hellwig
2024-04-29 6:15 ` [PATCH 3/9] xfs: lift a xfs_valid_startblock into xfs_bmapi_allocate Christoph Hellwig
2024-04-29 6:15 ` [PATCH 4/9] xfs: don't open code XFS_FILBLKS_MIN in xfs_bmapi_write Christoph Hellwig
2024-04-29 6:15 ` [PATCH 5/9] xfs: pass the actual offset and len to allocate to xfs_bmapi_allocate Christoph Hellwig
2024-04-29 15:48 ` Darrick J. Wong
2024-04-29 17:18 ` Christoph Hellwig
2024-04-29 17:20 ` Darrick J. Wong
2024-04-29 6:15 ` [PATCH 6/9] xfs: remove the xfs_iext_peek_prev_extent call in xfs_bmapi_allocate Christoph Hellwig
2024-04-29 6:15 ` [PATCH 7/9] xfs: fix xfs_bmap_add_extent_delay_real for partial conversions Christoph Hellwig
2024-04-29 15:48 ` Darrick J. Wong
2024-04-29 6:15 ` [PATCH 8/9] xfs: do not allocate the entire delalloc extent in xfs_bmapi_write Christoph Hellwig
2024-04-29 6:15 ` [PATCH 9/9] mm,page_owner: don't remove GFP flags in add_stack_record_to_list Christoph Hellwig
2024-04-29 6:17 ` Christoph Hellwig
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAEJPjCuj2AH2iUh4fcL9Ux-D52utREpXp7XGVdhGwYnDNuDMSA@mail.gmail.com \
--to=lyutoon@gmail.com \
--cc=chandan.babu@oracle.com \
--cc=david@fromorbit.com \
--cc=djwong@kernel.org \
--cc=hch@lst.de \
--cc=linux-xfs@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).