| Message ID | 20230103205937.1126626-1-rrichter@amd.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | cxl/mbox: Fix Payload Length check for Get Log command | expand |
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. The spec does feel ambiguous here as it could have left off the word "maximum" to make it clearer it is the data currently pending value, or leave off the word "available" to make it clear that this value is the max that will ever be valid for one retrieval, but not necessarily currently. I do not think "CXL 3.0, Table 8-36, Note 2" helps to clear this up either as that does not answer what expectations software can assume about Get Log::Log Size. 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? Table 8-94 "Identify Memory Device Output Payload" specifies the *possible* data that can be made available in a log. > > Change cxl_xfer_log() to pass the number of payload bytes back to the > caller to determine the number of log entries. > > Logs can be bigger than the maximum payload length and multiple Get > Log commands can be issued. If the received payload size is smaller > than the maximum payload size we can assume all valid bytes have been > fetched. Stop sending further Get Log commands then. > > Also, implement CXL_NO_PAYLOAD_SIZE_VALIDATION as special value to > @min_out to skip the payload size validation check. A value of zero > for @min_out is already widespread in use as default if the size must > match @size_out. Thus, zero can not be passed to allow zero length > variable payloads, CXL_NO_PAYLOAD_SIZE_VALIDATION should be used > instead for this case. > > On that occasion, change debug messages to also report supported > opcodes. > > 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. 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. > > Fixes: 2aeaf663b85e ("cxl/mbox: Add variable output size validation for internal commands") > Signed-off-by: Robert Richter <rrichter@amd.com> > --- > drivers/cxl/core/mbox.c | 41 ++++++++++++++++++++++++++++++----------- > drivers/cxl/cxlmem.h | 5 +++++ > 2 files changed, 35 insertions(+), 11 deletions(-) > > diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c > index b03fba212799..0c2056ae8aff 100644 > --- a/drivers/cxl/core/mbox.c > +++ b/drivers/cxl/core/mbox.c > @@ -183,11 +183,16 @@ int cxl_internal_send_cmd(struct cxl_dev_state *cxlds, > * Variable sized output needs to at least satisfy the caller's > * minimum if not the fully requested size. > */ > + > + if (min_out == CXL_NO_PAYLOAD_SIZE_VALIDATION) > + return 0; > + > if (min_out == 0) > min_out = out_size; > > if (mbox_cmd->size_out < min_out) > return -EIO; > + > return 0; > } > EXPORT_SYMBOL_NS_GPL(cxl_internal_send_cmd, CXL); > @@ -554,6 +559,7 @@ static int cxl_xfer_log(struct cxl_dev_state *cxlds, uuid_t *uuid, u32 size, u8 > { > u32 remaining = size; > u32 offset = 0; > + u32 size_out; > > while (remaining) { > u32 xfer_size = min_t(u32, remaining, cxlds->payload_size); > @@ -572,6 +578,8 @@ static int cxl_xfer_log(struct cxl_dev_state *cxlds, uuid_t *uuid, u32 size, u8 > .size_in = sizeof(log), > .payload_in = &log, > .size_out = xfer_size, > + /* Any size is allowed (CXL 3.0, Table 8-36). */ > + .min_out = CXL_NO_PAYLOAD_SIZE_VALIDATION, > .payload_out = out, > }; > > @@ -579,12 +587,24 @@ static int cxl_xfer_log(struct cxl_dev_state *cxlds, uuid_t *uuid, u32 size, u8 > if (rc < 0) > return rc; > > - out += xfer_size; > - remaining -= xfer_size; > - offset += xfer_size; > + size_out = mbox_cmd.size_out; > + if (size_out > xfer_size) > + return -ENXIO; > + > + out += size_out; > + remaining -= size_out; > + offset += size_out; > + > + /* > + * A smaller output payload length indicates all valid > + * bytes have been fetched. > + */ > + if (size_out < xfer_size) > + break; > } > > - return 0; > + /* Total number of bytes fetched. */ > + return offset; > } > > /** > @@ -608,13 +628,11 @@ static void cxl_walk_cel(struct cxl_dev_state *cxlds, size_t size, u8 *cel) > u16 opcode = le16_to_cpu(cel_entry[i].opcode); > struct cxl_mem_command *cmd = cxl_mem_find_command(opcode); > > - if (!cmd) { > - dev_dbg(cxlds->dev, > - "Opcode 0x%04x unsupported by driver", opcode); > - continue; > - } > + if (cmd) > + set_bit(cmd->info.id, cxlds->enabled_cmds); > > - set_bit(cmd->info.id, cxlds->enabled_cmds); > + dev_dbg(cxlds->dev, "Opcode 0x%04x %ssupported by driver", > + opcode, cmd ? "" : "un"); > } > } > > @@ -695,11 +713,12 @@ int cxl_enumerate_cmds(struct cxl_dev_state *cxlds) > } > > rc = cxl_xfer_log(cxlds, &uuid, size, log); > - if (rc) { > + if (rc < 0) { > kvfree(log); > goto out; > } > > + size = (u32)rc; > cxl_walk_cel(cxlds, size, log); > kvfree(log); > > diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h > index ab138004f644..2db24b062913 100644 > --- a/drivers/cxl/cxlmem.h > +++ b/drivers/cxl/cxlmem.h > @@ -102,6 +102,10 @@ static inline struct cxl_ep *cxl_ep_load(struct cxl_port *port, > * variable sized output commands, it tells the exact number of bytes > * written. > * @min_out: (input) internal command output payload size validation > + * * %0: Payload size must match @size_out. > + * * %>0: Minimum payload size. > + * * %CXL_NO_PAYLOAD_SIZE_VALIDATION: Skip payload size validation check. > + * > * @return_code: (output) Error code returned from hardware. > * > * This is the primary mechanism used to send commands to the hardware. > @@ -117,6 +121,7 @@ struct cxl_mbox_cmd { > size_t size_in; > size_t size_out; > size_t min_out; > +#define CXL_NO_PAYLOAD_SIZE_VALIDATION SIZE_MAX > u16 return_code; > }; > > -- > 2.30.2 >
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
Dan, On 04.01.23 12:31:59, Robert Richter wrote: > 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. any opinion here? Looks like the device may send a smaller payload in the 401h command than the size given in 400h. I have sent an updated patch last week: [PATCH v2] cxl/mbox: Fix Payload Length check for Get Log command https://patchwork.kernel.org/project/cxl/patch/20230104202954.1163366-1-rrichter@amd.com/ Please take a look. Thanks, -Robert
diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c index b03fba212799..0c2056ae8aff 100644 --- a/drivers/cxl/core/mbox.c +++ b/drivers/cxl/core/mbox.c @@ -183,11 +183,16 @@ int cxl_internal_send_cmd(struct cxl_dev_state *cxlds, * Variable sized output needs to at least satisfy the caller's * minimum if not the fully requested size. */ + + if (min_out == CXL_NO_PAYLOAD_SIZE_VALIDATION) + return 0; + if (min_out == 0) min_out = out_size; if (mbox_cmd->size_out < min_out) return -EIO; + return 0; } EXPORT_SYMBOL_NS_GPL(cxl_internal_send_cmd, CXL); @@ -554,6 +559,7 @@ static int cxl_xfer_log(struct cxl_dev_state *cxlds, uuid_t *uuid, u32 size, u8 { u32 remaining = size; u32 offset = 0; + u32 size_out; while (remaining) { u32 xfer_size = min_t(u32, remaining, cxlds->payload_size); @@ -572,6 +578,8 @@ static int cxl_xfer_log(struct cxl_dev_state *cxlds, uuid_t *uuid, u32 size, u8 .size_in = sizeof(log), .payload_in = &log, .size_out = xfer_size, + /* Any size is allowed (CXL 3.0, Table 8-36). */ + .min_out = CXL_NO_PAYLOAD_SIZE_VALIDATION, .payload_out = out, }; @@ -579,12 +587,24 @@ static int cxl_xfer_log(struct cxl_dev_state *cxlds, uuid_t *uuid, u32 size, u8 if (rc < 0) return rc; - out += xfer_size; - remaining -= xfer_size; - offset += xfer_size; + size_out = mbox_cmd.size_out; + if (size_out > xfer_size) + return -ENXIO; + + out += size_out; + remaining -= size_out; + offset += size_out; + + /* + * A smaller output payload length indicates all valid + * bytes have been fetched. + */ + if (size_out < xfer_size) + break; } - return 0; + /* Total number of bytes fetched. */ + return offset; } /** @@ -608,13 +628,11 @@ static void cxl_walk_cel(struct cxl_dev_state *cxlds, size_t size, u8 *cel) u16 opcode = le16_to_cpu(cel_entry[i].opcode); struct cxl_mem_command *cmd = cxl_mem_find_command(opcode); - if (!cmd) { - dev_dbg(cxlds->dev, - "Opcode 0x%04x unsupported by driver", opcode); - continue; - } + if (cmd) + set_bit(cmd->info.id, cxlds->enabled_cmds); - set_bit(cmd->info.id, cxlds->enabled_cmds); + dev_dbg(cxlds->dev, "Opcode 0x%04x %ssupported by driver", + opcode, cmd ? "" : "un"); } } @@ -695,11 +713,12 @@ int cxl_enumerate_cmds(struct cxl_dev_state *cxlds) } rc = cxl_xfer_log(cxlds, &uuid, size, log); - if (rc) { + if (rc < 0) { kvfree(log); goto out; } + size = (u32)rc; cxl_walk_cel(cxlds, size, log); kvfree(log); diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h index ab138004f644..2db24b062913 100644 --- a/drivers/cxl/cxlmem.h +++ b/drivers/cxl/cxlmem.h @@ -102,6 +102,10 @@ static inline struct cxl_ep *cxl_ep_load(struct cxl_port *port, * variable sized output commands, it tells the exact number of bytes * written. * @min_out: (input) internal command output payload size validation + * * %0: Payload size must match @size_out. + * * %>0: Minimum payload size. + * * %CXL_NO_PAYLOAD_SIZE_VALIDATION: Skip payload size validation check. + * * @return_code: (output) Error code returned from hardware. * * This is the primary mechanism used to send commands to the hardware. @@ -117,6 +121,7 @@ struct cxl_mbox_cmd { size_t size_in; size_t size_out; size_t min_out; +#define CXL_NO_PAYLOAD_SIZE_VALIDATION SIZE_MAX u16 return_code; };
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. Change cxl_xfer_log() to pass the number of payload bytes back to the caller to determine the number of log entries. Logs can be bigger than the maximum payload length and multiple Get Log commands can be issued. If the received payload size is smaller than the maximum payload size we can assume all valid bytes have been fetched. Stop sending further Get Log commands then. Also, implement CXL_NO_PAYLOAD_SIZE_VALIDATION as special value to @min_out to skip the payload size validation check. A value of zero for @min_out is already widespread in use as default if the size must match @size_out. Thus, zero can not be passed to allow zero length variable payloads, CXL_NO_PAYLOAD_SIZE_VALIDATION should be used instead for this case. On that occasion, change debug messages to also report supported opcodes. 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. Fixes: 2aeaf663b85e ("cxl/mbox: Add variable output size validation for internal commands") Signed-off-by: Robert Richter <rrichter@amd.com> --- drivers/cxl/core/mbox.c | 41 ++++++++++++++++++++++++++++++----------- drivers/cxl/cxlmem.h | 5 +++++ 2 files changed, 35 insertions(+), 11 deletions(-)