Re: [PATCH] libxfs: don't write uninitialized heap contents into new directory blocks

["Darrick J. Wong" <darrick.wong@oracle.com>]

On Thu, Mar 19, 2015 at 02:28:34PM -0400, Brian Foster wrote:
> On Wed, Mar 18, 2015 at 04:21:59PM -0700, Darrick J. Wong wrote:
> > When we're initializing a new directory block, zero the buffer
> > contents to avoid writing random heap contents (and crc thereof)
> > to disk.  This eliminates a few valgrind complaints in xfs_repair.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> 
> Well it seems Ok to me:
> 
> Reviewed-by: Brian Foster <bfoster@redhat.com>
> 
> ... but I do notice that we start ending up with multiple memset() calls
> throughout these codepaths. E.g., xfs_dir3_data_init() memsets the
> target bytes for the header if crc is enabled, xfs_dir3_leaf_get_buf()
> calls xfs_dir3_leaf_init() which does something similar, etc.
> 
> I wonder if we should just zero the buffer content unconditionally on
> allocation?

I thought about that too -- certainly we could zero b_addr as soon as it's
allocated in __initbuf, which would shut up Valgrind.  I think this is what
Dave was getting at when he suggested __libxfs_getbufr(), even though that
function only frees b_addr if it touches it at all.  On the other hand, the
next thing could happen is that the buffer is read in from disk, which makes
the zeroing unnecessary.  The readbuf functions call the getbuf functions,
which makes it difficult to tell from the getbuf functions what's going to
happen next.

However, I think there's a second problem: what if the xfsbuf is reused without
freeing the b_addr?  This seems possible if we read in the block, detach it
from whatever points to it, and then we want to allocate it to a directory.
The buffer's still in memory and it's still the right size, so we don't free
b_addr; the init function doesn't overwrite the whole buffer, and now we've
just leaked old disk contents.  Adding a memset to getbuf would fix this, but
again at a cost of unnecessary zeroing.

Sprinkling memsets around the init code is a fair amount of work, but it solves
the previous problem without the unnecessary memsets.

--D

> 
> Brian
> 
> >  libxfs/xfs_da_btree.c  |    1 +
> >  libxfs/xfs_dir2_data.c |    1 +
> >  libxfs/xfs_dir2_leaf.c |    1 +
> >  libxfs/xfs_dir2_node.c |    2 +-
> >  4 files changed, 4 insertions(+), 1 deletion(-)
> > 
> > diff --git a/libxfs/xfs_da_btree.c b/libxfs/xfs_da_btree.c
> > index b731b54..bc73cbc 100644
> > --- a/libxfs/xfs_da_btree.c
> > +++ b/libxfs/xfs_da_btree.c
> > @@ -562,6 +562,7 @@ xfs_da3_root_split(
> >  	if (error)
> >  		return error;
> >  	node = bp->b_addr;
> > +	memset(bp->b_addr, 0, bp->b_bcount);
> >  	oldroot = blk1->bp->b_addr;
> >  	if (oldroot->hdr.info.magic == cpu_to_be16(XFS_DA_NODE_MAGIC) ||
> >  	    oldroot->hdr.info.magic == cpu_to_be16(XFS_DA3_NODE_MAGIC)) {
> > diff --git a/libxfs/xfs_dir2_data.c b/libxfs/xfs_dir2_data.c
> > index dc9df4d..79c2fc2 100644
> > --- a/libxfs/xfs_dir2_data.c
> > +++ b/libxfs/xfs_dir2_data.c
> > @@ -559,6 +559,7 @@ xfs_dir3_data_init(
> >  		XFS_DATA_FORK);
> >  	if (error)
> >  		return error;
> > +	memset(bp->b_addr, 0, bp->b_bcount);
> >  	bp->b_ops = &xfs_dir3_data_buf_ops;
> >  	xfs_trans_buf_set_type(tp, bp, XFS_BLFT_DIR_DATA_BUF);
> >  
> > diff --git a/libxfs/xfs_dir2_leaf.c b/libxfs/xfs_dir2_leaf.c
> > index 8e0cbc9..6eb03fc 100644
> > --- a/libxfs/xfs_dir2_leaf.c
> > +++ b/libxfs/xfs_dir2_leaf.c
> > @@ -382,6 +382,7 @@ xfs_dir3_leaf_get_buf(
> >  	if (error)
> >  		return error;
> >  
> > +	memset(bp->b_addr, 0, bp->b_bcount);
> >  	xfs_dir3_leaf_init(mp, tp, bp, dp->i_ino, magic);
> >  	xfs_dir3_leaf_log_header(tp, bp);
> >  	if (magic == XFS_DIR2_LEAF1_MAGIC)
> > diff --git a/libxfs/xfs_dir2_node.c b/libxfs/xfs_dir2_node.c
> > index 3737e4e..3e2f04d 100644
> > --- a/libxfs/xfs_dir2_node.c
> > +++ b/libxfs/xfs_dir2_node.c
> > @@ -247,7 +247,7 @@ xfs_dir3_free_get_buf(
> >  	 * Initialize the new block to be empty, and remember
> >  	 * its first slot as our empty slot.
> >  	 */
> > -	memset(bp->b_addr, 0, sizeof(struct xfs_dir3_free_hdr));
> > +	memset(bp->b_addr, 0, bp->b_bcount);
> >  	memset(&hdr, 0, sizeof(hdr));
> >  
> >  	if (xfs_sb_version_hascrc(&mp->m_sb)) {
> > 
> > _______________________________________________
> > xfs mailing list
> > xfs@oss.sgi.com
> > http://oss.sgi.com/mailman/listinfo/xfs
> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs