Re: [PATCH 1/3] xfsrestore: fix inventory unpacking

[Donald Douwsma <ddouwsma@redhat.com>]

On 29/09/2022 01:12, Darrick J. Wong wrote:
> On Wed, Sep 28, 2022 at 03:53:05PM +1000, Donald Douwsma wrote:
>> When xfsrestore reads the inventory from tape media it fails to convert
>> media file records from bigendian. If the xfsdump inventory is not
>> available xfsrestore will write this invalid record to the on-line
>> inventory.
>>
>> [root@rhel8 ~]# xfsdump -L Test1 -M "" -f /dev/st0 /boot > /dev/null
>> [root@rhel8 ~]# xfsdump -L Test2 -M "" -f /dev/st0 /boot > /dev/null
>> [root@rhel8 ~]# rm -rf /var/lib/xfsdump/inventory/
>> [root@rhel8 ~]# mt -f /dev/nst0 asf 2
>> [root@rhel8 ~]# xfsrestore -f /dev/nst0 -L Test2 /tmp/test2
>> xfsrestore: using scsi tape (drive_scsitape) strategy
>> xfsrestore: version 3.1.8 (dump format 3.0) - type ^C for status and control
>> xfsrestore: searching media for dump
>> xfsrestore: preparing drive
>> xfsrestore: examining media file 3
>> xfsrestore: found dump matching specified label:
>> xfsrestore: hostname: rhel8
>> xfsrestore: mount point: /boot
>> xfsrestore: volume: /dev/sda1
>> xfsrestore: session time: Tue Sep 27 16:05:28 2022
>> xfsrestore: level: 0
>> xfsrestore: session label: "Test2"
>> xfsrestore: media label: ""
>> xfsrestore: file system id: 26dd5aa0-b901-4cf5-9b68-0c5753cb3ab8
>> xfsrestore: session id: 62402423-7ae0-49ed-8ecb-9e5bc7642b91
>> xfsrestore: media id: 47ba45ca-3417-4006-ab10-3dc6419b83e2
>> xfsrestore: incorporating on-media session inventory into online inventory
>> xfsrestore: /var/lib/xfsdump/inventory created
>> xfsrestore: using on-media session inventory
>> xfsrestore: searching media for directory dump
>> xfsrestore: rewinding
>> xfsrestore: examining media file 0
>> xfsrestore: inventory session uuid (62402423-7ae0-49ed-8ecb-9e5bc7642b91) does not match the media header's session uuid (1771d9e8-a1ba-4e87-a61e-f6be97e41b45)
>> xfsrestore: examining media file 1
>> xfsrestore: inventory session uuid (62402423-7ae0-49ed-8ecb-9e5bc7642b91) does not match the media header's session uuid (1771d9e8-a1ba-4e87-a61e-f6be97e41b45)
>> xfsrestore: examining media file 2
>> xfsrestore: reading directories
>> xfsrestore: 9 directories and 320 entries processed
>> xfsrestore: directory post-processing
>> xfsrestore: restore complete: 0 seconds elapsed
>> xfsrestore: Restore Summary:
>> xfsrestore:   stream 0 /dev/nst0 OK (success)
>> xfsrestore: Restore Status: SUCCESS
>> [root@rhel8 ~]# xfsdump -I
>> file system 0:
>>          fs id:          26dd5aa0-b901-4cf5-9b68-0c5753cb3ab8
>>          session 0:
>>                  mount point:    rhel8:/boot
>>                  device:         rhel8:/dev/sda1
>>                  time:           Tue Sep 27 16:05:28 2022
>>                  session label:  "Test2"
>>                  session id:     62402423-7ae0-49ed-8ecb-9e5bc7642b91
>>                  level:          0
>>                  resumed:        NO
>>                  subtree:        NO
>>                  streams:        1
>>                  stream 0:
>>                          pathname:       /dev/st0
>>                          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:       47ba45ca-3417-4006-ab10-3dc6419b83e2
>> xfsdump: Dump Status: SUCCESS
>> [root@rhel8 ~]#
>> [root@rhel8 ~]# ls /tmp/test2
>> efi  grub2  loader
>>
>> The invalid start and end inode information cause xfsrestore to consider
>> 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, or if there is only one dump session on tape, xfsrestore is
>> somewhat inconsistent in this regard. Subsequent restores will use the
>> invalid on-line inventory and fail to restore files.
>>
>> Fix this by correctly unpacking the inventory data.
>>
>> Signed-off-by: Donald Douwsma <ddouwsma@redhat.com>
>> ---
>>   inventory/inv_stobj.c | 27 +++++++--------------------
>>   1 file changed, 7 insertions(+), 20 deletions(-)
>>
>> diff --git a/inventory/inv_stobj.c b/inventory/inv_stobj.c
>> index c20e71c..5075ee4 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;
>>   
>> @@ -1087,26 +1087,13 @@ stobj_unpack_sessinfo(
>>   
>>   	/* 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);
> 
> I wonder, do you need to preserve this behavior (writing the inventory
> records to "moids")?  testmain.c seems to want to read the file, but
> OTOH that looks like some sort of test program; arbitrarily overwriting
> a file in $PWD seems ... risky?  And I guess this chunk has never run.
> 
> Also testmain.c has such gems as:
> 
> "/dana/hates/me"
> 
> "/the/krays"
> 
> and mentions that -I supersedes most of its functionality.  So maybe
> that's why you deleted moids?  Aside from the name just sounding gross?
> 
> :)
> 

I think they were trying to mirror what's done in stobj_pack_sessinfo(),
which takes a file handle as a parameter, possibly its meant to provide
locking while packing for the inventory. Calling put_invtrecord without
having first called get_invtrecord seems unbalanced, and the unneeded
file "mods" was just left around. Either way the target for
stobj_unpack_sessinfo is a buffer not a file.

AFIKT this was a work in progress by someone who never got to finish it.

>> -		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,
> 
> Nit: trailing whitespace                                      here ^

Urg, sorry I thought I'd fixed all the white-space problems.

> 
> If the the answer to the above question is "Yeah, nobody wants the moids
> file" then:

Yes, I think moids was a hack that shouldn't have been there.

> Reviewed-by: Darrick J. Wong <djwong@kernel.org>
> 
> --D
> 
>> +					     (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
>> -- 
>> 2.31.1
>>
>