| Message ID | 20180427123613.20624-4-david@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 04/27/2018 08:36 AM, David Hildenbrand wrote: > Move the Multiple-epoch facility handling into it and rename it to > kvm_s390_get_tod_clock(). > > This leaves us with: > - kvm_s390_set_tod_clock() > - kvm_s390_get_tod_clock() > - kvm_s390_get_tod_clock_fast() > > So all Multiple-epoch facility is hidden in these functions. > > Signed-off-by: David Hildenbrand <david@redhat.com> Aside from the one nit described below, looks good to me! Reviewed-by: Collin Walling <walling@linux.ibm.com> > --- > arch/s390/kvm/kvm-s390.c | 22 +++++++++------------- > 1 file changed, 9 insertions(+), 13 deletions(-) > > diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c > index b3eee3cc6e78..fb753b9439fa 100644 > --- a/arch/s390/kvm/kvm-s390.c > +++ b/arch/s390/kvm/kvm-s390.c > @@ -1033,8 +1033,8 @@ static int kvm_s390_set_tod(struct kvm *kvm, struct kvm_device_attr *attr) > return ret; > } > > -static void kvm_s390_get_tod_clock_ext(struct kvm *kvm, > - struct kvm_s390_vm_tod_clock *gtod) > +static void kvm_s390_get_tod_clock(struct kvm *kvm, > + struct kvm_s390_vm_tod_clock *gtod) Nit: add one more whitespace for the second line of parameters > { > struct kvm_s390_tod_clock_ext htod; > > @@ -1043,10 +1043,12 @@ static void kvm_s390_get_tod_clock_ext(struct kvm *kvm, > get_tod_clock_ext((char *)&htod); > > gtod->tod = htod.tod + kvm->arch.epoch; > - gtod->epoch_idx = htod.epoch_idx + kvm->arch.epdx; > - > - if (gtod->tod < htod.tod) > - gtod->epoch_idx += 1; > + gtod->epoch_idx = 0; > + if (test_kvm_facility(kvm, 139)) { > + gtod->epoch_idx = htod.epoch_idx + kvm->arch.epdx; > + if (gtod->tod < htod.tod) > + gtod->epoch_idx += 1; > + } > > preempt_enable(); > } > @@ -1055,13 +1057,7 @@ static int kvm_s390_get_tod_ext(struct kvm *kvm, struct kvm_device_attr *attr) > { > struct kvm_s390_vm_tod_clock gtod; > > - memset(>od, 0, sizeof(gtod)); > - > - if (test_kvm_facility(kvm, 139)) > - kvm_s390_get_tod_clock_ext(kvm, >od); > - else > - gtod.tod = kvm_s390_get_tod_clock_fast(kvm); > - > + kvm_s390_get_tod_clock(kvm, >od); > if (copy_to_user((void __user *)attr->addr, >od, sizeof(gtod))) > return -EFAULT; > >
On Fri, 27 Apr 2018 14:36:13 +0200 David Hildenbrand <david@redhat.com> wrote: > Move the Multiple-epoch facility handling into it and rename it to > kvm_s390_get_tod_clock(). > > This leaves us with: > - kvm_s390_set_tod_clock() > - kvm_s390_get_tod_clock() > - kvm_s390_get_tod_clock_fast() > > So all Multiple-epoch facility is hidden in these functions. > > Signed-off-by: David Hildenbrand <david@redhat.com> > --- > arch/s390/kvm/kvm-s390.c | 22 +++++++++------------- > 1 file changed, 9 insertions(+), 13 deletions(-) Reviewed-by: Cornelia Huck <cohuck@redhat.com>
On 27.04.2018 17:58, Collin Walling wrote: > On 04/27/2018 08:36 AM, David Hildenbrand wrote: >> Move the Multiple-epoch facility handling into it and rename it to >> kvm_s390_get_tod_clock(). >> >> This leaves us with: >> - kvm_s390_set_tod_clock() >> - kvm_s390_get_tod_clock() >> - kvm_s390_get_tod_clock_fast() >> >> So all Multiple-epoch facility is hidden in these functions. >> >> Signed-off-by: David Hildenbrand <david@redhat.com> > > Aside from the one nit described below, looks good to me! > > Reviewed-by: Collin Walling <walling@linux.ibm.com> > >> --- >> arch/s390/kvm/kvm-s390.c | 22 +++++++++------------- >> 1 file changed, 9 insertions(+), 13 deletions(-) >> >> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c >> index b3eee3cc6e78..fb753b9439fa 100644 >> --- a/arch/s390/kvm/kvm-s390.c >> +++ b/arch/s390/kvm/kvm-s390.c >> @@ -1033,8 +1033,8 @@ static int kvm_s390_set_tod(struct kvm *kvm, struct kvm_device_attr *attr) >> return ret; >> } >> >> -static void kvm_s390_get_tod_clock_ext(struct kvm *kvm, >> - struct kvm_s390_vm_tod_clock *gtod) >> +static void kvm_s390_get_tod_clock(struct kvm *kvm, >> + struct kvm_s390_vm_tod_clock *gtod) > > Nit: add one more whitespace for the second line of parameters It only looks like it's wrong in the patch file. The end result is correct. Thanks!
On 04/27/2018 02:36 PM, David Hildenbrand wrote: > Move the Multiple-epoch facility handling into it and rename it to [..] > @@ -1055,13 +1057,7 @@ static int kvm_s390_get_tod_ext(struct kvm *kvm, struct kvm_device_attr *attr) > { > struct kvm_s390_vm_tod_clock gtod; > > - memset(>od, 0, sizeof(gtod)); We should keep the memset, otherwise the padding might leak kernel info as we copy . (found by smatch) arch/s390/kvm/kvm-s390.c:1068 kvm_s390_get_tod_ext() warn: check that 'gtod' doesn't leak information (struct has a hole after 'epoch_idx') > - > - if (test_kvm_facility(kvm, 139)) > - kvm_s390_get_tod_clock_ext(kvm, >od); > - else > - gtod.tod = kvm_s390_get_tod_clock_fast(kvm); > - > + kvm_s390_get_tod_clock(kvm, >od); > if (copy_to_user((void __user *)attr->addr, >od, sizeof(gtod))) > return -EFAULT; > >
On 02.05.2018 21:36, Christian Borntraeger wrote: > > > On 04/27/2018 02:36 PM, David Hildenbrand wrote: >> Move the Multiple-epoch facility handling into it and rename it to > [..] > > >> @@ -1055,13 +1057,7 @@ static int kvm_s390_get_tod_ext(struct kvm *kvm, struct kvm_device_attr *attr) >> { >> struct kvm_s390_vm_tod_clock gtod; >> >> - memset(>od, 0, sizeof(gtod)); > > We should keep the memset, otherwise the padding might leak kernel info as we copy . > (found by smatch) Thanks for the info and running the checker. @Janosch please tell me if I should resend! > > > arch/s390/kvm/kvm-s390.c:1068 kvm_s390_get_tod_ext() warn: check that 'gtod' doesn't leak information (struct has a hole after 'epoch_idx') > > >> - >> - if (test_kvm_facility(kvm, 139)) >> - kvm_s390_get_tod_clock_ext(kvm, >od); >> - else >> - gtod.tod = kvm_s390_get_tod_clock_fast(kvm); >> - >> + kvm_s390_get_tod_clock(kvm, >od); >> if (copy_to_user((void __user *)attr->addr, >od, sizeof(gtod))) >> return -EFAULT; >> >> >
On 02.05.2018 21:40, David Hildenbrand wrote: > On 02.05.2018 21:36, Christian Borntraeger wrote: >> >> >> On 04/27/2018 02:36 PM, David Hildenbrand wrote: >>> Move the Multiple-epoch facility handling into it and rename it to >> [..] >> >> >>> @@ -1055,13 +1057,7 @@ static int kvm_s390_get_tod_ext(struct kvm *kvm, struct kvm_device_attr *attr) >>> { >>> struct kvm_s390_vm_tod_clock gtod; >>> >>> - memset(>od, 0, sizeof(gtod)); >> >> We should keep the memset, otherwise the padding might leak kernel info as we copy . >> (found by smatch) > > Thanks for the info and running the checker. > > @Janosch please tell me if I should resend! I'll fix that, no need to resend. > >> >> >> arch/s390/kvm/kvm-s390.c:1068 kvm_s390_get_tod_ext() warn: check that 'gtod' doesn't leak information (struct has a hole after 'epoch_idx') >> >> >>> - >>> - if (test_kvm_facility(kvm, 139)) >>> - kvm_s390_get_tod_clock_ext(kvm, >od); >>> - else >>> - gtod.tod = kvm_s390_get_tod_clock_fast(kvm); >>> - >>> + kvm_s390_get_tod_clock(kvm, >od); >>> if (copy_to_user((void __user *)attr->addr, >od, sizeof(gtod))) >>> return -EFAULT; >>> >>> >> > >
diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c index b3eee3cc6e78..fb753b9439fa 100644 --- a/arch/s390/kvm/kvm-s390.c +++ b/arch/s390/kvm/kvm-s390.c @@ -1033,8 +1033,8 @@ static int kvm_s390_set_tod(struct kvm *kvm, struct kvm_device_attr *attr) return ret; } -static void kvm_s390_get_tod_clock_ext(struct kvm *kvm, - struct kvm_s390_vm_tod_clock *gtod) +static void kvm_s390_get_tod_clock(struct kvm *kvm, + struct kvm_s390_vm_tod_clock *gtod) { struct kvm_s390_tod_clock_ext htod; @@ -1043,10 +1043,12 @@ static void kvm_s390_get_tod_clock_ext(struct kvm *kvm, get_tod_clock_ext((char *)&htod); gtod->tod = htod.tod + kvm->arch.epoch; - gtod->epoch_idx = htod.epoch_idx + kvm->arch.epdx; - - if (gtod->tod < htod.tod) - gtod->epoch_idx += 1; + gtod->epoch_idx = 0; + if (test_kvm_facility(kvm, 139)) { + gtod->epoch_idx = htod.epoch_idx + kvm->arch.epdx; + if (gtod->tod < htod.tod) + gtod->epoch_idx += 1; + } preempt_enable(); } @@ -1055,13 +1057,7 @@ static int kvm_s390_get_tod_ext(struct kvm *kvm, struct kvm_device_attr *attr) { struct kvm_s390_vm_tod_clock gtod; - memset(>od, 0, sizeof(gtod)); - - if (test_kvm_facility(kvm, 139)) - kvm_s390_get_tod_clock_ext(kvm, >od); - else - gtod.tod = kvm_s390_get_tod_clock_fast(kvm); - + kvm_s390_get_tod_clock(kvm, >od); if (copy_to_user((void __user *)attr->addr, >od, sizeof(gtod))) return -EFAULT;
Move the Multiple-epoch facility handling into it and rename it to kvm_s390_get_tod_clock(). This leaves us with: - kvm_s390_set_tod_clock() - kvm_s390_get_tod_clock() - kvm_s390_get_tod_clock_fast() So all Multiple-epoch facility is hidden in these functions. Signed-off-by: David Hildenbrand <david@redhat.com> --- arch/s390/kvm/kvm-s390.c | 22 +++++++++------------- 1 file changed, 9 insertions(+), 13 deletions(-)