Re: remote fileio: st_ino truncated to 32 bits in vFile:stat/lstat

[Abhay Kandpal <abhay@linux.ibm.com>]

[-- Attachment #1: Type: text/plain, Size: 7425 bytes --]

Hi Andrew,

Thanks — that all makes sense, and yes, I understand why options 1 and 2 break cross-version communication.
I'll go with the negotiated approach.

Here is the completed table.  The "Actual Size" column is sizeof() of the corresponding struct stat member:

   | Field Name  | Type        | Size | Actual |
   |             |             |  Now | Size   |
   |-------------+-------------+------+--------|
   | fst_dev     | fio_uint_t  |    4 | 8      |
   | fst_ino     | fio_uint_t  |    4 | 8      |
   | fst_mode    | fio_mode_t  |    4 | 4      |
   | fst_nlink   | fio_uint_t  |    4 | 8      |
   | fst_uid     | fio_uint_t  |    4 | 4      |
   | fst_gid     | fio_uint_t  |    4 | 4      |
   | fst_rdev    | fio_uint_t  |    4 | 8      |
   | fst_size    | fio_ulong_t |    8 | 8      |
   | fst_blksize | fio_ulong_t |    8 | 8      |
   | fst_blocks  | fio_ulong_t |    8 | 8      |
   | fst_atime   | fio_time_t  |    4 | 8      |
   | fst_mtime   | fio_time_t  |    4 | 8      |
   | fst_ctime   | fio_time_t  |    4 | 8      |

So six fields are too narrow: fst_dev, fst_ino, fst_nlink, fst_rdev and the three timestamps.
fst_size, fst_blksize and fst_blocks are already fio_ulong_t.
fst_mode, fst_uid and fst_gid are 4 bytes in struct stat as well, so those look correct as they are.

I got identical sizes on powerpc64le, powerpc64 big-endian and x86-64, and on both ext4 and xfs,
so this appears to come from glibc rather than from the architecture or filesystem.
I have not checked a 32-bit build.

Whether the truncation is visible depends on how large the filesystem's inode numbers get.
Largest inode under /home on the three machines I have:

ext4, 2 TB       132907009    passes
xfs,  2 TB      3896368632    passes
xfs,  86 TB   171801755018    fails

The second one is at about 91% of 2^32, so it passes today but would not after the filesystem grew.

I'll start on the two-struct approach with qSupported negotiation as you described,
converting at the wire boundary in both directions.

Thanks,
Abhay



On 26/08/26 22:51, Andrew Burgess wrote:
> Abhay Kandpal<abhay@linux.ibm.com> writes:
>
>> Hi Andrew,
>>
>> I hit a failure in gdb.server/fileio-packets.exp on a machine whose filesystem uses inode numbers above 2^32.
>>
>> All four stat/lstat checks fail, with every field matching except st_ino:
>>
>> remote = {..., 'st_ino': 2161233200, ...}
>> local  = {..., 'st_ino': 122420317488, ...}
>>
>> 122420317488 = 0x1C80CE9BB0
>> 2161233200  =   0x80CE9BB0
>>
>> The field is 4 bytes wide in the protocol struct, while st_ino is 64-bit on Linux:
>>
>> gdbsupport/fileio.h:130   fio_uint_t  fst_ino;
>> gdbsupport/fileio.cc:281  host_to_fileio_uint ((long) st->st_ino, fst->fst_ino);
>>
>> fst_size, fst_blksize and fst_blocks in the same struct already use the 8-byte fio_ulong_t.
>> It reproduces on xfs (86 TB, largest inode 171801755018) and passes on ext4 (2 TB, largest inode 132907009),
>> so it depends on the filesystem rather than the architecture.
>>
>> https://sourceware.org/bugzilla/show_bug.cgi?id=34567 <https://sourceware.org/bugzilla/show_bug.cgi?id=34567>
>>
>> Before writing anything I wanted to ask how you'd prefer this handled.
>>
>> struct fio_stat is shared between the vFile packets and the older F-packet protocol,
>> so widening fst_ino changes the wire layout for both.
>>
>> I can see three options:
>>
>>    1. Widen the shared field.
> I think this would not be accepted as this would break communication
> between different versions of GDB and gdbserver, right?
>
>>    2. Add a separate wider struct used only by the vFile packets,
>>       along the lines of your reasoning in c29a37f7417 about those packetsstill being new.
> I think only fixing vFile would be a mistake.  The 'F' packets are not
> an older protocol that has been replaced with vFile.  The two systems
> offer similar functionality, but in opposite directions.  If we're
> fixing one direction then we really should fix both.
>
> In c29a37f7417 I changed the underlying implementation of the 'stat'
> packet from using 'lstat' to using 'stat'.  The actual on-the-wire bits
> didn't change, just what gdbserver did with them.  An old GDB can still
> talk to a new gdbserver and vice versa.  What you're proposing would
> break this cross version communication, just like option #1, right?
>
>>    3. Negotiate the wider format through qSupported.
> I think this is the only possible way forward unfortunately.  It is
> going to be more work, but anything else is going to end up breaking
> backward compatibility.
>
>> The second seems the least disruptive, but I don't have a good sense of who else implements these packets.
> That's a huge problem we have.  We really have no visibility at all for
> how this stuff is used outside the GDB project.  We solve this problem
> by basically assuming that anything that has been released might be
> being used, and so cannot be changed.  That's probably not true, but we
> just have no way of knowing.
>
>> Also worth deciding at the same time, if the layout is being revised:
>> fst_dev and fst_rdev are fio_uint_t although dev_t is 64-bit, and the three timestamps are 4 bytes.
> It sounds like what you're saying is that a whole bunch of fields are
> the wrong size.  I put together this table:
>
>    | Field Name  | Type        | Size | Actual |
>    |             |             |  Now | Size   |
>    |-------------+-------------+------+--------|
>    | fst_dev     | fio_uint_t  |    4 | 8      |
>    | fst_ino     | fio_uint_t  |    4 | 8      |
>    | fst_mode    | fio_mode_t  |    4 | ?      |
>    | fst_nlink   | fio_uint_t  |    4 | ?      |
>    | fst_uid     | fio_uint_t  |    4 | ?      |
>    | fst_gid     | fio_uint_t  |    4 | ?      |
>    | fst_rdev    | fio_uint_t  |    4 | 8      |
>    | fst_size    | fio_ulong_t |    8 | ?      |
>    | fst_blksize | fio_ulong_t |    8 | ?      |
>    | fst_blocks  | fio_ulong_t |    8 | ?      |
>    | fst_atime   | fio_time_t  |    4 | 8?     |
>    | fst_mtime   | fio_time_t  |    4 | 8?     |
>    | fst_ctime   | fio_time_t  |    4 | 8?     |
>
> The 'Size Now' is the current field size in fio_stat, while the 'Actual
> Size' is what the fields need to be in order to be correct on your
> system.  You mention the three timestamps above, but don't say what size
> they need to be.  I'm assuming 8, but that might not be correct either.
> Also there are some fields that you haven't mentioned, maybe they are
> all correct, but we should check.
>
> What I'd suggest is that you finish filling in the above table, then I
> think you'll want to have two versions of fio_stat, one with the current
> field sizes, and one with the wider field sizes.
>
> I'd suggest that you update the core throughout to use the struct with
> the wider field sizes, but at some point before this struct is sent down
> the wire, if the use of the wide struct has not been negotiated, then
> you'll need to squeeze the wide struct into the short one, trimming the
> fields.  I think it's OK to print a warning if non-zero information is
> discarded at this point.
>
> Similarly, when one of these structs is pulled from the wire, if the
> incoming packet is the narrow version, I'd expand it into the wide
> version.  In this way, most GDB/gdbserver code will only need to handle
> the version with the wider fields, but the wire protocol remains
> unchanged.
>
> Thanks,
> Andrew
>

[-- Attachment #2: Type: text/html, Size: 9529 bytes --]