tmpfs: Rework file handles

[olce]

Summary

Change 'struct tmpfs_fid_data' to behave consistently with the private
structure other FSes use. In a nutshell, make it a full alias of
'struct fid', instead of just using it to fill 'fid_data'. This implies
adding a length field at start (aliasing 'fid_len' of 'struct fid'), and
filling 'fid_len' with the full size of the aliased structure.

To ensure that the new 'struct tmpfs_fid_data' is smaller than 'struct
fid', which the compile-time assert introduced in commit
91b5592a1e1af974 ("fs: Add static asserts for the size of fid
structures") checks (and thus was not strong enough when added), shrink
the 'tfd_gen' field to 'unsigned int', as it only contains the result of
a call to arc4random(), and move it before 'tfd_gen' to compact the
structure (there is still a 16-bit hole before 'tfd_gen').

A consequence of this change is that copying the 'struct tmpfs_fid_data'
into a stack-allocated variable becomes unnecessary, as all fields are
then naturally aligned (provided that the embedding 'struct fhandle' is
so, which is normally guaranteed by kernel's malloc()).

[rmacklem]

I'll leave it someone familiar with tmpfs to decide if having
tn_gen wrap around to zero is a real concern.

I am not sure why others are concerned about using __packed.
The uses are simple and not that frequently executed.

sys/fs/tmpfs/tmpfs.h

219

Although this field is initialized by arc4random(),
it is also incremented.
--> Having it 64bits guarantees it will not wrap around to zero.

[rmacklem]

Good catch w.r.t. the _Static_assert(). It does need to be fixed,
even if these changes don't go in.

[rmacklem]

Btw, I replaced "sizeof(struct fid)" with MAXFIDSZ in the _Static_assert()
and it passed (at least for amd64).

It might make more sense to convert the other file systems where the
_Static_assert() fails with code that just copies fid_data the way tmpfs does?
(tarfs is the current case)

[rmacklem]

Oh, and I think just copying fid_data would fix cd9660
as well, for those who don't like __packed.

[olce]

In D47956#1093709, @rmacklem wrote:

I'll leave it someone familiar with tmpfs to decide if having
tn_gen wrap around to zero is a real concern.

Wrapping around doesn't seem to be of a concern, as incrementing is just a shortcut chosen to avoid calling arc4random() each time a tmpfs_node structure is recycled, and I very much doubt that allowing the counter in > 32 bits territory in this case adds any real security. Actually, the shortcut itself looks slightly dangerous, and it may be better to remove it entirely, but that's a different matter.

I am not sure why others are concerned about using __packed.
The uses are simple and not that frequently executed.

I don't think there are many more reasons than that it's better not to use __packed for the compiler to generate better, straightforward code, and that seeing __packed is surprising, as it is usually used for serialization/deserialization issues with other systems/components (it may be argued that this is somehow the case here, as we're in fact doing a kind of ser/des into the generic 'struct fid').

I don't really know how frequent fid building and processing are. Is this done at each NFS access request? In any case, I agree that performance-wise this doesn't matter currently as this is second order or more compared to what is in the implementation of tmpfs_fhtovp() (which scans all existing tmpfs nodes!).

[olce]

In D47956#1093749, @rmacklem wrote:

Btw, I replaced "sizeof(struct fid)" with MAXFIDSZ in the _Static_assert()
and it passed (at least for amd64).

If we don't commit the changes here, yes, that would be enough. You can even commit it in the meantime if you prefer.

It might make more sense to convert the other file systems where the
_Static_assert() fails with code that just copies fid_data the way tmpfs does?
(tarfs is the current case)

I think we should actually do the opposite, i.e., stop copying unaligned structures/fields when what the code does is actually just access the fields once, and rely on __packed in cases where we can't reorder the fields to shrink the structure size enough.

This copying is in fact more or less implementing __packed by hand, and I think source-wise it's worse to do that than just using __packed (there may be different reasons to still want to avoid __packed, e.g., if it is believed that compilers are frequently buggy in this respect, on more exotic architectures).

In D47956#1093750, @rmacklem wrote:

Oh, and I think just copying fid_data would fix cd9660
as well, for those who don't like __packed.

That would technically work, but is the same case as tarfs above and I wouldn't do it for the same reasons exposed above.

I'd like to know if some people oppose the use of __packed in these contexts, and why.