Re: [PATCH] xfsdump: enable dump header checksums

[Bill Kendall <wkendall@xxxxxxx>]

On 09/02/2011 09:51 AM, Gim Leong Chin wrote:
> Hi Bill,
>
>
> May I understand:
>
> 1) Are the check sums written in the dump file?
> 2) Which fields would have the check sums?

The headers that precede directory entries, files (inodes),
extents (user data) and extended attributes all have checksum
fields. They are stored in the dump stream and the stream has
flags indicating whether or not the checksums were written so
that restore knows if it should check them.

> 3) If the checksums at restore time do not agree with the fields, is there any kind of recovery action possible?

Possibly, though that's not part of this patch. For example if
a file header was corrupt, it may be possible to skip forward
until the next valid file header is found and continue the
restore from there.

Thanks,
Bill

> Thanks!
>
>
> GL
>
> --- On Tue, 30/8/11, Bill Kendall<wkendall@xxxxxxx>  wrote:
>
> > From: Bill Kendall<wkendall@xxxxxxx>
> > Subject: [PATCH] xfsdump: enable dump header checksums
> > To: xfs@xxxxxxxxxxx
> > Date: Tuesday, 30 August, 2011, 5:41 AM
> > Various structures in a dump file
> > optionally contain a checksum, but
> > the code to compute and validate the checksum has not been
> > enabled.
> > The checksum code has a negligible performance impact and
> > so this
> > patch enables the checksum code unconditionally. Also:
> >
> > - make sure all header sizes are multiples of 4 bytes
> >    (a requirement of the checksum routine)
> > - zero structures to ensure internal padding has a known
> > value
> > - fix a bug in dump_extattr_buildrecord() which
> > checksummed
> >    the wrong header structure
> > - add calc_checksum() and is_checksum_valid() routines to
> >    cut down on duplicate code
> >
> > Signed-off-by: Bill Kendall<wkendall@xxxxxxx>
> > ---
> >   common/content_inode.h |   20 +++++++++++
> >   dump/content.c
> >     |   85
> > +++++++++++-------------------------------------
> >   restore/Makefile       |
> >    2 +-
> >   restore/content.c
> > |   40 ++--------------------
> >   4 files changed, 44 insertions(+), 103 deletions(-)
> >
> > diff --git a/common/content_inode.h
> > b/common/content_inode.h
> > index 479fdfc..e119354 100644
> > --- a/common/content_inode.h
> > +++ b/common/content_inode.h
> > @@ -347,4 +347,24 @@ typedef struct extattrhdr
> > extattrhdr_t;
> >       /* a linux "secure" mode attribute
> >        */
> >
> > +/* Routines for calculating and validating checksums on
> > xfsdump headers
> > + */
> > +static inline u_int32_t
> > +calc_checksum(void *startp, void *endp)
> > +{
> > +    u_int32_t sum;
> > +    u_int32_t *sump = (u_int32_t *)startp;
> > +    for (sum = 0; sump<  (u_int32_t
> > *)endp; sum += *sump++);
> > +    return ~sum + 1;
> > +}
> > +
> > +static inline bool_t
> > +is_checksum_valid(void *startp, void *endp)
> > +{
> > +    u_int32_t sum;
> > +    u_int32_t *sump = (u_int32_t *)startp;
> > +    for (sum = 0; sump<  (u_int32_t
> > *)endp; sum += *sump++);
> > +    return sum == 0 ? BOOL_TRUE :
> > BOOL_FALSE;
> > +}
> > +
> >   #endif /* CONTENT_INODE_H */
> > diff --git a/dump/content.c b/dump/content.c
> > index 9905c88..2cf15ba 100644
> > --- a/dump/content.c
> > +++ b/dump/content.c
> > @@ -585,6 +585,13 @@ content_init( intgen_t argc,
> >           sizeof(
> > content_inode_hdr_t ));
> >       ASSERT( sizeof( extattrhdr_t ) ==
> > EXTATTRHDR_SZ );
> >
> > +    /* must be a multiple of 32-bits for
> > checksums
> > +     */
> > +    ASSERT( FILEHDR_SZ % sizeof( u_int32_t
> > ) == 0 );
> > +    ASSERT( EXTENTHDR_SZ  % sizeof(
> > u_int32_t ) == 0 );
> > +    ASSERT( DIRENTHDR_SZ % sizeof(
> > u_int32_t ) == 0 );
> > +    ASSERT( EXTATTRHDR_SZ % sizeof(
> > u_int32_t ) == 0 );
> > +
> >       /* calculate offsets of portions of the
> > write hdr template
> >        */
> >       dwhdrtemplatep = ( drive_hdr_t *
> > )gwhdrtemplatep->gh_upper;
> > @@ -1491,8 +1498,7 @@ baseuuidbypass:
> >       var_skip(&fsid, inomap_skip );
> >
> >       /* fill in write header template
> > content info. always produce
> > -     * an inomap and dir dump for
> > each media file. flag the checksums
> > -     * available if so compiled
> > (see -D...CHECKSUM in Makefile).
> > +     * an inomap and dir dump for
> > each media file.
> >        */
> >       ASSERT( sizeof(
> > cwhdrtemplatep->ch_specific )>= sizeof(
> > *scwhdrtemplatep ));
> >       scwhdrtemplatep->cih_mediafiletype =
> > CIH_MEDIAFILETYPE_DATA;
> > @@ -1506,15 +1512,9 @@ baseuuidbypass:
> >       if ( sc_inv_updatepr ) {
> >
> > scwhdrtemplatep->cih_dumpattr |= CIH_DUMPATTR_INVENTORY;
> >       }
> > -#ifdef FILEHDR_CHECKSUM
> >       scwhdrtemplatep->cih_dumpattr |=
> > CIH_DUMPATTR_FILEHDR_CHECKSUM;
> > -#endif /* FILEHDR_CHECKSUM */
> > -#ifdef EXTENTHDR_CHECKSUM
> >       scwhdrtemplatep->cih_dumpattr |=
> > CIH_DUMPATTR_EXTENTHDR_CHECKSUM;
> > -#endif /* EXTENTHDR_CHECKSUM */
> > -#ifdef DIRENTHDR_CHECKSUM
> >       scwhdrtemplatep->cih_dumpattr |=
> > CIH_DUMPATTR_DIRENTHDR_CHECKSUM;
> > -#endif /* DIRENTHDR_CHECKSUM */
> >       scwhdrtemplatep->cih_dumpattr |=
> > CIH_DUMPATTR_DIRENTHDR_GEN;
> >       if ( sc_incrpr ) {
> >
> > scwhdrtemplatep->cih_dumpattr |=
> > CIH_DUMPATTR_INCREMENTAL;
> > @@ -1528,10 +1528,8 @@ baseuuidbypass:
> >       }
> >       if ( sc_dumpextattrpr ) {
> >
> > scwhdrtemplatep->cih_dumpattr |= CIH_DUMPATTR_EXTATTR;
> > -#ifdef EXTATTRHDR_CHECKSUM
> >
> > scwhdrtemplatep->cih_dumpattr |=
> >
> >
> > CIH_DUMPATTR_EXTATTRHDR_CHECKSUM;
> > -#endif /* EXTATTRHDR_CHECKSUM */
> >       }
> >
> >       scwhdrtemplatep->cih_rootino =
> > sc_rootxfsstatp->bs_ino;
> > @@ -3743,6 +3741,8 @@ dump_extattr_buildrecord( xfs_bstat_t
> > *statp,
> >
> >     namesz, namesrcp,
> >
> >     valuesz );
> >       ( void )strcpy( namep, namesrcp );
> > +
> > +    memset( ( void * )&tmpah, 0,
> > sizeof( tmpah ));
> >       tmpah.ah_sz = recsz;
> >       ASSERT( EXTATTRHDR_SZ + namesz<
> > UINT16MAX );
> >       tmpah.ah_valoff = ( u_int16_t )(
> > EXTATTRHDR_SZ + namesz );
> > @@ -3750,17 +3750,8 @@ dump_extattr_buildrecord(
> > xfs_bstat_t *statp,
> >           (( flag&
> > ATTR_ROOT ) ? EXTATTRHDR_FLAGS_ROOT :
> >           (( flag&
> > ATTR_SECURE ) ? EXTATTRHDR_FLAGS_SECURE : 0));
> >       tmpah.ah_valsz = valuesz;
> > -    tmpah.ah_checksum = 0;
> > -#ifdef EXTATTRHDR_CHECKSUM
> > -    {
> > -    register u_int32_t *sump = ( u_int32_t
> > * )ahdrp;
> > -    register u_int32_t *endp = ( u_int32_t
> > * )( ahdrp + 1 );
> > -    register u_int32_t sum;
> >       tmpah.ah_flags |=
> > EXTATTRHDR_FLAGS_CHECKSUM;
> > -    for ( sum = 0 ; sump<  endp ; sum +=
> > *sump++ ) ;
> > -    tmpah.ah_checksum = ~sum + 1;
> > -    }
> > -#endif /* EXTATTRHDR_CHECKSUM */
> > +    tmpah.ah_checksum = calc_checksum(
> > &tmpah,&tmpah + 1 );
> >
> >       xlate_extattrhdr(ahdrp,&tmpah,
> > -1);
> >       *valuepp = valuep;
> > @@ -3782,23 +3773,13 @@ dump_extattrhdr( drive_t *drivep,
> >       intgen_t rval;
> >       rv_t rv;
> >
> > +    memset( ( void * )&ahdr, 0, sizeof(
> > ahdr ));
> >       ahdr.ah_sz = recsz;
> >       ASSERT( valoff<  UINT16MAX );
> >       ahdr.ah_valoff = ( u_int16_t )valoff;
> > -    ahdr.ah_flags = ( u_int16_t )flags;
> > +    ahdr.ah_flags = ( u_int16_t )flags |
> > EXTATTRHDR_FLAGS_CHECKSUM;
> >       ahdr.ah_valsz = valsz;
> > -    ahdr.ah_checksum = 0;
> > -
> > -#ifdef EXTATTRHDR_CHECKSUM
> > -    {
> > -    register u_int32_t *sump = ( u_int32_t
> > * )&ahdr;
> > -    register u_int32_t *endp = ( u_int32_t
> > * )(&ahdr + 1 );
> > -    register u_int32_t sum;
> > -    ahdr.ah_flags |=
> > EXTATTRHDR_FLAGS_CHECKSUM;
> > -    for ( sum = 0 ; sump<  endp ; sum +=
> > *sump++ ) ;
> > -    ahdr.ah_checksum = ~sum + 1;
> > -    }
> > -#endif /* EXTATTRHDR_CHECKSUM */
> > +    ahdr.ah_checksum = calc_checksum(
> > &ahdr,&ahdr + 1 );
> >
> >       xlate_extattrhdr(&ahdr,
> > &tmpahdr, 1);
> >       rval = write_buf( ( char *
> > )&tmpahdr,
> > @@ -5102,11 +5083,6 @@ dump_filehdr( drive_t *drivep,
> >       drive_ops_t *dop = drivep->d_opsp;
> >       register filehdr_t *fhdrp =
> > contextp->cc_filehdrp;
> >       filehdr_t tmpfhdrp;
> > -#ifdef FILEHDR_CHECKSUM
> > -    register u_int32_t *sump = ( u_int32_t
> > * )fhdrp;
> > -    register u_int32_t *endp = ( u_int32_t
> > * )( fhdrp + 1 );
> > -    register u_int32_t sum;
> > -#endif /* FILEHDR_CHECKSUM */
> >       intgen_t rval;
> >       rv_t rv;
> >
> > @@ -5118,13 +5094,8 @@ dump_filehdr( drive_t *drivep,
> >
> > copy_xfs_bstat(&fhdrp->fh_stat, statp);
> >       }
> >       fhdrp->fh_offset = offset;
> > -    fhdrp->fh_flags = flags;
> > -
> > -#ifdef FILEHDR_CHECKSUM
> > -    fhdrp->fh_flags |=
> > FILEHDR_FLAGS_CHECKSUM;
> > -    for ( sum = 0 ; sump<  endp ; sum +=
> > *sump++ ) ;
> > -    fhdrp->fh_checksum = ~sum + 1;
> > -#endif /* FILEHDR_CHECKSUM */
> > +    fhdrp->fh_flags = flags |
> > FILEHDR_FLAGS_CHECKSUM;
> > +    fhdrp->fh_checksum = calc_checksum(
> > fhdrp, fhdrp + 1 );
> >
> >       xlate_filehdr(fhdrp,&tmpfhdrp,
> > 1);
> >       rval = write_buf( ( char *
> > )&tmpfhdrp,
> > @@ -5164,11 +5135,6 @@ dump_extenthdr( drive_t *drivep,
> >       drive_ops_t *dop = drivep->d_opsp;
> >       register extenthdr_t *ehdrp =
> > contextp->cc_extenthdrp;
> >       extenthdr_t tmpehdrp;
> > -#ifdef EXTENTHDR_CHECKSUM
> > -    register u_int32_t *sump = ( u_int32_t
> > * )ehdrp;
> > -    register u_int32_t *endp = ( u_int32_t
> > * )( ehdrp + 1 );
> > -    register u_int32_t sum;
> > -#endif /* EXTENTHDR_CHECKSUM */
> >       intgen_t rval;
> >       rv_t rv;
> >       char typestr[20];
> > @@ -5198,15 +5164,10 @@ dump_extenthdr( drive_t *drivep,
> >
> >       ( void )memset( ( void * )ehdrp, 0,
> > sizeof( *ehdrp ));
> >       ehdrp->eh_type = type;
> > -    ehdrp->eh_flags = flags;
> > +    ehdrp->eh_flags = flags |
> > EXTENTHDR_FLAGS_CHECKSUM;
> >       ehdrp->eh_offset = offset;
> >       ehdrp->eh_sz = sz;
> > -
> > -#ifdef EXTENTHDR_CHECKSUM
> > -    ehdrp->eh_flags |=
> > EXTENTHDR_FLAGS_CHECKSUM;
> > -    for ( sum = 0 ; sump<  endp ; sum +=
> > *sump++ ) ;
> > -    ehdrp->eh_checksum = ~sum + 1;
> > -#endif /* EXTENTHDR_CHECKSUM */
> > +    ehdrp->eh_checksum = calc_checksum(
> > ehdrp, ehdrp + 1 );
> >
> >       xlate_extenthdr(ehdrp,&tmpehdrp,
> > 1);
> >       rval = write_buf( ( char *
> > )&tmpehdrp,
> > @@ -5249,11 +5210,6 @@ dump_dirent( drive_t *drivep,
> >       direnthdr_t *tmpdhdrp;
> >       size_t direntbufsz =
> > contextp->cc_mdirentbufsz;
> >       size_t sz;
> > -#ifdef DIRENTHDR_CHECKSUM
> > -    register u_int32_t *sump = ( u_int32_t
> > * )dhdrp;
> > -    register u_int32_t *endp = ( u_int32_t
> > * )( dhdrp + 1 );
> > -    register u_int32_t sum;
> > -#endif /* DIRENTHDR_CHECKSUM */
> >       intgen_t rval;
> >       rv_t rv;
> >
> > @@ -5290,10 +5246,7 @@ dump_dirent( drive_t *drivep,
> >           strcpy(
> > dhdrp->dh_name, name );
> >       }
> >
> > -#ifdef DIRENTHDR_CHECKSUM
> > -    for ( sum = 0 ; sump<  endp ; sum +=
> > *sump++ ) ;
> > -    dhdrp->dh_checksum = ~sum + 1;
> > -#endif /* DIRENTHDR_CHECKSUM */
> > +    dhdrp->dh_checksum = calc_checksum(
> > dhdrp, dhdrp + 1);
> >
> >       tmpdhdrp = malloc(sz);
> >       xlate_direnthdr(dhdrp, tmpdhdrp, 1);
> > diff --git a/restore/Makefile b/restore/Makefile
> > index 78ecc2c..588a8f0 100644
> > --- a/restore/Makefile
> > +++ b/restore/Makefile
> > @@ -103,7 +103,7 @@ LLDLIBS = $(LIBUUID) $(LIBHANDLE)
> > $(LIBATTR) $(LIBRMT)
> >   LTDEPENDENCIES = $(LIBRMT)
> >
> >   LCFLAGS = -DRESTORE -DRMT -DBASED -DDOSOCKS -DINVCONVFIX
> > -DPIPEINVFIX \
> > -    -DEOMFIX -DSESSCPLT -DWHITEPARSE
> > -DDIRENTHDR_CHECKSUM \
> > +    -DEOMFIX -DSESSCPLT -DWHITEPARSE \
> >       -DF_FSSETDM
> >
> >   default: depend $(LTCOMMAND)
> > diff --git a/restore/content.c b/restore/content.c
> > index e3e4994..25849d7 100644
> > --- a/restore/content.c
> > +++ b/restore/content.c
> > @@ -8000,11 +8000,6 @@ read_filehdr( drive_t *drivep,
> > filehdr_t *fhdrp, bool_t fhcs )
> >       drive_ops_t *dop = drivep->d_opsp;
> >       /* REFERENCED */
> >       intgen_t nread;
> > -#ifdef FILEHDR_CHECKSUM
> > -    register u_int32_t *sump = ( u_int32_t
> > * )fhdrp;
> > -    register u_int32_t *endp = ( u_int32_t
> > * )( fhdrp + 1 );
> > -    register u_int32_t sum;
> > -#endif /* FILEHDR_CHECKSUM */
> >       intgen_t rval;
> >       filehdr_t tmpfh;
> >
> > @@ -8041,21 +8036,18 @@ read_filehdr( drive_t *drivep,
> > filehdr_t *fhdrp, bool_t fhcs )
> >
> > bstatp->bs_ino,
> >             bstatp->bs_mode
> > );
> >
> > -#ifdef FILEHDR_CHECKSUM
> >       if ( fhcs ) {
> >           if ( ! (
> > fhdrp->fh_flags&  FILEHDR_FLAGS_CHECKSUM )) {
> >
> > mlog( MLOG_NORMAL | MLOG_WARNING, _(
> >
> >        "corrupt file header\n") );
> >
> > return RV_CORRUPT;
> >           }
> > -        for ( sum = 0 ; sump
> > <  endp ; sum += *sump++ ) ;
> > -        if ( sum ) {
> > +        if (
> > !is_checksum_valid( fhdrp, fhdrp + 1)) {
> >
> > mlog( MLOG_NORMAL | MLOG_WARNING, _(
> >
> >        "bad file header checksum\n") );
> >
> > return RV_CORRUPT;
> >           }
> >       }
> > -#endif /* FILEHDR_CHECKSUM */
> >
> >       return RV_OK;
> >   }
> > @@ -8067,11 +8059,6 @@ read_extenthdr( drive_t *drivep,
> > extenthdr_t *ehdrp, bool_t ehcs )
> >       drive_ops_t *dop = drivep->d_opsp;
> >       /* REFERENCED */
> >       intgen_t nread;
> > -#ifdef EXTENTHDR_CHECKSUM
> > -    register u_int32_t *sump = ( u_int32_t
> > * )ehdrp;
> > -    register u_int32_t *endp = ( u_int32_t
> > * )( ehdrp + 1 );
> > -    register u_int32_t sum;
> > -#endif /* EXTENTHDR_CHECKSUM */
> >       intgen_t rval;
> >       extenthdr_t tmpeh;
> >
> > @@ -8108,21 +8095,18 @@ read_extenthdr( drive_t *drivep,
> > extenthdr_t *ehdrp, bool_t ehcs )
> >
> > ehdrp->eh_type,
> >             ehdrp->eh_flags
> > );
> >
> > -#ifdef EXTENTHDR_CHECKSUM
> >       if ( ehcs ) {
> >           if ( ! (
> > ehdrp->eh_flags&  EXTENTHDR_FLAGS_CHECKSUM )) {
> >
> > mlog( MLOG_NORMAL | MLOG_WARNING, _(
> >
> >        "corrupt extent header\n") );
> >
> > return RV_CORRUPT;
> >           }
> > -        for ( sum = 0 ; sump
> > <  endp ; sum += *sump++ ) ;
> > -        if ( sum ) {
> > +        if (
> > !is_checksum_valid( ehdrp, ehdrp + 1)) {
> >
> > mlog( MLOG_NORMAL | MLOG_WARNING, _(
> >
> >        "bad extent header checksum\n") );
> >
> > return RV_CORRUPT;
> >           }
> >       }
> > -#endif /* EXTENTHDR_CHECKSUM */
> >
> >       return RV_OK;
> >   }
> > @@ -8137,11 +8121,6 @@ read_dirent( drive_t *drivep,
> >       drive_ops_t *dop = drivep->d_opsp;
> >       /* REFERENCED */
> >       intgen_t nread;
> > -#ifdef DIRENTHDR_CHECKSUM
> > -    register u_int32_t *sump = ( u_int32_t
> > * )dhdrp;
> > -    register u_int32_t *endp = ( u_int32_t
> > * )( dhdrp + 1 );
> > -    register u_int32_t sum;
> > -#endif /* DIRENTHDR_CHECKSUM */
> >       intgen_t rval;
> >       direnthdr_t tmpdh;
> >
> > @@ -8180,21 +8159,18 @@ read_dirent( drive_t *drivep,
> >             ( size_t
> > )dhdrp->dh_gen,
> >             ( size_t
> > )dhdrp->dh_sz );
> >
> > -#ifdef DIRENTHDR_CHECKSUM
> >       if ( dhcs ) {
> >           if ( dhdrp->dh_sz
> > == 0 ) {
> >
> > mlog( MLOG_NORMAL | MLOG_WARNING, _(
> >
> >        "corrupt directory entry header\n") );
> >
> > return RV_CORRUPT;
> >           }
> > -        for ( sum = 0 ; sump
> > <  endp ; sum += *sump++ ) ;
> > -        if ( sum ) {
> > +        if (
> > !is_checksum_valid( dhdrp, dhdrp + 1)) {
> >
> > mlog( MLOG_NORMAL | MLOG_WARNING, _(
> >
> >        "bad directory entry header
> > checksum\n") );
> >
> > return RV_CORRUPT;
> >           }
> >       }
> > -#endif /* DIRENTHDR_CHECKSUM */
> >
> >       /* if null, return
> >        */
> > @@ -8246,11 +8222,6 @@ read_extattrhdr( drive_t *drivep,
> > extattrhdr_t *ahdrp, bool_t ahcs )
> >       drive_ops_t *dop = drivep->d_opsp;
> >       /* REFERENCED */
> >       intgen_t nread;
> > -#ifdef EXTATTRHDR_CHECKSUM
> > -    register u_int32_t *sump = ( u_int32_t
> > * )ahdrp;
> > -    register u_int32_t *endp = ( u_int32_t
> > * )( ahdrp + 1 );
> > -    register u_int32_t sum;
> > -#endif /* EXTATTRHDR_CHECKSUM */
> >       intgen_t rval;
> >       extattrhdr_t tmpah;
> >
> > @@ -8288,21 +8259,18 @@ read_extattrhdr( drive_t *drivep,
> > extattrhdr_t *ahdrp, bool_t ahcs )
> >
> > ahdrp->ah_valsz,
> >
> > ahdrp->ah_checksum );
> >
> > -#ifdef EXTATTRHDR_CHECKSUM
> >       if ( ahcs ) {
> >           if ( ! (
> > ahdrp->ah_flags&  EXTATTRHDR_FLAGS_CHECKSUM )) {
> >
> > mlog( MLOG_NORMAL | MLOG_WARNING, _(
> >
> >        "corrupt extattr header\n") );
> >
> > return RV_CORRUPT;
> >           }
> > -        for ( sum = 0 ; sump
> > <  endp ; sum += *sump++ ) ;
> > -        if ( sum ) {
> > +        if (
> > !is_checksum_valid( ahdrp, ahdrp + 1)) {
> >
> > mlog( MLOG_NORMAL | MLOG_WARNING, _(
> >
> >        "bad extattr header checksum\n") );
> >
> > return RV_CORRUPT;
> >           }
> >       }
> > -#endif /* EXTATTRHDR_CHECKSUM */
> >
> >       return RV_OK;
> >   }
> > --
> > 1.7.0.4
> >
> > _______________________________________________
> > xfs mailing list
> > xfs@xxxxxxxxxxx
> > http://oss.sgi.com/mailman/listinfo/xfs

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