Re: [PATCH v2 1/1] migration/dirtyrate: Fix segmentation fault

[Yong Huang <yong.huang@smartx.com>]

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

On Wed, Apr 24, 2024 at 3:17 PM Zhijian Li (Fujitsu) <lizhijian@fujitsu.com>
wrote:

>
>
> On 24/04/2024 12:52, mii wrote:
> >
> > On 2024/04/24 10:28, Yong Huang wrote:
> >>
> >>
> >> On Tue, Apr 23, 2024 at 9:35 PM Peter Xu <peterx@redhat.com> wrote:
> >>
> >>     On Tue, Apr 23, 2024 at 09:13:08AM +0000, Masato Imai wrote:
> >>     > When the KVM acceleration parameter is not set, executing
> calc_dirty_rate
> >>     > with the -r or -b option results in a segmentation fault due to
> accessing
> >>     > a null kvm_state pointer in the kvm_dirty_ring_enabled function.
> >>     > This commit adds a check for kvm_enabled to prevent segmentation
> faults.
> >>     >
> >>     > Signed-off-by: Masato Imai <mii@sfc.wide.ad.jp>
> >>     > ---
> >>     >  migration/dirtyrate.c | 7 +++++++
> >>     >  1 file changed, 7 insertions(+)
> >>     >
> >>     > diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
> >>     > index 1d2e85746f..2a7df52519 100644
> >>     > --- a/migration/dirtyrate.c
> >>     > +++ b/migration/dirtyrate.c
> >>     > @@ -799,6 +799,13 @@ void qmp_calc_dirty_rate(int64_t calc_time,
> >>     >       * dirty ring mode only works when kvm dirty ring is enabled.
> >>     >       * on the contrary, dirty bitmap mode is not.
> >>     >       */
> >>     > +    if (!kvm_enabled() &&
> >>     > +        (mode == DIRTY_RATE_MEASURE_MODE_DIRTY_RING ||
> >>     > +         mode == DIRTY_RATE_MEASURE_MODE_DIRTY_BITMAP)) {
> >>     > +        error_setg(errp, "mode %s requires kvm to be enabled.",
> >>     > +  DirtyRateMeasureMode_str(mode));
> >>     > +        return;
> >>     > +    }
> >>
> >>     Logically dirty bitmap should work with tcg.  So the other option
> is to let
> >>     kvm_dirty_ring_enabled() check kvm_state too and return false if
> >>     kvm_state==NULL?
> >>
> >>
> >> Agree, better solution
> >>
> >>
> >>     >      if (((mode == DIRTY_RATE_MEASURE_MODE_DIRTY_RING) &&
> >>     >          !kvm_dirty_ring_enabled()) ||
> >>     >          ((mode == DIRTY_RATE_MEASURE_MODE_DIRTY_BITMAP) &&
> >>     > --
> >>     > 2.34.1
> >>     >
> >>
> >>     --
> >>     Peter Xu
> >>
> >>
> >> Thanks,
> >> Yong
> >>
> >>
> >> --
> >> Best regards
> >
> > Thank you for the review. I agree with that solution.
> >
> > Update will be like:
> >
> > diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> > index 931f74256e..0f8499365d 100644
> > --- a/accel/kvm/kvm-all.c
> > +++ b/accel/kvm/kvm-all.c
> > @@ -2312,6 +2312,9 @@ bool kvm_vcpu_id_is_valid(int vcpu_id)
> >
> >   bool kvm_dirty_ring_enabled(void)
> >   {
>
>
> How about,
>
>         return kvm_state && vm_state->kvm_dirty_ring_size;
>

Hi, Masato Imai, would you like to continue this patch with version 3
as suggested?


>
>
> Thanks
> Zhijian
>
>
> > +    if (kvm_state == NULL) {
> > +        return false;
> > +    }
> >       return kvm_state->kvm_dirty_ring_size ? true : false;
> >   }
> >


Thanks,
Yong


-- 
Best regards

[-- Attachment #2: Type: text/html, Size: 5365 bytes --]