xfs
[Top] [All Lists]

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

To: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Subject: Re: [PATCH v2] xfs: fix possible overflow in xfs_ioc_trim()
From: Lukas Czerner <lczerner@xxxxxxxxxx>
Date: Wed, 21 Sep 2011 09:46:59 +0200 (CEST)
Cc: Lukas Czerner <lczerner@xxxxxxxxxx>, xfs@xxxxxxxxxxx
In-reply-to: <20110920171220.GA17204@xxxxxxxxxxxxx>
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> <20110920171220.GA17204@xxxxxxxxxxxxx>
User-agent: Alpine 2.00 (LFD 1167 2008-08-23)
On Tue, 20 Sep 2011, Christoph Hellwig wrote:

> > 
> > 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.

Already done in the second version of the xfstests patch.

> 
> > @@ -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.
> 

Thanks!
-Lukas

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