Re: [RFC] nvdimm: Unitialized variable used in DSM calls

On Mon, Apr 24, 2017 at 03:56:50PM -0700, Dan Williams wrote: > On Mon, Apr 24, 2017 at 3:36 PM, Jerry Hoemann <jerry.hoemann(a)hpe.com&gt; wrote: > > nd_cmd_out_size is called by __nd_ioctl to size the buffer passed to > > acpi_nfit_ctl. If the DSM function being called has a variable > > sized output, nd_cmd_out_size will look at the return field to > > determine buffer size. However, the DSM call hasn't been made yet, > > so output size is core residue. > > > > Have nd_cmd_out_size be bimodal with version "early" to be called > > before the DSM call is made. For variable sized output fields > > have it return ND_IOCTL_MAX_BUFLEN. __nd_ioctl sees new return > > values and adjust buffer size accordingly. > > > > The downside to this approach are: > > 1) Requires user buffer input size to be ND_IOCTL_MAX_BUFLEN (4 MB) for > > calls to DSM with variable return. > > 2) The needless copyin of 4MB > > > > An alternative approach (not yet prototyped) would move the call > > to nd_cmd_out_size until after the return from acpi_nfit_ctl. Here, > > the call has been made and return size would be known. > > > > The size of the buffer allocated in would always be ND_IOCTL_MAX_BUFLEN. > > > > Signed-off-by: Jerry Hoemann <jerry.hoemann(a)hpe.com&gt; > > --- > > drivers/nvdimm/bus.c | 30 ++++++++++++++++++++++++------ > > 1 file changed, 24 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/nvdimm/bus.c b/drivers/nvdimm/bus.c > > index 23d4a17..50f8cc6 100644 > > --- a/drivers/nvdimm/bus.c > > +++ b/drivers/nvdimm/bus.c > > @@ -713,9 +713,9 @@ u32 nd_cmd_in_size(struct nvdimm *nvdimm, int cmd, > > } > > EXPORT_SYMBOL_GPL(nd_cmd_in_size); > > > > -u32 nd_cmd_out_size(struct nvdimm *nvdimm, int cmd, > > +u32 nd_cmd_out_size_early(struct nvdimm *nvdimm, int cmd, > > const struct nd_cmd_desc *desc, int idx, const u32 *in_field, > > - const u32 *out_field, unsigned long remainder) > > + const u32 *out_field, unsigned long remainder, int early) > > { > > if (idx >= desc->out_num) > > return UINT_MAX; > > @@ -725,9 +725,13 @@ u32 nd_cmd_out_size(struct nvdimm *nvdimm, int cmd, > > > > if (nvdimm && cmd == ND_CMD_GET_CONFIG_DATA && idx == 1) > > return in_field[1]; > > - else if (nvdimm && cmd == ND_CMD_VENDOR && idx == 2) > > + else if (nvdimm && cmd == ND_CMD_VENDOR && idx == 2) { > > + if (early) > > + return ND_IOCTL_MAX_BUFLEN; > > return out_field[1]; > > Are you using ndctl_dimm_cmd_new_vendor_specific() to submit a vendor > specific command? It already pre-populates out_field[1] with the > initial output size. I was actually concerned w/ calling the DSM for nd_cmd_ars_status and changed ND_CMD_VENDOR on spec as it looked like it had the same pattern. Hit this using a stand alone test tool I have. For nd_cmd_ars_status, does the caller want to set the output field "num_records" before making the IOCTL call? I.e. setting the maximum number of records the caller is prepared to accept?

I took a quick look at the ndctl source and its not obvious that is what the tool is doing. But I might have missed it. --