Re: [PATCH 2/2] drivers/vfio: Support EEH error injection

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 2773 bytes --]

On Thu, Mar 12, 2015 at 02:16:42PM +1100, Gavin Shan wrote:
> On Thu, Mar 12, 2015 at 11:57:21AM +1100, David Gibson wrote:
> >On Wed, Mar 11, 2015 at 05:34:11PM +1100, Gavin Shan wrote:
> >> The patch adds one more EEH sub-command (VFIO_EEH_PE_INJECT_ERR)
> >> to inject the specified EEH error, which is represented by
> >> (struct vfio_eeh_pe_err), to the indicated PE for testing purpose.
> >> 
> >> Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
> >> ---
> >>  Documentation/vfio.txt        | 47 ++++++++++++++++++++++++++++++-------------
> >>  drivers/vfio/vfio_spapr_eeh.c | 14 +++++++++++++
> >>  include/uapi/linux/vfio.h     | 34 ++++++++++++++++++++++++++++++-
> >>  3 files changed, 80 insertions(+), 15 deletions(-)
> >> 
> >> diff --git a/Documentation/vfio.txt b/Documentation/vfio.txt
> >> index 96978ec..2e7f736 100644
> >> --- a/Documentation/vfio.txt
> >> +++ b/Documentation/vfio.txt
> >> @@ -328,7 +328,13 @@ So 4 additional ioctls have been added:
> >>  
> >>  The code flow from the example above should be slightly changed:
> >>  
> >> -	struct vfio_eeh_pe_op pe_op = { .argsz = sizeof(pe_op), .flags = 0 };
> >> +	struct vfio_eeh_pe_op *pe_op;
> >> +	struct vfio_eeh_pe_err *pe_err;
> >> +
> >> +	pe_op = malloc(sizeof(*pe_op) + sizeof(*pe_err));
> >> +	pe_err = (void *)pe_op + sizeof(*pe_op);
> >> +	pe_op->argsz = sizeof(*pe_op) + sizeof(*pe_err);
> >
> >Surely that argsz can't be correct for most of the operations.  The
> >extended structure should only be there for the error inject ioctl,
> >yes?
> >
> 
> argsz isn't appropriate for most cases because kernel has the check
> "expected_argsz < passed_argsz", not "expected_argsz ==
> passed_argsz".

It works for now, but if any of those calls was extended with more
data, it would break horribly.  By setting the argsz greater than
necessary, you're effectively passing uninitialized data to the
ioctl().  At the moment, the ioctl() ignores it, but the whole point
of the argsz value is that in the future, it might not.

> However, I'll fix it as follows to avoid confusion after collecting
> more comments:
> 
> 	struct vfio_eeh_pe_op *pe_op;
> 	struct vfio_eeh_pe_err *pe_err;
> 
> 	/* For all cases except error injection */
> 	pe_op = malloc(sizeof(*pe_op));
> 	pe_op->argsz = sizeof(*pe_op);
> 
> 	/* For error injection case here */
> 	pe_op = realloc(sizeof(*pe_op) + sizeof(*pe_err));
> 	pe_op->argsz = sizeof(*pe_op) + sizeof(*pe_err);
> 	pe_err = (void *)pe_op + sizeof(*pe_op);
> 
> Thanks,
> Gavin
> 
> 
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]