| Message ID | 1485377117-18105-1-git-send-email-thuth@redhat.com |
|---|---|
| State | New |
| Headers | show |
Le 25/01/2017 à 21:45, Thomas Huth a écrit : > When running QEMU with "-M none -device loader,file=kernel.elf", it > currently crashes with a segmentation fault, because the "none"-machine > does not have any CPU by default and the generic loader code tries > to dereference s->cpu. Fix it by adding an appropriate check for a > NULL pointer. > > Reported-by: Laurent Vivier <laurent@vivier.eu> > Signed-off-by: Thomas Huth <thuth@redhat.com> > --- > hw/core/generic-loader.c | 9 +++++---- > 1 file changed, 5 insertions(+), 4 deletions(-) > > diff --git a/hw/core/generic-loader.c b/hw/core/generic-loader.c > index 58f1f02..4601267 100644 > --- a/hw/core/generic-loader.c > +++ b/hw/core/generic-loader.c > @@ -137,20 +137,21 @@ static void generic_loader_realize(DeviceState *dev, Error **errp) > #endif > > if (s->file) { > + AddressSpace *as = s->cpu ? s->cpu->as : NULL; Should we just abort if s->cpu is NULL? Laurent
On Wed, Jan 25, 2017 at 12:52 PM, Laurent Vivier <laurent@vivier.eu> wrote: > Le 25/01/2017 à 21:45, Thomas Huth a écrit : >> When running QEMU with "-M none -device loader,file=kernel.elf", it >> currently crashes with a segmentation fault, because the "none"-machine >> does not have any CPU by default and the generic loader code tries >> to dereference s->cpu. Fix it by adding an appropriate check for a >> NULL pointer. >> >> Reported-by: Laurent Vivier <laurent@vivier.eu> >> Signed-off-by: Thomas Huth <thuth@redhat.com> >> --- >> hw/core/generic-loader.c | 9 +++++---- >> 1 file changed, 5 insertions(+), 4 deletions(-) >> >> diff --git a/hw/core/generic-loader.c b/hw/core/generic-loader.c >> index 58f1f02..4601267 100644 >> --- a/hw/core/generic-loader.c >> +++ b/hw/core/generic-loader.c >> @@ -137,20 +137,21 @@ static void generic_loader_realize(DeviceState *dev, Error **errp) >> #endif >> >> if (s->file) { >> + AddressSpace *as = s->cpu ? s->cpu->as : NULL; > > Should we just abort if s->cpu is NULL? I agree, what is the use case where you are loading images without a CPU? If there is a use case (maybe some KVM thing?) then this patch looks fine to me. Thanks, Alistair > > Laurent > >
On 26.01.2017 00:26, Alistair Francis wrote: > On Wed, Jan 25, 2017 at 12:52 PM, Laurent Vivier <laurent@vivier.eu> wrote: >> Le 25/01/2017 à 21:45, Thomas Huth a écrit : >>> When running QEMU with "-M none -device loader,file=kernel.elf", it >>> currently crashes with a segmentation fault, because the "none"-machine >>> does not have any CPU by default and the generic loader code tries >>> to dereference s->cpu. Fix it by adding an appropriate check for a >>> NULL pointer. >>> >>> Reported-by: Laurent Vivier <laurent@vivier.eu> >>> Signed-off-by: Thomas Huth <thuth@redhat.com> >>> --- >>> hw/core/generic-loader.c | 9 +++++---- >>> 1 file changed, 5 insertions(+), 4 deletions(-) >>> >>> diff --git a/hw/core/generic-loader.c b/hw/core/generic-loader.c >>> index 58f1f02..4601267 100644 >>> --- a/hw/core/generic-loader.c >>> +++ b/hw/core/generic-loader.c >>> @@ -137,20 +137,21 @@ static void generic_loader_realize(DeviceState *dev, Error **errp) >>> #endif >>> >>> if (s->file) { >>> + AddressSpace *as = s->cpu ? s->cpu->as : NULL; >> >> Should we just abort if s->cpu is NULL? > > I agree, what is the use case where you are loading images without a CPU? > > If there is a use case (maybe some KVM thing?) then this patch looks fine to me. I think there is no real use case yet. But this fix is 1) simpler than doing an error_report() + exit() here, and 2) maybe the vision of constructing machines on the fly with QEMU will eventually come true one day in the distant future, so with that patch here, the code would already be prepared for the case when QEMU gets started without CPUs and the CPUs are then later added via QOM... Well, I don't mind ... if you prefer an error message instead, feel free to suggest another patch. I'm fine as long as we do not simply crash with a segmentation fault here. Thomas
Le 26/01/2017 à 06:50, Thomas Huth a écrit : > On 26.01.2017 00:26, Alistair Francis wrote: >> On Wed, Jan 25, 2017 at 12:52 PM, Laurent Vivier <laurent@vivier.eu> wrote: >>> Le 25/01/2017 à 21:45, Thomas Huth a écrit : >>>> When running QEMU with "-M none -device loader,file=kernel.elf", it >>>> currently crashes with a segmentation fault, because the "none"-machine >>>> does not have any CPU by default and the generic loader code tries >>>> to dereference s->cpu. Fix it by adding an appropriate check for a >>>> NULL pointer. >>>> >>>> Reported-by: Laurent Vivier <laurent@vivier.eu> >>>> Signed-off-by: Thomas Huth <thuth@redhat.com> >>>> --- >>>> hw/core/generic-loader.c | 9 +++++---- >>>> 1 file changed, 5 insertions(+), 4 deletions(-) >>>> >>>> diff --git a/hw/core/generic-loader.c b/hw/core/generic-loader.c >>>> index 58f1f02..4601267 100644 >>>> --- a/hw/core/generic-loader.c >>>> +++ b/hw/core/generic-loader.c >>>> @@ -137,20 +137,21 @@ static void generic_loader_realize(DeviceState *dev, Error **errp) >>>> #endif >>>> >>>> if (s->file) { >>>> + AddressSpace *as = s->cpu ? s->cpu->as : NULL; >>> >>> Should we just abort if s->cpu is NULL? >> >> I agree, what is the use case where you are loading images without a CPU? >> >> If there is a use case (maybe some KVM thing?) then this patch looks fine to me. > > I think there is no real use case yet. But this fix is 1) simpler than > doing an error_report() + exit() here, and 2) maybe the vision of > constructing machines on the fly with QEMU will eventually come true one > day in the distant future, so with that patch here, the code would > already be prepared for the case when QEMU gets started without CPUs and > the CPUs are then later added via QOM... > Well, I don't mind ... if you prefer an error message instead, feel free > to suggest another patch. I'm fine as long as we do not simply crash > with a segmentation fault here. OK, the use of NULL as "as" seems reasonable (this is what uses load_elf()), so: Reviewed-by: Laurent Vivier <laurent@vivier.eu>
On Thu, Jan 26, 2017 at 12:38 AM, Laurent Vivier <laurent@vivier.eu> wrote: > Le 26/01/2017 à 06:50, Thomas Huth a écrit : >> On 26.01.2017 00:26, Alistair Francis wrote: >>> On Wed, Jan 25, 2017 at 12:52 PM, Laurent Vivier <laurent@vivier.eu> wrote: >>>> Le 25/01/2017 à 21:45, Thomas Huth a écrit : >>>>> When running QEMU with "-M none -device loader,file=kernel.elf", it >>>>> currently crashes with a segmentation fault, because the "none"-machine >>>>> does not have any CPU by default and the generic loader code tries >>>>> to dereference s->cpu. Fix it by adding an appropriate check for a >>>>> NULL pointer. >>>>> >>>>> Reported-by: Laurent Vivier <laurent@vivier.eu> >>>>> Signed-off-by: Thomas Huth <thuth@redhat.com> >>>>> --- >>>>> hw/core/generic-loader.c | 9 +++++---- >>>>> 1 file changed, 5 insertions(+), 4 deletions(-) >>>>> >>>>> diff --git a/hw/core/generic-loader.c b/hw/core/generic-loader.c >>>>> index 58f1f02..4601267 100644 >>>>> --- a/hw/core/generic-loader.c >>>>> +++ b/hw/core/generic-loader.c >>>>> @@ -137,20 +137,21 @@ static void generic_loader_realize(DeviceState *dev, Error **errp) >>>>> #endif >>>>> >>>>> if (s->file) { >>>>> + AddressSpace *as = s->cpu ? s->cpu->as : NULL; >>>> >>>> Should we just abort if s->cpu is NULL? >>> >>> I agree, what is the use case where you are loading images without a CPU? >>> >>> If there is a use case (maybe some KVM thing?) then this patch looks fine to me. >> >> I think there is no real use case yet. But this fix is 1) simpler than >> doing an error_report() + exit() here, and 2) maybe the vision of >> constructing machines on the fly with QEMU will eventually come true one >> day in the distant future, so with that patch here, the code would >> already be prepared for the case when QEMU gets started without CPUs and >> the CPUs are then later added via QOM... Ok, that is a good enough reason for me. Reviewed-by: Alistair Francis <alistair.francis@xilinx.com> Thanks, Alistair >> Well, I don't mind ... if you prefer an error message instead, feel free >> to suggest another patch. I'm fine as long as we do not simply crash >> with a segmentation fault here. > > OK, the use of NULL as "as" seems reasonable (this is what uses > load_elf()), so: > > Reviewed-by: Laurent Vivier <laurent@vivier.eu> > > >
On 26 January 2017 at 05:50, Thomas Huth <thuth@redhat.com> wrote: > I think there is no real use case yet. But this fix is 1) simpler than > doing an error_report() + exit() here, and 2) maybe the vision of > constructing machines on the fly with QEMU will eventually come true one > day in the distant future, so with that patch here, the code would > already be prepared for the case when QEMU gets started without CPUs and > the CPUs are then later added via QOM... For the latter to work you'd need to do something more anyway, because you still need to be able to say "load this image into this CPU's address space", even if the CPUs are getting added later via QOM. You'd need to say "delay the image load until we actually have a CPU to load it for" or something... Passing a NULL address space to load_elf_as() etc is saying "I am definitely a board which has system_address_space as its one and only address space", which is OK if you're a board model but a bit more dubious if you're a generic device like this one. thanks -- PMM
On 25.01.2017 21:45, Thomas Huth wrote: > When running QEMU with "-M none -device loader,file=kernel.elf", it > currently crashes with a segmentation fault, because the "none"-machine > does not have any CPU by default and the generic loader code tries > to dereference s->cpu. Fix it by adding an appropriate check for a > NULL pointer. > > Reported-by: Laurent Vivier <laurent@vivier.eu> > Signed-off-by: Thomas Huth <thuth@redhat.com> > --- > hw/core/generic-loader.c | 9 +++++---- > 1 file changed, 5 insertions(+), 4 deletions(-) > > diff --git a/hw/core/generic-loader.c b/hw/core/generic-loader.c > index 58f1f02..4601267 100644 > --- a/hw/core/generic-loader.c > +++ b/hw/core/generic-loader.c > @@ -137,20 +137,21 @@ static void generic_loader_realize(DeviceState *dev, Error **errp) > #endif > > if (s->file) { > + AddressSpace *as = s->cpu ? s->cpu->as : NULL; > + > if (!s->force_raw) { > size = load_elf_as(s->file, NULL, NULL, &entry, NULL, NULL, > - big_endian, 0, 0, 0, s->cpu->as); > + big_endian, 0, 0, 0, as); > > if (size < 0) { > size = load_uimage_as(s->file, &entry, NULL, NULL, NULL, NULL, > - s->cpu->as); > + as); > } > } > > if (size < 0 || s->force_raw) { > /* Default to the maximum size being the machine's ram size */ > - size = load_image_targphys_as(s->file, s->addr, ram_size, > - s->cpu->as); > + size = load_image_targphys_as(s->file, s->addr, ram_size, as); > } else { > s->addr = entry; > } Ping? Thomas
On Fri, Jan 27, 2017 at 9:06 AM, Peter Maydell <peter.maydell@linaro.org> wrote: > On 26 January 2017 at 05:50, Thomas Huth <thuth@redhat.com> wrote: >> I think there is no real use case yet. But this fix is 1) simpler than >> doing an error_report() + exit() here, and 2) maybe the vision of >> constructing machines on the fly with QEMU will eventually come true one >> day in the distant future, so with that patch here, the code would >> already be prepared for the case when QEMU gets started without CPUs and >> the CPUs are then later added via QOM... > > For the latter to work you'd need to do something more anyway, > because you still need to be able to say "load this image > into this CPU's address space", even if the CPUs are getting > added later via QOM. You'd need to say "delay the image > load until we actually have a CPU to load it for" or > something... Passing a NULL address space to load_elf_as() > etc is saying "I am definitely a board which has > system_address_space as its one and only address space", > which is OK if you're a board model but a bit more dubious > if you're a generic device like this one. Doesn't this not just crashing at least move us a step closer in the right direction (even if it isn't the correct address space)? The other option of just printing an error and exit doesn't seem super useful. Either is better then a segfault though, so we need to do something here. Thanks, Alistair > > thanks > -- PMM >
27.02.2017 22:36, Thomas Huth wrote: > On 25.01.2017 21:45, Thomas Huth wrote: >> When running QEMU with "-M none -device loader,file=kernel.elf", it >> currently crashes with a segmentation fault, because the "none"-machine >> does not have any CPU by default and the generic loader code tries >> to dereference s->cpu. Fix it by adding an appropriate check for a >> NULL pointer. Applied to -trivial, thanks! /mjt
diff --git a/hw/core/generic-loader.c b/hw/core/generic-loader.c index 58f1f02..4601267 100644 --- a/hw/core/generic-loader.c +++ b/hw/core/generic-loader.c @@ -137,20 +137,21 @@ static void generic_loader_realize(DeviceState *dev, Error **errp) #endif if (s->file) { + AddressSpace *as = s->cpu ? s->cpu->as : NULL; + if (!s->force_raw) { size = load_elf_as(s->file, NULL, NULL, &entry, NULL, NULL, - big_endian, 0, 0, 0, s->cpu->as); + big_endian, 0, 0, 0, as); if (size < 0) { size = load_uimage_as(s->file, &entry, NULL, NULL, NULL, NULL, - s->cpu->as); + as); } } if (size < 0 || s->force_raw) { /* Default to the maximum size being the machine's ram size */ - size = load_image_targphys_as(s->file, s->addr, ram_size, - s->cpu->as); + size = load_image_targphys_as(s->file, s->addr, ram_size, as); } else { s->addr = entry; }
When running QEMU with "-M none -device loader,file=kernel.elf", it currently crashes with a segmentation fault, because the "none"-machine does not have any CPU by default and the generic loader code tries to dereference s->cpu. Fix it by adding an appropriate check for a NULL pointer. Reported-by: Laurent Vivier <laurent@vivier.eu> Signed-off-by: Thomas Huth <thuth@redhat.com> --- hw/core/generic-loader.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)