* [PATCH v4] drivers/vfio: Fix a redundant copy bug @ 2018-10-17 19:18 Wenwen Wang 2018-10-29 18:56 ` Wenwen Wang 0 siblings, 1 reply; 5+ messages in thread From: Wenwen Wang @ 2018-10-17 19:18 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 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. Signed-off-by: Wenwen Wang <wang6495@umn.edu> --- drivers/vfio/vfio_spapr_eeh.c | 9 ++++++--- 1 file changed, 6 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, -- 2.7.4 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v4] drivers/vfio: Fix a redundant copy bug 2018-10-17 19:18 [PATCH v4] drivers/vfio: Fix a redundant copy bug Wenwen Wang @ 2018-10-29 18:56 ` Wenwen Wang 2018-10-29 21:32 ` Alex Williamson 0 siblings, 1 reply; 5+ messages in thread From: Wenwen Wang @ 2018-10-29 18:56 UTC (permalink / raw) To: Wenwen Wang; +Cc: Kangjie Lu, alex.williamson, kvm, open list Hello, Could you please apply this patch? Thanks! Wenwen On Wed, Oct 17, 2018 at 2:18 PM 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 > 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. > > Signed-off-by: Wenwen Wang <wang6495@umn.edu> > --- > drivers/vfio/vfio_spapr_eeh.c | 9 ++++++--- > 1 file changed, 6 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, > -- > 2.7.4 > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v4] drivers/vfio: Fix a redundant copy bug 2018-10-29 18:56 ` Wenwen Wang @ 2018-10-29 21:32 ` Alex Williamson 2018-10-29 21:35 ` Wenwen Wang 2018-11-19 6:41 ` David Gibson 0 siblings, 2 replies; 5+ messages in thread From: Alex Williamson @ 2018-10-29 21:32 UTC (permalink / raw) To: Wenwen Wang; +Cc: Kangjie Lu, kvm, open list, aik, David Gibson On Mon, 29 Oct 2018 13:56:54 -0500 Wenwen Wang <wang6495@umn.edu> wrote: > Hello, > > Could you please apply this patch? Thanks! I'd like to see testing and/or review from David or Alexey since I also don't have an environment for spapr/eeh. We're already late into the v4.20 merge window so this is probably v4.21 material. Thanks, Alex > On Wed, Oct 17, 2018 at 2:18 PM 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 > > 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. > > > > Signed-off-by: Wenwen Wang <wang6495@umn.edu> > > --- > > drivers/vfio/vfio_spapr_eeh.c | 9 ++++++--- > > 1 file changed, 6 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, > > -- > > 2.7.4 > > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v4] drivers/vfio: Fix a redundant copy bug 2018-10-29 21:32 ` Alex Williamson @ 2018-10-29 21:35 ` Wenwen Wang 2018-11-19 6:41 ` David Gibson 1 sibling, 0 replies; 5+ messages in thread From: Wenwen Wang @ 2018-10-29 21:35 UTC (permalink / raw) To: alex.williamson; +Cc: Kangjie Lu, kvm, open list, aik, david, Wenwen Wang On Mon, Oct 29, 2018 at 4:32 PM Alex Williamson <alex.williamson@redhat.com> wrote: > > On Mon, 29 Oct 2018 13:56:54 -0500 > Wenwen Wang <wang6495@umn.edu> wrote: > > > Hello, > > > > Could you please apply this patch? Thanks! > > I'd like to see testing and/or review from David or Alexey since I also > don't have an environment for spapr/eeh. We're already late into the > v4.20 merge window so this is probably v4.21 material. Thanks, I see. Thanks for your response, Alex! > > On Wed, Oct 17, 2018 at 2:18 PM 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 > > > 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. > > > > > > Signed-off-by: Wenwen Wang <wang6495@umn.edu> > > > --- > > > drivers/vfio/vfio_spapr_eeh.c | 9 ++++++--- > > > 1 file changed, 6 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, > > > -- > > > 2.7.4 > > > > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v4] drivers/vfio: Fix a redundant copy bug 2018-10-29 21:32 ` Alex Williamson 2018-10-29 21:35 ` Wenwen Wang @ 2018-11-19 6:41 ` David Gibson 1 sibling, 0 replies; 5+ messages in thread From: David Gibson @ 2018-11-19 6:41 UTC (permalink / raw) To: Alex Williamson; +Cc: Wenwen Wang, Kangjie Lu, kvm, open list, aik [-- Attachment #1: Type: text/plain, Size: 4001 bytes --] On Mon, Oct 29, 2018 at 03:32:34PM -0600, Alex Williamson wrote: > On Mon, 29 Oct 2018 13:56:54 -0500 > Wenwen Wang <wang6495@umn.edu> wrote: > > > Hello, > > > > Could you please apply this patch? Thanks! > > I'd like to see testing and/or review from David or Alexey since I also > don't have an environment for spapr/eeh. We're already late into the > v4.20 merge window so this is probably v4.21 material. Thanks, I didn't spot this for ages, since I wasn't CCed, I'm guessing the same is true for Alexey. TBH, I don't think it's worth bothering with this. It's a tiny amount of extra work on a rare debug path. A couple of code lines simplicity is worth more than a few bytes worth of redundant copy here. Testing is.. difficult, since the EEH is already so broken it's hard to know what to compare against. Sam Bobroff is already looking medium-long term at a bunch of EEH rework, so it's quite possible this will be rewritten then anyway. > > Alex > > > On Wed, Oct 17, 2018 at 2:18 PM 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 > > > 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. > > > > > > Signed-off-by: Wenwen Wang <wang6495@umn.edu> > > > --- > > > drivers/vfio/vfio_spapr_eeh.c | 9 ++++++--- > > > 1 file changed, 6 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, > -- 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: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2018-11-19 6:41 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-10-17 19:18 [PATCH v4] drivers/vfio: Fix a redundant copy bug Wenwen Wang 2018-10-29 18:56 ` Wenwen Wang 2018-10-29 21:32 ` Alex Williamson 2018-10-29 21:35 ` Wenwen Wang 2018-11-19 6:41 ` David Gibson
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).