xfs
[Top] [All Lists]

Re: [PATCH 2/3] metadump: Only verify obfuscated metadata being dumped

To: Dave Chinner <david@xxxxxxxxxxxxx>, xfs@xxxxxxxxxxx
Subject: Re: [PATCH 2/3] metadump: Only verify obfuscated metadata being dumped
From: Eric Sandeen <sandeen@xxxxxxxxxxx>
Date: Fri, 28 Feb 2014 11:58:32 -0600
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <1393568723-982-3-git-send-email-david@xxxxxxxxxxxxx>
References: <1393568723-982-1-git-send-email-david@xxxxxxxxxxxxx> <1393568723-982-3-git-send-email-david@xxxxxxxxxxxxx>
User-agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:24.0) Gecko/20100101 Thunderbird/24.3.0
On 2/28/14, 12:25 AM, Dave Chinner wrote:
> From: Dave Chinner <dchinner@xxxxxxxxxx>
> 
> The discontiguous buffer support series added a verifier check on
> the metadata buffers before they go written to the metadump image.
> If this failed, it returned an error, and the result would be that
> we stopped processing the metadata and exited, truncating the dump.
> 
> xfs_metadump is supposed to dump the metadata in the filesystem for
> forensic analysis purposes, which means we actually want it to
> retain any corruptions it finds in the filesystem. Hence running the
> verifier - even to recalculate CRCs - when the metadata is
> unmodified is the wrong thing to be doing. And stopping the dump
> when we come across an error is even worse.
> 
> We still need to do CRC recalculation when obfuscating names and
> attributes. Hence we need to make running the verifier conditional
> on the buffer or inode:
>       a) being uncorrupted when read, and
>       b) modified by the obfuscation code.
> 
> If either of these conditions is not true, then we don't run the
> verifier or recalculate the CRCs.

Lord how I hate "if (!dont_obfuscate)" but that's not your fault.  ;)

Reviewed-by: Eric Sandeen <sandeen@xxxxxxxxxx>

> Signed-off-by: Dave Chinner <dchinner@xxxxxxxxxx>
> ---
>  db/io.h       |  1 +
>  db/metadump.c | 61 
> ++++++++++++++++++++++++++++++++++++++++++++++-------------
>  2 files changed, 49 insertions(+), 13 deletions(-)
> 
> diff --git a/db/io.h b/db/io.h
> index 4f24c83..d8cf383 100644
> --- a/db/io.h
> +++ b/db/io.h
> @@ -41,6 +41,7 @@ typedef struct iocur {
>       int                     ino_crc_ok:1;
>       int                     ino_buf:1;
>       int                     dquot_buf:1;
> +     int                     need_crc:1;
>  } iocur_t;
>  
>  #define DB_RING_ADD 1                   /* add to ring on set_cur */
> diff --git a/db/metadump.c b/db/metadump.c
> index 14902a7..3248009 100644
> --- a/db/metadump.c
> +++ b/db/metadump.c
> @@ -190,26 +190,36 @@ write_buf_segment(
>       return 0;
>  }
>  
> +/*
> + * we want to preserve the state of the metadata in the dump - whether it is
> + * intact or corrupt, so even if the buffer has a verifier attached to it we
> + * don't want to run it prior to writing the buffer to the metadump image.
> + *
> + * The only reason for running the verifier is to recalculate the CRCs on a
> + * buffer that has been obfuscated. i.e. a buffer than metadump modified 
> itself.
> + * In this case, we only run the verifier if the buffer was not corrupt to 
> begin
> + * with so that we don't accidentally correct buffers with CRC or errors in 
> them
> + * when we are obfuscating them.
> + */
>  static int
>  write_buf(
>       iocur_t         *buf)
>  {
> +     struct xfs_buf  *bp = buf->bp;
>       int             i;
>       int             ret;
>  
>       /*
>        * Run the write verifier to recalculate the buffer CRCs and check
> -      * we are writing something valid to disk
> +      * metadump didn't introduce a new corruption. Warn if the verifier
> +      * failed, but still continue to dump it into the output file.
>        */
> -     if (buf->bp && buf->bp->b_ops) {
> -             buf->bp->b_error = 0;
> -             buf->bp->b_ops->verify_write(buf->bp);
> -             if (buf->bp->b_error) {
> -                     fprintf(stderr,
> -     _("%s: write verifer failed on bno 0x%llx/0x%x\n"),
> -                             __func__, (long long)buf->bp->b_bn,
> -                             buf->bp->b_bcount);
> -                     return -buf->bp->b_error;
> +     if (buf->need_crc && bp && bp->b_ops && !bp->b_error) {
> +             bp->b_ops->verify_write(bp);
> +             if (bp->b_error) {
> +                     print_warning(
> +                             "obfuscation corrupted block at bno 
> 0x%llx/0x%x",
> +                             (long long)bp->b_bn, bp->b_bcount);
>               }
>       }
>  
> @@ -1359,12 +1369,15 @@ process_single_fsb_objects(
>  
>                       obfuscate_dir_data_block(dp, o,
>                                                last == mp->m_dirblkfsbs);
> +                     iocur_top->need_crc = 1;
>                       break;
>               case TYP_SYMLINK:
>                       obfuscate_symlink_block(dp);
> +                     iocur_top->need_crc = 1;
>                       break;
>               case TYP_ATTR:
>                       obfuscate_attr_block(dp, o);
> +                     iocur_top->need_crc = 1;
>                       break;
>               default:
>                       break;
> @@ -1444,6 +1457,7 @@ process_multi_fsb_objects(
>  
>                       obfuscate_dir_data_block(iocur_top->data, o,
>                                                 last == mp->m_dirblkfsbs);
> +                     iocur_top->need_crc = 1;
>                       ret = write_buf(iocur_top);
>  out_pop:
>                       pop_cur();
> @@ -1724,6 +1738,13 @@ process_inode_data(
>       return 1;
>  }
>  
> +/*
> + * when we process the inode, we may change the data in the data and/or
> + * attribute fork if they are in short form and we are obfuscating names.
> + * In this case we need to recalculate the CRC of the inode, but we should
> + * only do that if the CRC in the inode is good to begin with. If the crc
> + * is not ok, we just leave it alone.
> + */
>  static int
>  process_inode(
>       xfs_agnumber_t          agno,
> @@ -1731,17 +1752,30 @@ process_inode(
>       xfs_dinode_t            *dip)
>  {
>       int                     success;
> +     bool                    crc_was_ok = false; /* no recalc by default */
> +     bool                    need_new_crc = false;
>  
>       success = 1;
>       cur_ino = XFS_AGINO_TO_INO(mp, agno, agino);
>  
> +     /* we only care about crc recalculation if we are obfuscating names. */
> +     if (!dont_obfuscate) {
> +             crc_was_ok = xfs_verify_cksum((char *)dip,
> +                                     mp->m_sb.sb_inodesize,
> +                                     offsetof(struct xfs_dinode, di_crc));
> +     }
> +
>       /* copy appropriate data fork metadata */
>       switch (be16_to_cpu(dip->di_mode) & S_IFMT) {
>               case S_IFDIR:
>                       success = process_inode_data(dip, TYP_DIR2);
> +                     if (dip->di_format == XFS_DINODE_FMT_LOCAL)
> +                             need_new_crc = 1;
>                       break;
>               case S_IFLNK:
>                       success = process_inode_data(dip, TYP_SYMLINK);
> +                     if (dip->di_format == XFS_DINODE_FMT_LOCAL)
> +                             need_new_crc = 1;
>                       break;
>               case S_IFREG:
>                       success = process_inode_data(dip, TYP_DATA);
> @@ -1756,6 +1790,7 @@ process_inode(
>               attr_data.remote_val_count = 0;
>               switch (dip->di_aformat) {
>                       case XFS_DINODE_FMT_LOCAL:
> +                             need_new_crc = 1;
>                               if (!dont_obfuscate)
>                                       obfuscate_sf_attr(dip);
>                               break;
> @@ -1770,6 +1805,9 @@ process_inode(
>               }
>               nametable_clear();
>       }
> +
> +     if (crc_was_ok && need_new_crc)
> +             xfs_dinode_calc_crc(mp, dip);
>       return success;
>  }
>  
> @@ -1840,9 +1878,6 @@ copy_inode_chunk(
>  
>               if (!process_inode(agno, agino + i, dip))
>                       goto pop_out;
> -
> -             /* calculate the new CRC for the inode */
> -             xfs_dinode_calc_crc(mp, dip);
>       }
>  skip_processing:
>       if (write_buf(iocur_top))
> 

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