* [PATCH v3] drivers/vfio: Fix a redundant copy bug
@ 2018-10-17 14:32 Wenwen Wang
2018-10-17 15:45 ` Alex Williamson
0 siblings, 1 reply; 5+ messages in thread
From: Wenwen Wang @ 2018-10-17 14:32 UTC (permalink / raw)
To: Wenwen Wang; +Cc: Kangjie Lu, Alex Williamson, open list:VFIO DRIVER, open list
In vfio_spapr_iommu_eeh_ioctl(), if the ioctl command is VFIO_EEH_PE_OP,
the user-space buffer 'arg' is copied to the kernel object 'op' and the
'argsz' and 'flags' fields of 'op' are checked. If the check fails, an
error code EINVAL is returned. Otherwise, 'op.op' is further checked
through a switch statement to invoke related handlers. If 'op.op' is
VFIO_EEH_PE_INJECT_ERR, the whole user-space buffer 'arg' is copied again
to 'op' to obtain the err information. However, in the following execution
of this case, the fields of 'op', except the field 'err', are actually not
used. That is, the second copy has a redundant part. Therefore, for both
performance consideration, the redundant part of the second copy should be
removed.
This patch removes such a part in the second copy. It only copies from
'err.type' to 'err.mask', which is exactly required by the
VFIO_EEH_PE_INJECT_ERR op.
This patch also adds a 4-byte reserved field in the structure
vfio_eeh_pe_op to make sure that the u64 fields in the structure
vfio_eeh_pe_err are 8-byte aligned.
Signed-off-by: Wenwen Wang <wang6495@umn.edu>
---
drivers/vfio/vfio_spapr_eeh.c | 9 ++++++---
include/uapi/linux/vfio.h | 1 +
2 files changed, 7 insertions(+), 3 deletions(-)
diff --git a/drivers/vfio/vfio_spapr_eeh.c b/drivers/vfio/vfio_spapr_eeh.c
index 38edeb4..66634c6 100644
--- a/drivers/vfio/vfio_spapr_eeh.c
+++ b/drivers/vfio/vfio_spapr_eeh.c
@@ -37,6 +37,7 @@ long vfio_spapr_iommu_eeh_ioctl(struct iommu_group *group,
struct eeh_pe *pe;
struct vfio_eeh_pe_op op;
unsigned long minsz;
+ unsigned long start, end;
long ret = -EINVAL;
switch (cmd) {
@@ -86,10 +87,12 @@ long vfio_spapr_iommu_eeh_ioctl(struct iommu_group *group,
ret = eeh_pe_configure(pe);
break;
case VFIO_EEH_PE_INJECT_ERR:
- minsz = offsetofend(struct vfio_eeh_pe_op, err.mask);
- if (op.argsz < minsz)
+ start = offsetof(struct vfio_eeh_pe_op, err.type);
+ end = offsetofend(struct vfio_eeh_pe_op, err.mask);
+ if (op.argsz < end)
return -EINVAL;
- if (copy_from_user(&op, (void __user *)arg, minsz))
+ if (copy_from_user(&op.err, (char __user *)arg +
+ start, end - start))
return -EFAULT;
ret = eeh_pe_inject_err(pe, op.err.type, op.err.func,
diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
index 1aa7b82..d904c42 100644
--- a/include/uapi/linux/vfio.h
+++ b/include/uapi/linux/vfio.h
@@ -729,6 +729,7 @@ struct vfio_eeh_pe_op {
__u32 argsz;
__u32 flags;
__u32 op;
+ __u32 __resv;
union {
struct vfio_eeh_pe_err err;
};
--
2.7.4
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v3] drivers/vfio: Fix a redundant copy bug
2018-10-17 14:32 [PATCH v3] drivers/vfio: Fix a redundant copy bug Wenwen Wang
@ 2018-10-17 15:45 ` Alex Williamson
2018-10-17 17:58 ` Wenwen Wang
0 siblings, 1 reply; 5+ messages in thread
From: Alex Williamson @ 2018-10-17 15:45 UTC (permalink / raw)
To: Wenwen Wang
Cc: Kangjie Lu, open list:VFIO DRIVER, open list, aik, David Gibson
On Wed, 17 Oct 2018 09:32:04 -0500
Wenwen Wang <wang6495@umn.edu> wrote:
> In vfio_spapr_iommu_eeh_ioctl(), if the ioctl command is VFIO_EEH_PE_OP,
> the user-space buffer 'arg' is copied to the kernel object 'op' and the
> 'argsz' and 'flags' fields of 'op' are checked. If the check fails, an
> error code EINVAL is returned. Otherwise, 'op.op' is further checked
> through a switch statement to invoke related handlers. If 'op.op' is
> VFIO_EEH_PE_INJECT_ERR, the whole user-space buffer 'arg' is copied again
> to 'op' to obtain the err information. However, in the following execution
> of this case, the fields of 'op', except the field 'err', are actually not
> used. That is, the second copy has a redundant part. Therefore, for both
> performance consideration, the redundant part of the second copy should be
> removed.
>
> This patch removes such a part in the second copy. It only copies from
> 'err.type' to 'err.mask', which is exactly required by the
> VFIO_EEH_PE_INJECT_ERR op.
>
> This patch also adds a 4-byte reserved field in the structure
> vfio_eeh_pe_op to make sure that the u64 fields in the structure
> vfio_eeh_pe_err are 8-byte aligned.
>
> Signed-off-by: Wenwen Wang <wang6495@umn.edu>
> ---
> drivers/vfio/vfio_spapr_eeh.c | 9 ++++++---
> include/uapi/linux/vfio.h | 1 +
> 2 files changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/vfio/vfio_spapr_eeh.c b/drivers/vfio/vfio_spapr_eeh.c
> index 38edeb4..66634c6 100644
> --- a/drivers/vfio/vfio_spapr_eeh.c
> +++ b/drivers/vfio/vfio_spapr_eeh.c
> @@ -37,6 +37,7 @@ long vfio_spapr_iommu_eeh_ioctl(struct iommu_group *group,
> struct eeh_pe *pe;
> struct vfio_eeh_pe_op op;
> unsigned long minsz;
> + unsigned long start, end;
> long ret = -EINVAL;
>
> switch (cmd) {
> @@ -86,10 +87,12 @@ long vfio_spapr_iommu_eeh_ioctl(struct iommu_group *group,
> ret = eeh_pe_configure(pe);
> break;
> case VFIO_EEH_PE_INJECT_ERR:
> - minsz = offsetofend(struct vfio_eeh_pe_op, err.mask);
> - if (op.argsz < minsz)
> + start = offsetof(struct vfio_eeh_pe_op, err.type);
I noted in the previous version that we already have this in minsz, so
you're fixing a redundant copy with a redundant operation.
> + end = offsetofend(struct vfio_eeh_pe_op, err.mask);
> + if (op.argsz < end)
> return -EINVAL;
> - if (copy_from_user(&op, (void __user *)arg, minsz))
> + if (copy_from_user(&op.err, (char __user *)arg +
> + start, end - start))
> return -EFAULT;
>
> ret = eeh_pe_inject_err(pe, op.err.type, op.err.func,
> diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
> index 1aa7b82..d904c42 100644
> --- a/include/uapi/linux/vfio.h
> +++ b/include/uapi/linux/vfio.h
> @@ -729,6 +729,7 @@ struct vfio_eeh_pe_op {
> __u32 argsz;
> __u32 flags;
> __u32 op;
> + __u32 __resv;
> union {
> struct vfio_eeh_pe_err err;
> };
Please don't include two separate issues in the same patch. Am I also
correct in assuming that this is untested? Thanks,
Alex
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v3] drivers/vfio: Fix a redundant copy bug
2018-10-17 15:45 ` Alex Williamson
@ 2018-10-17 17:58 ` Wenwen Wang
2018-10-17 19:05 ` Alex Williamson
0 siblings, 1 reply; 5+ messages in thread
From: Wenwen Wang @ 2018-10-17 17:58 UTC (permalink / raw)
To: alex.williamson; +Cc: Kangjie Lu, kvm, open list, aik, david, Wenwen Wang
On Wed, Oct 17, 2018 at 10:45 AM Alex Williamson
<alex.williamson@redhat.com> wrote:
>
> On Wed, 17 Oct 2018 09:32:04 -0500
> Wenwen Wang <wang6495@umn.edu> wrote:
>
> > In vfio_spapr_iommu_eeh_ioctl(), if the ioctl command is VFIO_EEH_PE_OP,
> > the user-space buffer 'arg' is copied to the kernel object 'op' and the
> > 'argsz' and 'flags' fields of 'op' are checked. If the check fails, an
> > error code EINVAL is returned. Otherwise, 'op.op' is further checked
> > through a switch statement to invoke related handlers. If 'op.op' is
> > VFIO_EEH_PE_INJECT_ERR, the whole user-space buffer 'arg' is copied again
> > to 'op' to obtain the err information. However, in the following execution
> > of this case, the fields of 'op', except the field 'err', are actually not
> > used. That is, the second copy has a redundant part. Therefore, for both
> > performance consideration, the redundant part of the second copy should be
> > removed.
> >
> > This patch removes such a part in the second copy. It only copies from
> > 'err.type' to 'err.mask', which is exactly required by the
> > VFIO_EEH_PE_INJECT_ERR op.
> >
> > This patch also adds a 4-byte reserved field in the structure
> > vfio_eeh_pe_op to make sure that the u64 fields in the structure
> > vfio_eeh_pe_err are 8-byte aligned.
> >
> > Signed-off-by: Wenwen Wang <wang6495@umn.edu>
> > ---
> > drivers/vfio/vfio_spapr_eeh.c | 9 ++++++---
> > include/uapi/linux/vfio.h | 1 +
> > 2 files changed, 7 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/vfio/vfio_spapr_eeh.c b/drivers/vfio/vfio_spapr_eeh.c
> > index 38edeb4..66634c6 100644
> > --- a/drivers/vfio/vfio_spapr_eeh.c
> > +++ b/drivers/vfio/vfio_spapr_eeh.c
> > @@ -37,6 +37,7 @@ long vfio_spapr_iommu_eeh_ioctl(struct iommu_group *group,
> > struct eeh_pe *pe;
> > struct vfio_eeh_pe_op op;
> > unsigned long minsz;
> > + unsigned long start, end;
> > long ret = -EINVAL;
> >
> > switch (cmd) {
> > @@ -86,10 +87,12 @@ long vfio_spapr_iommu_eeh_ioctl(struct iommu_group *group,
> > ret = eeh_pe_configure(pe);
> > break;
> > case VFIO_EEH_PE_INJECT_ERR:
> > - minsz = offsetofend(struct vfio_eeh_pe_op, err.mask);
> > - if (op.argsz < minsz)
> > + start = offsetof(struct vfio_eeh_pe_op, err.type);
>
> I noted in the previous version that we already have this in minsz, so
> you're fixing a redundant copy with a redundant operation.
The value in start is different from the value in minsz. So why is
this a redundant operation?
> > + end = offsetofend(struct vfio_eeh_pe_op, err.mask);
> > + if (op.argsz < end)
> > return -EINVAL;
> > - if (copy_from_user(&op, (void __user *)arg, minsz))
> > + if (copy_from_user(&op.err, (char __user *)arg +
> > + start, end - start))
> > return -EFAULT;
> >
> > ret = eeh_pe_inject_err(pe, op.err.type, op.err.func,
> > diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
> > index 1aa7b82..d904c42 100644
> > --- a/include/uapi/linux/vfio.h
> > +++ b/include/uapi/linux/vfio.h
> > @@ -729,6 +729,7 @@ struct vfio_eeh_pe_op {
> > __u32 argsz;
> > __u32 flags;
> > __u32 op;
> > + __u32 __resv;
> > union {
> > struct vfio_eeh_pe_err err;
> > };
>
> Please don't include two separate issues in the same patch. Am I also
> correct in assuming that this is untested? Thanks,
No problem. I will seperate these two patches. And yes, this is not tested.
Thanks,
Wenwen
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v3] drivers/vfio: Fix a redundant copy bug
2018-10-17 17:58 ` Wenwen Wang
@ 2018-10-17 19:05 ` Alex Williamson
2018-10-17 19:10 ` Wenwen Wang
0 siblings, 1 reply; 5+ messages in thread
From: Alex Williamson @ 2018-10-17 19:05 UTC (permalink / raw)
To: Wenwen Wang; +Cc: Kangjie Lu, kvm, open list, aik, david
On Wed, 17 Oct 2018 12:58:26 -0500
Wenwen Wang <wang6495@umn.edu> wrote:
> On Wed, Oct 17, 2018 at 10:45 AM Alex Williamson
> <alex.williamson@redhat.com> wrote:
> >
> > On Wed, 17 Oct 2018 09:32:04 -0500
> > Wenwen Wang <wang6495@umn.edu> wrote:
> >
> > > In vfio_spapr_iommu_eeh_ioctl(), if the ioctl command is VFIO_EEH_PE_OP,
> > > the user-space buffer 'arg' is copied to the kernel object 'op' and the
> > > 'argsz' and 'flags' fields of 'op' are checked. If the check fails, an
> > > error code EINVAL is returned. Otherwise, 'op.op' is further checked
> > > through a switch statement to invoke related handlers. If 'op.op' is
> > > VFIO_EEH_PE_INJECT_ERR, the whole user-space buffer 'arg' is copied again
> > > to 'op' to obtain the err information. However, in the following execution
> > > of this case, the fields of 'op', except the field 'err', are actually not
> > > used. That is, the second copy has a redundant part. Therefore, for both
> > > performance consideration, the redundant part of the second copy should be
> > > removed.
> > >
> > > This patch removes such a part in the second copy. It only copies from
> > > 'err.type' to 'err.mask', which is exactly required by the
> > > VFIO_EEH_PE_INJECT_ERR op.
> > >
> > > This patch also adds a 4-byte reserved field in the structure
> > > vfio_eeh_pe_op to make sure that the u64 fields in the structure
> > > vfio_eeh_pe_err are 8-byte aligned.
> > >
> > > Signed-off-by: Wenwen Wang <wang6495@umn.edu>
> > > ---
> > > drivers/vfio/vfio_spapr_eeh.c | 9 ++++++---
> > > include/uapi/linux/vfio.h | 1 +
> > > 2 files changed, 7 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/vfio/vfio_spapr_eeh.c b/drivers/vfio/vfio_spapr_eeh.c
> > > index 38edeb4..66634c6 100644
> > > --- a/drivers/vfio/vfio_spapr_eeh.c
> > > +++ b/drivers/vfio/vfio_spapr_eeh.c
> > > @@ -37,6 +37,7 @@ long vfio_spapr_iommu_eeh_ioctl(struct iommu_group *group,
> > > struct eeh_pe *pe;
> > > struct vfio_eeh_pe_op op;
> > > unsigned long minsz;
> > > + unsigned long start, end;
> > > long ret = -EINVAL;
> > >
> > > switch (cmd) {
> > > @@ -86,10 +87,12 @@ long vfio_spapr_iommu_eeh_ioctl(struct iommu_group *group,
> > > ret = eeh_pe_configure(pe);
> > > break;
> > > case VFIO_EEH_PE_INJECT_ERR:
> > > - minsz = offsetofend(struct vfio_eeh_pe_op, err.mask);
> > > - if (op.argsz < minsz)
> > > + start = offsetof(struct vfio_eeh_pe_op, err.type);
> >
> > I noted in the previous version that we already have this in minsz, so
> > you're fixing a redundant copy with a redundant operation.
>
> The value in start is different from the value in minsz. So why is
> this a redundant operation?
I suppose that's true given the alignment issue below, so we're
actually avoiding 16 bytes rather than 12. The benefit of this change
still seems pretty thin to me, but it is more correct, so I guess it's
ok. Do you want to send a new version or shall I just drop the vfio.h
changes and the last paragraph of the commit log in favor of the
separate patch? Alexey or David, do you want to provide an Ack for
these? Thanks,
Alex
> > > + end = offsetofend(struct vfio_eeh_pe_op, err.mask);
> > > + if (op.argsz < end)
> > > return -EINVAL;
> > > - if (copy_from_user(&op, (void __user *)arg, minsz))
> > > + if (copy_from_user(&op.err, (char __user *)arg +
> > > + start, end - start))
> > > return -EFAULT;
> > >
> > > ret = eeh_pe_inject_err(pe, op.err.type, op.err.func,
> > > diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
> > > index 1aa7b82..d904c42 100644
> > > --- a/include/uapi/linux/vfio.h
> > > +++ b/include/uapi/linux/vfio.h
> > > @@ -729,6 +729,7 @@ struct vfio_eeh_pe_op {
> > > __u32 argsz;
> > > __u32 flags;
> > > __u32 op;
> > > + __u32 __resv;
> > > union {
> > > struct vfio_eeh_pe_err err;
> > > };
> >
> > Please don't include two separate issues in the same patch. Am I also
> > correct in assuming that this is untested? Thanks,
>
> No problem. I will seperate these two patches. And yes, this is not tested.
>
> Thanks,
> Wenwen
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v3] drivers/vfio: Fix a redundant copy bug
2018-10-17 19:05 ` Alex Williamson
@ 2018-10-17 19:10 ` Wenwen Wang
0 siblings, 0 replies; 5+ messages in thread
From: Wenwen Wang @ 2018-10-17 19:10 UTC (permalink / raw)
To: alex.williamson; +Cc: Kangjie Lu, kvm, open list, aik, david, Wenwen Wang
On Wed, Oct 17, 2018 at 2:05 PM Alex Williamson
<alex.williamson@redhat.com> wrote:
>
> On Wed, 17 Oct 2018 12:58:26 -0500
> Wenwen Wang <wang6495@umn.edu> wrote:
>
> > On Wed, Oct 17, 2018 at 10:45 AM Alex Williamson
> > <alex.williamson@redhat.com> wrote:
> > >
> > > On Wed, 17 Oct 2018 09:32:04 -0500
> > > Wenwen Wang <wang6495@umn.edu> wrote:
> > >
> > > > In vfio_spapr_iommu_eeh_ioctl(), if the ioctl command is VFIO_EEH_PE_OP,
> > > > the user-space buffer 'arg' is copied to the kernel object 'op' and the
> > > > 'argsz' and 'flags' fields of 'op' are checked. If the check fails, an
> > > > error code EINVAL is returned. Otherwise, 'op.op' is further checked
> > > > through a switch statement to invoke related handlers. If 'op.op' is
> > > > VFIO_EEH_PE_INJECT_ERR, the whole user-space buffer 'arg' is copied again
> > > > to 'op' to obtain the err information. However, in the following execution
> > > > of this case, the fields of 'op', except the field 'err', are actually not
> > > > used. That is, the second copy has a redundant part. Therefore, for both
> > > > performance consideration, the redundant part of the second copy should be
> > > > removed.
> > > >
> > > > This patch removes such a part in the second copy. It only copies from
> > > > 'err.type' to 'err.mask', which is exactly required by the
> > > > VFIO_EEH_PE_INJECT_ERR op.
> > > >
> > > > This patch also adds a 4-byte reserved field in the structure
> > > > vfio_eeh_pe_op to make sure that the u64 fields in the structure
> > > > vfio_eeh_pe_err are 8-byte aligned.
> > > >
> > > > Signed-off-by: Wenwen Wang <wang6495@umn.edu>
> > > > ---
> > > > drivers/vfio/vfio_spapr_eeh.c | 9 ++++++---
> > > > include/uapi/linux/vfio.h | 1 +
> > > > 2 files changed, 7 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/drivers/vfio/vfio_spapr_eeh.c b/drivers/vfio/vfio_spapr_eeh.c
> > > > index 38edeb4..66634c6 100644
> > > > --- a/drivers/vfio/vfio_spapr_eeh.c
> > > > +++ b/drivers/vfio/vfio_spapr_eeh.c
> > > > @@ -37,6 +37,7 @@ long vfio_spapr_iommu_eeh_ioctl(struct iommu_group *group,
> > > > struct eeh_pe *pe;
> > > > struct vfio_eeh_pe_op op;
> > > > unsigned long minsz;
> > > > + unsigned long start, end;
> > > > long ret = -EINVAL;
> > > >
> > > > switch (cmd) {
> > > > @@ -86,10 +87,12 @@ long vfio_spapr_iommu_eeh_ioctl(struct iommu_group *group,
> > > > ret = eeh_pe_configure(pe);
> > > > break;
> > > > case VFIO_EEH_PE_INJECT_ERR:
> > > > - minsz = offsetofend(struct vfio_eeh_pe_op, err.mask);
> > > > - if (op.argsz < minsz)
> > > > + start = offsetof(struct vfio_eeh_pe_op, err.type);
> > >
> > > I noted in the previous version that we already have this in minsz, so
> > > you're fixing a redundant copy with a redundant operation.
> >
> > The value in start is different from the value in minsz. So why is
> > this a redundant operation?
>
> I suppose that's true given the alignment issue below, so we're
> actually avoiding 16 bytes rather than 12. The benefit of this change
> still seems pretty thin to me, but it is more correct, so I guess it's
> ok. Do you want to send a new version or shall I just drop the vfio.h
> changes and the last paragraph of the commit log in favor of the
> separate patch? Alexey or David, do you want to provide an Ack for
> these? Thanks,
I can send a new version of the patch. Thanks!
Wenwen
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2018-10-17 19:14 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-10-17 14:32 [PATCH v3] drivers/vfio: Fix a redundant copy bug Wenwen Wang
2018-10-17 15:45 ` Alex Williamson
2018-10-17 17:58 ` Wenwen Wang
2018-10-17 19:05 ` Alex Williamson
2018-10-17 19:10 ` Wenwen Wang
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).