xfs
[Top] [All Lists]

Re: [PATCH v2] xfs: fix possible overflow in xfs_ioc_trim()

To: Lukas Czerner <lczerner@xxxxxxxxxx>
Subject: Re: [PATCH v2] xfs: fix possible overflow in xfs_ioc_trim()
From: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Date: Tue, 20 Sep 2011 13:12:20 -0400
Cc: Christoph Hellwig <hch@xxxxxxxxxxxxx>, xfs@xxxxxxxxxxx
In-reply-to: <alpine.LFD.2.00.1109071333110.4579@xxxxxxxxxxxxxxxxxxxxxxxxxx>
References: <1315322977-22736-1-git-send-email-lczerner@xxxxxxxxxx> <20110906153301.GA21675@xxxxxxxxxxxxx> <alpine.LFD.2.00.1109071200480.4579@xxxxxxxxxxxxxxxxxxxxxxxxxx> <20110907112155.GA1017@xxxxxxxxxxxxx> <alpine.LFD.2.00.1109071333110.4579@xxxxxxxxxxxxxxxxxxxxxxxxxx>
User-agent: Mutt/1.5.21 (2010-09-15)
On Wed, Sep 07, 2011 at 02:26:54PM +0200, Lukas Czerner wrote:
> I do not think that start + len can overflow since we are doing
> XFS_B_TO_FSBT() on it first. Am I missing something ?

They don't have to overflow, but they can easily be outside
the range of valid AGs.

> > Care to update the test case to cover these cases as well?
> > 
> 
> I am not sure what do you mean ? There already is a check when both
> start and len are huge numbers. I am not sure if we can do more without
> significantly complicating the test to cover various start, or len
> numbers where can the fsblock->group_number overflow for various file
> systems.

Add a testcase where start is a relatively small number (smaller than an
AG/BG), but start + len is outside the fs.

> @@ -145,7 +145,7 @@ xfs_ioc_trim(
>       struct request_queue    *q = mp->m_ddev_targp->bt_bdev->bd_disk->queue;
>       unsigned int            granularity = q->limits.discard_granularity;
>       struct fstrim_range     range;
> +     xfs_fsblock_t           start, end, minlen;
>       xfs_agnumber_t          start_agno, end_agno, agno;
>       __uint64_t              blocks_trimmed = 0;
>       int                     error, last_error = 0;
> @@ -165,19 +165,21 @@ xfs_ioc_trim(
>        * matter as trimming blocks is an advisory interface.
>        */
>       start = XFS_B_TO_FSBT(mp, range.start);
> +     end = start + XFS_B_TO_FSBT(mp, range.len) - 1;
>       minlen = XFS_B_TO_FSB(mp, max_t(u64, granularity, range.minlen));
>  
> +     if (start >= mp->m_sb.sb_dblocks)
>               return -XFS_ERROR(EINVAL);
> +     start_agno = XFS_FSB_TO_AGNO(mp, start);
>  
> +     if (end >= mp->m_sb.sb_dblocks) {
> +             end = mp->m_sb.sb_dblocks - 1;
>               end_agno = mp->m_sb.sb_agcount - 1;
> +     } else
> +             end_agno = XFS_FSB_TO_AGNO(mp, end);

I'd rather do something like:

        if (start >= mp->m_sb.sb_dblocks)
                return -XFS_ERROR(EINVAL);
        if (end > mp->m_sb.sb_dblocks - 1)
                end = mp->m_sb.sb_dblocks - 1;


        start_agno = XFS_FSB_TO_AGNO(mp, start);
        end_agno = XFS_FSB_TO_AGNO(mp, end)

here.

Otherwise the patch looks fine.

<Prev in Thread] Current Thread [Next in Thread>