xfs
[Top] [All Lists]

Re: [PATCH v3 3/5] xfs: fix segment in xfs_buf_item_format_segment

To: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Subject: Re: [PATCH v3 3/5] xfs: fix segment in xfs_buf_item_format_segment
From: Dave Chinner <david@xxxxxxxxxxxxx>
Date: Mon, 10 Dec 2012 12:34:06 +1100
Cc: Mark Tinguely <tinguely@xxxxxxx>, xfs@xxxxxxxxxxx
In-reply-to: <20121208122921.GC29107@xxxxxxxxxxxxx>
References: <20121204231801.982422341@xxxxxxx> <20121204224938.820985192@xxxxxxx> <20121208122921.GC29107@xxxxxxxxxxxxx>
User-agent: Mutt/1.5.21 (2010-09-15)
On Sat, Dec 08, 2012 at 07:29:21AM -0500, Christoph Hellwig wrote:
> On Tue, Dec 04, 2012 at 05:18:04PM -0600, Mark Tinguely wrote:
> > Not every segment in a multi-segment buffer is dirty in a
> > transaction and they will not be outputted. The assert in
> > xfs_buf_item_format_segment() that checks for the at least
> > one chunk of data in the segment to be used is not necessary
> > true for multi-segmented buffers.
> > 
> > Signed-off-by: Mark Tinguely <tinguely@xxxxxxx>
> > ---
> >  fs/xfs/xfs_buf_item.c |   20 +++++++++++++++-----
> >  1 file changed, 15 insertions(+), 5 deletions(-)
> > 
> > Index: b/fs/xfs/xfs_buf_item.c
> > ===================================================================
> > --- a/fs/xfs/xfs_buf_item.c
> > +++ b/fs/xfs/xfs_buf_item.c
> > @@ -287,6 +287,17 @@ xfs_buf_item_format_segment(
> >      */
> >     base_size = offsetof(struct xfs_buf_log_format, blf_data_map) +
> >                     (blfp->blf_map_size * sizeof(blfp->blf_data_map[0]));
> > +
> > +   nvecs = 0;
> > +   first_bit = xfs_next_bit(blfp->blf_data_map, blfp->blf_map_size, 0);
> > +   if (!(bip->bli_flags & XFS_BLI_STALE) && first_bit == -1) {
> > +           /*
> > +            * If the map is not be dirty in the transaction, mark
> > +            * the size as zero and do not advance the vector pointer.
> > +            */
> > +           goto out;
> > +   }
> > +
> 
> I don't really like the style of this check.  What's the problem of
> doing it this way:
> 
>   1) fill out the first vecp
>   2) do the stale check as-is
>   3) handle the the first_bit == -1 case  ala:
> 
>       if (first_bit == -1) {
>               blfp->blf_size = 0;
>               return vecp;
>       }

Because if we have already written all the dirty regions and we have
a trailing clean buffer segment, vecp points past the memory
allocated for the vectors. Hence we can end up with memory
corruption if we write to vecp in this case.

Cheers,

Dave.
-- 
Dave Chinner
david@xxxxxxxxxxxxx

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