Re: [PATCH] xfsrestore: fix inventory unpacking

["Darrick J. Wong" <djwong@kernel.org>]

On Wed, Sep 14, 2022 at 01:47:08PM +1000, Donald Douwsma wrote:
> When xfsrestore reads the inventory from tape media it fails to convert
> media file records from bigendin. If the xfsdump inventory is not

bigendian?

> available xfsrestore will write this invalid record to the on-line
> inventory.
> 
> [root@rhel8 xfsdump-dev]# xfsdump -I
> file system 0:
>         fs id:          26dd5aa0-b901-4cf5-9b68-0c5753cb3ab8
>         session 0:
>                 mount point:    rhel8:/boot
>                 device:         rhel8:/dev/sda1
>                 time:           Fri Sep  9 14:29:03 2022
>                 session label:  ""
>                 session id:     05f11cfe-2301-4000-89f2-2025091da413
>                 level:          0
>                 resumed:        NO
>                 subtree:        NO
>                 streams:        1
>                 stream 0:
>                         pathname:       /dev/nst0
>                         start:          ino 133 offset 0
>                         end:            ino 1572997 offset 0
>                         interrupted:    YES
>                         media files:    1
>                         media file 0:
>                                 mfile index:    33554432
>                                 mfile type:     data
>                                 mfile size:     211187836911616
>                                 mfile start:    ino 9583660007044415488 offset 0
>                                 mfile end:      ino 9583686395323482112 offset 0
>                                 media label:    ""
>                                 media id:       4bf9ed40-6377-4926-be62-1bf7b59b1619
> xfsdump: Dump Status: SUCCESS
> 
> The invalid start and end inode information cause xfsrestore to consider

What's invalid here?  I gather it's the transition from 1572997 to
9583660007044415488?

> that non-directory files do not reside in the current media and will
> fail to restore them.
> 
> The behaviour of an initial restore may succeed if the position of the
> tape is such that the data file is encountered before the inventory
> file. Subsequent restores will use the invalid on-line inventory and
> fail to restore files.
> 
> Fix this by correctly unpacking the inventory data.

Which chunk makes this happen?  I'm afraid I'm not that familiar with
xfsrestore, so I can't really spot where the endian conversion is made.
Is it that chunk where you remove the "#ifdef INVT_DELETION" (which
AFAICT is never defined anywhere) and make it so that
xlate_invt_mediafile is always called?

> Also handle multiple
> streams and untangle the logic where stobj_unpack_sessinfo is called.

This sounds like multiple patches to me -- one to fix the missing
endianness conversion, another to handle the multiple streams, and a
third to do the cleanup in pi_addfile.

> Signed-off-by: Donald Douwsma <ddouwsma@redhat.com>
> ---
>  inventory/inv_stobj.c | 38 ++++++++++++++------------------------
>  restore/content.c     | 13 +++++--------
>  2 files changed, 19 insertions(+), 32 deletions(-)
> 
> diff --git a/inventory/inv_stobj.c b/inventory/inv_stobj.c
> index c20e71c..efaf46d 100644
> --- a/inventory/inv_stobj.c
> +++ b/inventory/inv_stobj.c
> @@ -1008,7 +1008,7 @@ stobj_unpack_sessinfo(
>          size_t             bufsz,
>  	invt_sessinfo_t   *s)
>  {
> -	uint 		 i;
> +	uint 		 i, j;
>  	char	         *tmpbuf;
>  	char 		 *p = (char *)bufp;
>  
> @@ -1080,35 +1080,25 @@ stobj_unpack_sessinfo(
>  	p += sizeof(invt_session_t);
>  
>  	/* the array of all the streams belonging to this session */
> -	xlate_invt_stream((invt_stream_t *)p, (invt_stream_t *)tmpbuf, 1);
> -	bcopy(tmpbuf, p, sizeof(invt_stream_t));
>  	s->strms = (invt_stream_t *)p;
> -	p += s->ses->s_cur_nstreams * sizeof(invt_stream_t);
> +        for (i = 0; i < s->ses->s_cur_nstreams; i++) {

Indentation damage here.

> +                xlate_invt_stream((invt_stream_t *)p, 
> +				  (invt_stream_t *)tmpbuf, 1);
> +                bcopy(tmpbuf, p, sizeof(invt_stream_t));
> +                p += sizeof(invt_stream_t);
> +        }
>  
>  	/* all the media files */
>  	s->mfiles = (invt_mediafile_t *)p;
> -
> -#ifdef INVT_DELETION
> -	{
> -		int tmpfd = open("moids", O_RDWR | O_CREAT, S_IRUSR|S_IWUSR);
> -		uint j;
> -		invt_mediafile_t *mmf = s->mfiles;
> -		for (i=0; i< s->ses->s_cur_nstreams; i++) {
> -			for (j=0; j< s->strms[i].st_nmediafiles;
> -			     j++, mmf++)
> -				xlate_invt_mediafile((invt_mediafile_t *)mmf, (invt_mediafile_t *)tmpbuf, 1);
> -				bcopy(tmpbuf, mmf, sizeof(invt_mediafile_t));
> -				put_invtrecord(tmpfd, &mmf->mf_moid,
> -					 sizeof(uuid_t), 0, SEEK_END, 0);
> +	for (i=0; i< s->ses->s_cur_nstreams; i++) {
> +		for (j=0; j < s->strms[i].st_nmediafiles; j++) {
> +			xlate_invt_mediafile((invt_mediafile_t *)p, 
> +					     (invt_mediafile_t *)tmpbuf, 1);
> +			bcopy(tmpbuf, p, sizeof(invt_mediafile_t));
> +			p +=  sizeof(invt_mediafile_t);
>  		}
> -		close(tmpfd);
>  	}
> -#endif
> -	for (i = 0; i < s->ses->s_cur_nstreams; i++) {
> -		p += (size_t) (s->strms[i].st_nmediafiles)
> -			* sizeof(invt_mediafile_t);
> -	}
> -
> +	
>  	/* sanity check the size of the buffer given to us vs. the size it
>  	   should be */
>  	if ((size_t) (p - (char *) bufp) != bufsz) {
> diff --git a/restore/content.c b/restore/content.c
> index b3999f9..bbced2d 100644
> --- a/restore/content.c
> +++ b/restore/content.c
> @@ -5463,17 +5463,14 @@ pi_addfile(Media_t *Mediap,
>  			 * desc.
>  			 */
>  			sessp = 0;
> -			if (!buflen) {
> -				ok = BOOL_FALSE;
> -			} else {
> -			    /* extract the session information from the buffer */
> -			    if (stobj_unpack_sessinfo(bufp, buflen, &sessinfo)<0) {
> -				ok = BOOL_FALSE;
> -			    } else {
> +			ok = BOOL_FALSE;
> +			/* extract the session information from the buffer */
> +			if (buflen && 

There's a lot of trailing whitespace added by this patch.

Also, what is the purpose of this change?  Is there something
significant about calling stobj_convert_sessinfo even if
stobj_unpack_sessinfo returns a negative number?

*OH* that unpack function returns "bool_t", which means that we only
care about zero and nonzero.  So I think this is just a cleanup?

--D

> +			    stobj_unpack_sessinfo(bufp, buflen, &sessinfo)) {
>  				stobj_convert_sessinfo(&sessp, &sessinfo);
>  				ok = BOOL_TRUE;
> -			    }
>  			}
> +
>  			if (!ok || !sessp) {
>  				mlog(MLOG_DEBUG | MLOG_WARNING | MLOG_MEDIA, _(
>  				      "on-media session "
> -- 
> 2.31.1
>