Messages in this thread | | | Date | Wed, 4 Jan 2023 12:31:54 +0100 | From | Robert Richter <> | Subject | Re: [PATCH] cxl/mbox: Fix Payload Length check for Get Log command |
| |
On 03.01.23 14:11:33, Dan Williams wrote: > Robert Richter wrote: > > Commit 2aeaf663b85e introduced strict checking for variable length > > payload size validation. The payload length of received data must > > match the size of the requested data by the caller except for the case > > where the min_out value is set. > > > > The Get Log command does not have a header with a length field set. > > The Log size is determined by the Get Supported Logs command (CXL 3.0, > > 8.2.9.5.1). However, the actual size can be smaller and the number of > > valid bytes in the payload output must be determined reading the > > Payload Length field (CXL 3.0, Table 8-36, Note 2). > > > > Two issues arise: The command can successfully complete with a payload > > length of zero. And, the valid payload length must then also be > > consumed by the caller. > > Perhaps this is confusion about what the "Log Size" field of Get > Supported Logs means? My reading is that the "Log Size" field indicates > the data "currently available" in the log. Correct me if I am wrong, but > it seems your reading is that it is the "possibly available" data and > software can not assume anything is available until it actually goes to > read the log.
> Are you sure that this is not a device-side implementation issue where > it needs to make sure that Get Supported Logs indicates what Get Log can > expect?
The spec is not really clear here and I have seen a CXL device firmware implementation that interprets it like that. We could demand a firmware fix for that, but the kernel driver would be more robust if we lower the strictness here.
Reading the spec again I just found that "the maximum size of each Log" is mentioned in the description:
""" 8.2.9.5.1 Get Supported Logs (Opcode 0400h)
Retrieve the list of device specific logs (identified by UUID) and the maximum size of each Log. """
With that and the note in Table 8-36 stating that the exact payload of a variable length command should be determined using the Payload Length field, I think the commands can return different payload lengths.
> > There could be other variable payloads commands affected by this > > strict check, the implementation of GET_LSA and SET_LSA in this kernel > > could possibly be broken too. A fix of this is not scope of this > > patch. > > SET_LSA cannot be broken because SET_LSA does not return an output > payload, and GET_LSA never expects short reads.
Ok, I haven't checked the details here but thought it is worth to note.
> Now, if short reads need to be supported on production devices for any > variable length output payload command, I would rather see that handled > as a cxl_internal_send_cmd() special case where mbox_cmd->size_out is > consulted when cxl_internal_send_cmd() returns -EIO.
I will prepare a v2 with that change included.
Thanks,
-Robert
| |