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

[Gavin Shan <gwshan@linux.vnet.ibm.com>]

On Thu, Mar 12, 2015 at 03:21:29PM +1100, David Gibson wrote:
>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.
>

Thank you for more explanation. I agree that it's worthy to pass precise
argument size. I'll fix it as below in next revision:

>> 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