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

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

[Abhay Kandpal]

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

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.
  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.
  3. Negotiate the wider format through qSupported.

The second seems the least disruptive, but I don't have a good sense of who else implements these packets.

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.

Thanks,
Abhay


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

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

[Andrew Burgess]

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

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

[Abhay Kandpal]

[-- 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 --]