Re: Re: [PATCH v2] cxl/mbox: bound the Get Supported Logs entry count by the payload

[黄高彬]

Hi Richard,

Thank you, and sorry for the slow turn-around -- both suggestions are in v3,
and the spec pointer settles the clamping-versus-rejecting question rather
than just making rejecting safer.

> I would suggest to keep the signature and do the check here instead.
> cxl_get_gsl() issues the command, owns the buffer and already has the
> kvfree()/ERR_PTR error path. Then the caller doesn't need to know the header
> size or the entry size, and gsl->entries can be trusted by whoever uses the
> struct.

Agreed, and that is where v3 puts it.  cxl_enumerate_cmds() is now unchanged
from mainline:

	mbox_cmd = (struct cxl_mbox_cmd) {
		.opcode = CXL_MBOX_OP_GET_SUPPORTED_LOGS,
		.size_out = cxl_mbox->payload_size,
		.payload_out = ret,
		/* At least the header must be valid */
		.min_out = struct_size(ret, entry, 0),
	};
	rc = cxl_internal_send_cmd(cxl_mbox, &mbox_cmd);
	if (rc < 0) {
		kvfree(ret);
		return ERR_PTR(rc);
	}

	max_entries = (mbox_cmd.size_out - struct_size(ret, entry, 0)) /
		      sizeof(ret->entry[0]);
	if (le16_to_cpu(ret->entries) > max_entries) {
		dev_err(mds->cxlds.dev,
			"GSL: device claimed %u entries but the payload holds %zu\n",
			le16_to_cpu(ret->entries), max_entries);
		kvfree(ret);
		return ERR_PTR(-EIO);
	}

> .min_out = 2 is the only min_out in the tree that's not a full header.
> Raising it to struct_size(ret, entry, 0) lets cxl_internal_send_cmd() reject
> the short response with -EIO, so the "too short for the header" part can
> go away.

Taken.  The subtraction cannot underflow once min_out guarantees the header,
so the guard around it is gone too, and it matches the two users already in
this file (struct_size(payload, records, 0) and struct_size(po, record, 0)).

> The only kvfree() is at "out" section, directly return here would leak
> gsl.

That was mine, in v2.  Moving the check into cxl_get_gsl() removes the path
instead of repairing it.  sashiko reported the same leak on v2.

> Agree with Alison and Jonathan, don't clamp here.
> CXL r4.0 Section 8.2.10.5.1 Get Supported Logs, Table 8-249 ... defines
> "Number of Supported Logs Entries" as "the number of Supported Log Entries
> returned in the output payload".
> It's not total, so a counter larger than what arrived is a malformed
> response, not a partial list.

Thank you for the citation.  It is in the commit message now, because it is
what makes rejecting the right answer rather than merely the safer one.

> About -EIO v.s. -ENXIO, I would go with -EIO.

-EIO it is, and it is now literally cxl_internal_send_cmd()'s payload size
mismatch error rather than a choice made here.

v3:
https://lore.kernel.org/all/20260929060610.3549718-1-huanggaobin23@xxxxxxxxxx/

Gaobin