fwcontrol: Allocate full fw_asyreq structures passed to the kernel

[jhb]

Summary

The FW_ASYREQ ioctl accepts a struct fw_asyreq object as its argument,
meaning that the kernel always copies in the full structure in
sys_ioctl before passing the request down to the driver. However,
fwcontrol was allocating smaller objects that contained only the
request header and a variable-sized payload. This means that the
kernel copy in sys_ioctl was reading off the end of this buffer. On
current architectures this happened to be ok, but it is UB.

Instead, allocate a full structure.

Reported by: GCC 14 -Walloc-size

[rlibby]

I don't know this code but I'm assuming that we don't really care about
the overhead here or performance generally for this ioctl. In other
words, we don't need a longer term fix to do a saner amount of copying,
correct? It seems that we never actually use asyreq->data in the ioctl,
so we're now allocating and passing around an extra 2k because of a
weird definition?

I'm not sure that this would have always been okay in current arch,
couldn't we get unlucky and run off a map?

Also, I think this kind of pattern is behind many of our -Warray-bounds
violations too, where we allocate a part of a structure and then rely on
code referring to only those parts. Especially in areas like cam. This
one is a little different in that we apparently also actually access
beyond the short allocation.

[rlibby]

I guess you can also consider if you want to make the malloc spelling more conventional. Don't need the casts, and can now do sizeof(*asyreq). I'm ambivalent, but in case you hadn't considered it.

[brooks]

Why is struct fw_asyreq_t even a type?

[jhb]

In D46014#1048966, @brooks wrote:

Why is struct fw_asyreq_t even a type?

I think only to support this weirdness where the intention was to have variable-sized messages, but then the ioctl doesn't support variable-sized messages. I didn't remove the type name because I don't want to break ports, but would not oppose to having it removed as a separate commit.

[jhb]

In D46014#1048959, @rlibby wrote:

I don't know this code but I'm assuming that we don't really care about
the overhead here or performance generally for this ioctl. In other
words, we don't need a longer term fix to do a saner amount of copying,
correct? It seems that we never actually use asyreq->data in the ioctl,
so we're now allocating and passing around an extra 2k because of a
weird definition?

So the ioctl was always doing the 2k copy, the only change here is how much
space malloc is reserving.

I'm not sure that this would have always been okay in current arch,
couldn't we get unlucky and run off a map?

I think all of our malloc implementations to date do not cause sub-page-sized
allocations to span a page boundary.

Also, I think this kind of pattern is behind many of our -Warray-bounds
violations too, where we allocate a part of a structure and then rely on
code referring to only those parts. Especially in areas like cam. This
one is a little different in that we apparently also actually access
beyond the short allocation.

Well, some of those are due to using [0] arrays instead of the more modern syntax for variable length arrays.

[jhb]

In D46014#1048963, @rlibby wrote:

I guess you can also consider if you want to make the malloc spelling more conventional. Don't need the casts, and can now do sizeof(*asyreq). I'm ambivalent, but in case you hadn't considered it.

I had wanted to remove the casts actually, so I will make those changes before pushing.

[rlibby]

In D46014#1049503, @jhb wrote:

In D46014#1048959, @rlibby wrote:

Also, I think this kind of pattern is behind many of our -Warray-bounds
violations too, where we allocate a part of a structure and then rely on
code referring to only those parts. Especially in areas like cam. This
one is a little different in that we apparently also actually access
beyond the short allocation.

Well, some of those are due to using [0] arrays instead of the more modern syntax for variable length arrays.

This is a tangent obviously, but since we're talking about this kind of warning cleanup, and I've been thinking about whether it is worth time to go address it.

My understanding, at least of the docs, is that -Warray-bounds does not warn about old-style VLAs where the [0] array is at the end of the structure, at least not by default.
https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html#index-Wno-array-bounds

It will warn in places where a [0] array is used as a quasi-union, like struct { int a[0]; char b[sizeof(int)]; }. I don't recall if we have any of those in tree, but I have seen that kind of warning elsewhere.

Examples of the warning-tripping code I'm referring to are things like in sys/amd64/amd64/pmap.c:reclaim_pv_chunk_domain, pc_marker = (struct pv_chunk *)&pc_marker_b; and subsequent use, or the many callers of sys/cam/cam_xpt.c:xpt_action that cast their argument to union ccb *. In those cases, gcc issues a warning about casting and using a pointer to an object that is in fact smaller than the type referenced by the pointer (Warray-bounds). We are relying (hopefully correctly) on the subsequent use never actually accessing the fields that are beyond the end of the actual object, but gcc issues the warning without knowing that or being able to check that.

I think we should at least come up with a better pattern for this kind of code for the future, regardless of whether we rototill the older stuff.

Anyway, just some related thoughts.

[jhb]

In D46014#1049557, @rlibby wrote:

In D46014#1049503, @jhb wrote:

In D46014#1048959, @rlibby wrote:

Also, I think this kind of pattern is behind many of our -Warray-bounds
violations too, where we allocate a part of a structure and then rely on
code referring to only those parts. Especially in areas like cam. This
one is a little different in that we apparently also actually access
beyond the short allocation.

Well, some of those are due to using [0] arrays instead of the more modern syntax for variable length arrays.

This is a tangent obviously, but since we're talking about this kind of warning cleanup, and I've been thinking about whether it is worth time to go address it.

My understanding, at least of the docs, is that -Warray-bounds does not warn about old-style VLAs where the [0] array is at the end of the structure, at least not by default.
https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html#index-Wno-array-bounds

I think I misremembered and that some places use [1] for a VLA, but since the size is part of an ABI it's hard to go fix those to use a proper VLA.

It will warn in places where a [0] array is used as a quasi-union, like struct { int a[0]; char b[sizeof(int)]; }. I don't recall if we have any of those in tree, but I have seen that kind of warning elsewhere.

We have a few of these in Linux-inherited bits (e.g. OFED and the mlx* drivers).

Examples of the warning-tripping code I'm referring to are things like in sys/amd64/amd64/pmap.c:reclaim_pv_chunk_domain, pc_marker = (struct pv_chunk *)&pc_marker_b; and subsequent use, or the many callers of sys/cam/cam_xpt.c:xpt_action that cast their argument to union ccb *. In those cases, gcc issues a warning about casting and using a pointer to an object that is in fact smaller than the type referenced by the pointer (Warray-bounds). We are relying (hopefully correctly) on the subsequent use never actually accessing the fields that are beyond the end of the actual object, but gcc issues the warning without knowing that or being able to check that.

I think we should at least come up with a better pattern for this kind of code for the future, regardless of whether we rototill the older stuff.

Anyway, just some related thoughts.

The union ccb issue (which I also ran into in a similar form of in CTL in a few places for NVMe vs SCSI) is that what the API wants to be is that you have a base class (struct ccb_hdr) and a bunch of derived classes (struct ccb_foo`). The union really should only be used to allocate storage, similar to a struct sockaddr_storage, but the API calls should all be using the base class. That is, things like xpt_action should be taking a struct ccb_hdr * instead of a union ccb *. That's a pretty giant rototill of CAM, but I think that's the "right" way to fix it.

One could make a similar argument for the pmap case where the linked list is really of pv_chunk headers that should be downcasted to an actual pv_chunk when the full chunk is needed.