Re: [PATCH 1/9] xfs: fix error returns from xfs_bmapi_write

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxx> 于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@xxxxxx> 于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@xxxxxx>
>> Reported-by: 刘通 <lyutoon@xxxxxxxxx>
>> Reviewed-by: Darrick J. Wong <djwong@xxxxxxxxxx>
>> ---
>>  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 = &map;
>> +       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 = &map;
>> -               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
>>





[Index of Archives]     [XFS Filesystem Development (older mail)]     [Linux Filesystem Development]     [Linux Audio Users]     [Yosemite Trails]     [Linux Kernel]     [Linux RAID]     [Linux SCSI]


  Powered by Linux