Re: [Qemu-devel] [PATCH] qom: cpu: destroy work_mutex in cpu_common_finalize

[李强]

Hi Paolo,

What's the status of this patch? I don't see it in upstream.


Thanks,
Li Qiang


At 2019-01-08 07:41:09, "Paolo Bonzini"  wrote:
>On 02/01/19 08:41, Li Qiang wrote:
>> Commit 376692b9dc6(cpus: protect work list with work_mutex)
>> initialize a work_mutex in cpu_common_initfn, however forget
>> to destroy it. This will cause resource leak when hotunplug cpu
>> or hotplug cpu fails.
>> 
>> Signed-off-by: Li Qiang 
>> ---
>>  qom/cpu.c | 3 +++
>>  1 file changed, 3 insertions(+)
>> 
>> diff --git a/qom/cpu.c b/qom/cpu.c
>> index 9ad1372d57..367ebf9d61 100644
>> --- a/qom/cpu.c
>> +++ b/qom/cpu.c
>> @@ -380,6 +380,9 @@ static void cpu_common_initfn(Object *obj)
>>  
>>  static void cpu_common_finalize(Object *obj)
>>  {
>> +CPUState *cpu = CPU(obj);
>> +
>> +qemu_mutex_destroy(>work_mutex);
>>  }
>>  
>>  static int64_t cpu_common_get_arch_id(CPUState *cpu)
>> 
>
>Queued, thanks.
>
>Paolo

Re: [Qemu-devel] [PATCH PULL 00/10] RDMA queue

[李强]

Hi Marcel,
Seems you lost another:


hw: pvrdma: fix memory leak in error path
-->http://lists.gnu.org/archive/html/qemu-devel/2019-01/msg01217.html



At 2019-01-19 18:03:05, "Marcel Apfelbaum"  wrote:
>The following changes since commit a8d2b0685681e2f291faaa501efbbd76875f8ec8:
>
>  Merge remote-tracking branch 'remotes/cohuck/tags/s390x-20190118' into 
> staging (2019-01-18 16:56:15 +)
>
>are available in the Git repository at:
>
>  https://github.com/marcel-apf/qemu tags/rdma-pull-request
>
>for you to fetch changes up to 0f645ba16c6b76ccf2076d38460aa998198893bc:
>
>  contrib/rdmacm-mux: fix clang compilation (2019-01-19 11:01:57 +0200)
>
>
>RDMA queue
> * Clang compilation fix
> * Coverity fix
> * Various fixes for the pvrdma device
>
>
>Kamal Heib (1):
>  docs/pvrdma: Update rdmacm-mux documentation
>
>Li Qiang (1):
>  hw: rdma: fix an off-by-one issue
>
>Marcel Apfelbaum (3):
>  contrib/rdmacm-mux: remove Wno-format-truncation flag
>  hw/rdma: modify struct initialization
>  contrib/rdmacm-mux: fix clang compilation
>
>Yuval Shaia (5):
>  hw/pvrdma: Remove max-sge command-line param
>  hw/rdma: Delete unused struct member
>  hw/pvrdma: Post CQE when receive invalid gid index
>  hw/pvrdma: Make function pvrdma_qp_send/recv return void.
>  hw/rdma: Verify that ptr is not NULL before freeing
>
> Makefile |  2 ++
> contrib/rdmacm-mux/Makefile.objs |  1 -
> contrib/rdmacm-mux/main.c| 12 
> docs/pvrdma.txt  |  4 ++-
> hw/rdma/rdma_backend.c   | 63 +++-
> hw/rdma/rdma_backend.h   | 12 
> hw/rdma/rdma_backend_defs.h  |  1 -
> hw/rdma/rdma_rm.c|  9 --
> hw/rdma/vmw/pvrdma_main.c| 10 +++
> hw/rdma/vmw/pvrdma_qp_ops.c  | 44 +---
> hw/rdma/vmw/pvrdma_qp_ops.h  |  4 +--
> 11 files changed, 96 insertions(+), 66 deletions(-)
>-- 
>2.17.1

Re: [Qemu-devel] [PATCH v2] s390: avoid potential null dereference ins390_pcihost_unplug()

[李强]

At 2019-01-08 00:10:29, "Cornelia Huck"  wrote:
>On Mon, 7 Jan 2019 16:04:35 +
>Peter Maydell  wrote:
>
>> On Mon, 7 Jan 2019 at 15:57, Cornelia Huck  wrote:
>> > On Mon, 7 Jan 2019 15:54:21 +
>> > Peter Maydell  wrote:  
>> > > On Mon, 7 Jan 2019 at 15:48, Cornelia Huck  wrote:  
>> > > > Sounds good. But please return anyway in the unplug case, so that the
>> > > > code is fine if asserts have been configured out.  
>> > >
>> > > Hopefully that won't cause the compiler to complain about
>> > > unreachable code :-)  
>> >
>> > BTW: Is there a common configuration where asserts are configured out?
>> > Not that this is an accident waiting to happen...  
>> 
>> No -- we insist they are always enabled, and osdep.h will #error
>> out if either NDEBUG or G_DISABLE_ASSERT are set.
>
>Ah, now I remember (I thought we still had that problem.)
>

>In that case, no return is needed.


Ok, later I will send out a revised patch.


Thanks,
Li Qiang

Re: [Qemu-devel] [PATCH] hw: pvrdma: fix memory leak in error path

[李强]

At 2019-01-08 00:26:58, "Yuval Shaia"  wrote:
>On Thu, Jan 03, 2019 at 02:47:37PM +0100, Philippe Mathieu-Daudé wrote:
>> On 1/3/19 2:03 PM, Li Qiang wrote:
>> > Spotted by Coverity: CID 1398595
>> > 
>> 
>> Fixes: 2b05705dc8
>> 
>> > Signed-off-by: Li Qiang 
>> 
>> Reviewed-by: Philippe Mathieu-Daudé 
>> 
>> > ---
>> >  hw/rdma/vmw/pvrdma_qp_ops.c | 2 ++
>> >  1 file changed, 2 insertions(+)
>> > 
>> > diff --git a/hw/rdma/vmw/pvrdma_qp_ops.c b/hw/rdma/vmw/pvrdma_qp_ops.c
>> > index 300471a4c9..584be2043e 100644
>> > --- a/hw/rdma/vmw/pvrdma_qp_ops.c
>> > +++ b/hw/rdma/vmw/pvrdma_qp_ops.c
>> > @@ -168,6 +168,7 @@ int pvrdma_qp_send(PVRDMADev *dev, uint32_t qp_handle)
>> >  sgid = rdma_rm_get_gid(>rdma_dev_res, 
>> > wqe->hdr.wr.ud.av.gid_index);
>> >  if (!sgid) {
>> >  pr_dbg("Fail to get gid for idx %d\n", 
>> > wqe->hdr.wr.ud.av.gid_index);
>> > +g_free(comp_ctx);
>> >  return -EIO;
>> >  }
>> >  pr_dbg("sgid_id=%d, sgid=0x%llx\n", wqe->hdr.wr.ud.av.gid_index,
>> > @@ -179,6 +180,7 @@ int pvrdma_qp_send(PVRDMADev *dev, uint32_t qp_handle)
>> >  if (sgid_idx <= 0) {
>> >  pr_dbg("Fail to get bk sgid_idx for sgid_idx %d\n",
>> > wqe->hdr.wr.ud.av.gid_index);
>> > +g_free(comp_ctx);
>> >  return -EIO;
>> >  }
>
>Since comp_ctx is not used until the two checks are done we just can
>relocate the allocation & initialization right after the two checks.

>


OK, will send a revised version later.


Thanks,
Li Qiang


>Yuval
>
>> >  
>> > 

Re: [Qemu-devel] [PATCH 2/5] pvrdma: add uar_read routine

[李强]

At 2018-12-11 23:22:32, "Yuval Shaia"  wrote:
>On Tue, Dec 11, 2018 at 06:56:39PM +0530, P J P wrote:
>> From: Prasad J Pandit 
>> 
>> Define skeleton 'uar_read' routine. Avoid NULL dereference.
>> 
>> Reported-by: Li Qiang 
>> Signed-off-by: Prasad J Pandit 
>> ---
>>  hw/rdma/vmw/pvrdma_main.c | 6 ++
>>  1 file changed, 6 insertions(+)
>> 
>> diff --git a/hw/rdma/vmw/pvrdma_main.c b/hw/rdma/vmw/pvrdma_main.c
>> index ca5fa8d981..a6211d416d 100644
>> --- a/hw/rdma/vmw/pvrdma_main.c
>> +++ b/hw/rdma/vmw/pvrdma_main.c
>> @@ -455,6 +455,11 @@ static const MemoryRegionOps regs_ops = {
>>  },
>>  };
>>  
>> +static uint64_t uar_read(void *opaque, hwaddr addr, unsigned size)
>> +{
>> +return 0;
>> +}
>> +
>>  static void uar_write(void *opaque, hwaddr addr, uint64_t val, unsigned 
>> size)
>>  {
>>  PVRDMADev *dev = opaque;
>> @@ -496,6 +501,7 @@ static void uar_write(void *opaque, hwaddr addr, 
>> uint64_t val, unsigned size)
>>  }
>>  
>>  static const MemoryRegionOps uar_ops = {
>> +.read = uar_read,
>

>Are you sure it is needed?


I'm quite sure this.
The issue here is that in memory_region_dispatch_read1
if there is no mr's read callback, the 'memory_region_read_with_attrs_accessor' 
will be called, but in that the 'mr->ops->raed_with_attrs' has no check.


In fact, I have send out a patch for the framework:
-->https://lists.gnu.org/archive/html/qemu-devel/2018-11/msg02265.html


But no more response.


>Looking at memory_region_dispatch_read1 i can see that there is a check but 
>>not sure this is the right place. Anyways, if it is not, i believe this
>should be framework responsibility.


Reference Peter's answer here:
-->https://lists.gnu.org/archive/html/qemu-devel/2018-09/msg01404.html


"Currently our semantics are "you must provide both read and write, even
if one of them just always returns 0 / does nothing / returns an error".
We could probably reasonably assert this at the point when the
MemoryRegionOps is registered."




Thanks,
Li Qiang


> >> .write = uar_write, >> .endianness = DEVICE_LITTLE_ENDIAN, >> .impl = { >> 
> >> -- >> 2.19.2 >>

Re: [Qemu-devel] [PATCH 0/3] fw_cfg: fix boot bootsplash and reboot-timeout error checking

[李强]

At 2018-11-21 03:59:28, "Philippe Mathieu-Daudé"  wrote:
>Hi Markus, Li.
>
>On 20/11/18 20:34, Markus Armbruster wrote:
>> Li Qiang  writes:
>> 
>>> And also do some code cleanup.
>>> A lot of thanks to Markus's review and advice.
>>>
>>> Li Qiang (3):
>>>fw_cfg: fix -boot bootsplash error checking
>>>fw_cfg: fix -boot reboot-timeout error checking
>>>fw_cfg: make qemu_extra_params_fw locally
>>>
>>>   hw/nvram/fw_cfg.c   | 68 ++---
>>>   include/sysemu/sysemu.h |  1 -
>>>   vl.c|  4 +--
>>>   3 files changed, 32 insertions(+), 41 deletions(-)
>> 
>> fw_cfg.c has no maintainer.  Who would be willing to merge this?
>
>I have various patches for fw_cfg waiting 4.0 to start, one add a 
>MAINTAINERS entry covered by Laszlo, Gerd and myself.

>


Nice, when 4.0 window open? I don't find the release planning.
Maybe my another fw_cfg patches can be merged:
-->https://lists.gnu.org/archive/html/qemu-devel/2018-11/msg4.html
-->https://lists.gnu.org/archive/html/qemu-devel/2018-11/msg5.html


Also I've prepared some qtest patch for fw_cfg. 


PS: I'm quite surprise the qemu's version up to 4 quickly(anyway, the 3.1 is 
just begin)...


Thanks,
Li Qiang


>This series was published way before soft freeze, but I am not sure it 
>applies for hard freeze. If so, it might goes thru Paolo's Misc tree, 

>else we'll take it for 4.0 (via fw_cfg-next).


>
>Regards,
>
>Phil.

Re: [Qemu-devel] [PATCH 2/2] hw: fw_cfg: refactor fw_cfg_reboot()

[李强]

At 2018-11-17 00:52:58, "Markus Armbruster"  wrote:
>Li Qiang  writes:
>
>> Currently the user can set a negative reboot_timeout.
>> Also it is wrong to parse 'reboot-timeout' with qemu_opt_get() and then
>> convert it to number.
>
>Again, it's not wrong per se, just needlessly complicated and
>error-prone.  What makes it wrong is ...
>
>> convert it to number. This patch refactor this function by following:
>> 1. ensure reboot_timeout is in 0~0x
>> 2. use qemu_opt_get_number() to parse reboot_timeout
>> 3. simlify code
>>
>> Signed-off-by: Li Qiang 
>> ---
>>  hw/nvram/fw_cfg.c | 23 +++
>>  vl.c  |  2 +-
>>  2 files changed, 12 insertions(+), 13 deletions(-)
>>
>> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
>> index 78f43dad93..6aca80846a 100644
>> --- a/hw/nvram/fw_cfg.c
>> +++ b/hw/nvram/fw_cfg.c
>> @@ -178,24 +178,23 @@ static void fw_cfg_bootsplash(FWCfgState *s)
>>  
>>  static void fw_cfg_reboot(FWCfgState *s)
>>  {
>> -int reboot_timeout = -1;
>> -char *p;
>> -const char *temp;
>> +const char *reboot_timeout = NULL;
>>  
>>  /* get user configuration */
>>  QemuOptsList *plist = qemu_find_opts("boot-opts");
>>  QemuOpts *opts = QTAILQ_FIRST(>head);
>> -if (opts != NULL) {
>> -temp = qemu_opt_get(opts, "reboot-timeout");
>> -if (temp != NULL) {
>> -p = (char *)temp;
>> -reboot_timeout = strtol(p, , 10);
>
>... the total lack of error checking here.  Same in PATCH 1.

>


Got.


>Here's my attempt at a clearer commit message:
>
>fw_cfg: Fix -boot reboot-timeout error checking
>
>fw_cfg_reboot() gets option parameter "reboot-timeout" with
>qemu_opt_get(), then converts it to an integer by hand.  It neglects
>to check that conversion for errors, and fails to reject negative
>values.  Positive values above the limit get reported and replaced
>by the limit.
>
>Check for conversion errors properly, and reject all values outside
>0..0x.

>


Thanks for your advice, I appreciate it and will change in the revision version.


>PATCH 1's commit message could be improved the same way.
>
>> -}
>> +reboot_timeout = qemu_opt_get(opts, "reboot-timeout");
>> +
>> +if (reboot_timeout == NULL) {
>> +return;
>>  }
>> +int64_t rt_val = qemu_opt_get_number(opts, "reboot-timeout", -1);
>> +
>>  /* validate the input */
>> -if (reboot_timeout > 0x) {
>> -error_report("reboot timeout is larger than 65535, force it to 
>> 65535.");
>> -reboot_timeout = 0x;
>> +if (rt_val < 0 || rt_val > 0x) {
>> +error_report("reboot timeout is invalid,"
>> + "it should be a value between 0 and 65535");
>> +exit(1);
>>  }
>>  fw_cfg_add_file(s, "etc/boot-fail-wait", g_memdup(_timeout, 4), 
>> 4);
>>  }
>
>Change in behavior when "reboot-timeout" isn't specified.
>
>Before your patch, we fw_cfg_add_file() with a value of -1.
>
>After your patch, we don't fw_cfg_add_file().
>
>Why is that okay?

>


Here I following Gerd's advice. 
For values >0x  or < 0, report and exit.
-->http://lists.gnu.org/archive/html/qemu-devel/2018-11/msg00551.html
Thanks,
Li Qiang
>> diff --git a/vl.c b/vl.c
>> index be37da46f0..086127ff0b 100644
>> --- a/vl.c
>> +++ b/vl.c
>> @@ -339,7 +339,7 @@ static QemuOptsList qemu_boot_opts = {
>>  .type = QEMU_OPT_NUMBER,
>>  }, {
>>  .name = "reboot-timeout",
>> -.type = QEMU_OPT_STRING,
>> +.type = QEMU_OPT_NUMBER,
>>  }, {
>>  .name = "strict",
>>  .type = QEMU_OPT_BOOL,

Re: [Qemu-devel] [PATCH 1/2] hw: fw_cfg: refactor fw_cfg_bootsplash()

[李强]

At 2018-11-17 00:33:34, "Markus Armbruster"  wrote:
>Li Qiang  writes:
>
>> Currently when the splash-time value is bigger than 0x
>> we report and correct it, when it is less than 0 we just ingore it.
>
>s/ingore/ignore/
>
>> Also we use qemu_opt_get() to get 'splash-time', then convert it to a number
>> ourselves. This is wrong.
>
>Well, doing it that way isn't wrong, it's just needlessly complicated
>and error-prone.
>
>Suggest starting a new paragraph right here.
>
>>   This patch does following:
>> 1. use qemu_opt_get_number() to parse 'splash-time'
>> 2. exit when the splash-time is invalid or loading the splash file failed
>> 3. simplify code
>>
>> Signed-off-by: Li Qiang 
>> ---
>>  hw/nvram/fw_cfg.c | 40 
>>  vl.c  |  2 +-
>>  2 files changed, 17 insertions(+), 25 deletions(-)
>>
>> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
>> index 946f765f7f..78f43dad93 100644
>> --- a/hw/nvram/fw_cfg.c
>> +++ b/hw/nvram/fw_cfg.c
>> @@ -118,55 +118,47 @@ error:
>>  
>>  static void fw_cfg_bootsplash(FWCfgState *s)
>>  {
>> -int boot_splash_time = -1;
>>  const char *boot_splash_filename = NULL;
>> -char *p;
>> +const char *boot_splash_time = NULL;
>>  char *filename, *file_data;
>>  gsize file_size;
>>  int file_type;
>> -const char *temp;
>>  
>>  /* get user configuration */
>>  QemuOptsList *plist = qemu_find_opts("boot-opts");
>>  QemuOpts *opts = QTAILQ_FIRST(>head);
>> -if (opts != NULL) {
>> -temp = qemu_opt_get(opts, "splash");
>> -if (temp != NULL) {
>> -boot_splash_filename = temp;
>> -}
>> -temp = qemu_opt_get(opts, "splash-time");
>> -if (temp != NULL) {
>> -p = (char *)temp;
>> -boot_splash_time = strtol(p, , 10);
>> -}
>> -}
>> +boot_splash_filename = qemu_opt_get(opts, "splash");
>> +boot_splash_time = qemu_opt_get(opts, "splash-time");
>
>You first get "splash-time" as a string, and then ...
>>  
>>  /* insert splash time if user configurated */
>> -if (boot_splash_time >= 0) {
>> +if (boot_splash_time) {
>> +int64_t bst_val = qemu_opt_get_number(opts, "splash-time", -1);
>
>... you get it again as a number.  I figure you do that because
>"splash-time not specified" is not the same as "splash-time=T" for any
>T.  I don't like such interfaces.  Not this patch's fault.
>
>Just noticed: qemu_extra_params_fw[] has external linkage, but is used
>only in this function.  Care to make it static in this function in a

>separate patch?


Will do in the next revision.


Thanks,
Li Qiang


>
>>  /* validate the input */
>> -if (boot_splash_time > 0x) {
>> -error_report("splash time is big than 65535, force it to 
>> 65535.");
>> -boot_splash_time = 0x;
>> +if (bst_val < 0 || bst_val > 0x) {
>> +error_report("splash time is invalid,"
>> + "it should be a value between 0 and 65535");
>> +exit(1);
>>  }
>>  /* use little endian format */
>> -qemu_extra_params_fw[0] = (uint8_t)(boot_splash_time & 0xff);
>> -qemu_extra_params_fw[1] = (uint8_t)((boot_splash_time >> 8) & 0xff);
>> +qemu_extra_params_fw[0] = (uint8_t)(bst_val & 0xff);
>> +qemu_extra_params_fw[1] = (uint8_t)((bst_val >> 8) & 0xff);
>>  fw_cfg_add_file(s, "etc/boot-menu-wait", qemu_extra_params_fw, 2);
>>  }
>>  
>>  /* insert splash file if user configurated */
>> -if (boot_splash_filename != NULL) {
>> +if (boot_splash_filename) {
>>  filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, 
>> boot_splash_filename);
>>  if (filename == NULL) {
>> -error_report("failed to find file '%s'.", boot_splash_filename);
>> -return;
>> +error_report("failed to find file '%s'", boot_splash_filename);
>> +exit(1);
>>  }
>>  
>>  /* loading file data */
>>  file_data = read_splashfile(filename, _size, _type);
>>  if (file_data == NULL) {
>>  g_free(filename);
>> -return;
>> +error_report("failed to read file '%s'", boot_splash_filename);
>> +exit(1);
>>  }
>>  g_free(boot_splash_filedata);
>>  boot_splash_filedata = (uint8_t *)file_data;
>> diff --git a/vl.c b/vl.c
>> index 55bab005b6..be37da46f0 100644
>> --- a/vl.c
>> +++ b/vl.c
>> @@ -336,7 +336,7 @@ static QemuOptsList qemu_boot_opts = {
>>  .type = QEMU_OPT_STRING,
>>  }, {
>>  .name = "splash-time",
>> -.type = QEMU_OPT_STRING,
>> +.type = QEMU_OPT_NUMBER,
>>  }, {
>>  .name = "reboot-timeout",
>>  .type = QEMU_OPT_STRING,
>
>Reviewed-by: Markus Armbruster 

Re: [Qemu-devel] [PATCH 2/2] tests: fw_cfg: add reboot_timeout test case

[李强]

At 2018-10-30 08:08:55, "Philippe Mathieu-Daudé"  wrote:
>On 28/10/18 13:40, Li Qiang wrote:
>> Signed-off-by: Li Qiang 
>> ---
>>   tests/fw_cfg-test.c | 13 -
>>   1 file changed, 12 insertions(+), 1 deletion(-)
>> 
>> diff --git a/tests/fw_cfg-test.c b/tests/fw_cfg-test.c
>> index 1c5103fe1c..37765f15f8 100644
>> --- a/tests/fw_cfg-test.c
>> +++ b/tests/fw_cfg-test.c
>> @@ -99,6 +99,15 @@ static void test_fw_cfg_boot_menu(void)
>>   g_assert_cmpint(qfw_cfg_get_u16(fw_cfg, FW_CFG_BOOT_MENU), ==, 
>> boot_menu);
>>   }
>>   
>> +static void test_fw_cfg_reboot_timeout(void)
>> +{
>> +uint32_t reboot_timeout;
>> +
>> +qfw_cfg_get_file(fw_cfg, "etc/boot-fail-wait",
>> + _timeout, sizeof(reboot_timeout));
>> +g_assert_cmpint(reboot_timeout, <=, 65535);
>> +}
>> +
>>   int main(int argc, char **argv)
>>   {
>>   QTestState *s;
>> @@ -106,7 +115,8 @@ int main(int argc, char **argv)
>>   
>>   g_test_init(, , NULL);
>>   
>> -s = qtest_init("-uuid 4600cb32-38ec-4b2f-8acb-81c6ea54f2d8");
>> +s = qtest_init("-uuid 4600cb32-38ec-4b2f-8acb-81c6ea54f2d8"
>> +   " -boot reboot-timeout=4294967295");
>
>I'd rather change this test to use qtest_add_data_func() ...:
>
> qtest_add_data_func("fw_cfg/reboot_timeout", "-boot 
>reboot-timeout=4294967295 ", test_fw_cfg_reboot_timeout);

>


Hi here I think use qtest_add_func is ok because there is no need
to make such an exception as all of them uses qtest_add_func.


Second, the uuid is also uses by all the cases though some of them are not 
needed.
Third, the -boot option will not affect the other cases.


Thanks,
Li Qiang


>... to avoid adding this command line option to all the tests, because 
>all tests are now failing:
>
>$ make check-qtest-i386
>[...]
>ERROR:qemu/tests/fw_cfg-test.c:33:test_fw_cfg_signature: assertion 
>failed (buf == "QEMU"): ("\377\377\377\377" == "QEMU")
>ERROR:qemu/tests/fw_cfg-test.c:40:test_fw_cfg_id: assertion failed: ((id 
>== 1) || (id == 3))
>ERROR:qemu/tests/fw_cfg-test.c:52:test_fw_cfg_uuid: assertion failed: 
>(memcmp(buf, uuid, sizeof(buf)) == 0)
>ERROR:qemu/tests/fw_cfg-test.c:57:test_fw_cfg_ram_size: assertion failed 
>(qfw_cfg_get_u64(fw_cfg, FW_CFG_RAM_SIZE) == ram_size): (-1 == 134217728)
>ERROR:qemu/tests/fw_cfg-test.c:62:test_fw_cfg_nographic: assertion 
>failed (qfw_cfg_get_u16(fw_cfg, FW_CFG_NOGRAPHIC) == 0): (65535 == 0)
>ERROR:qemu/tests/fw_cfg-test.c:67:test_fw_cfg_nb_cpus: assertion failed 
>(qfw_cfg_get_u16(fw_cfg, FW_CFG_NB_CPUS) == nb_cpus): (65535 == 1)
>ERROR:qemu/tests/fw_cfg-test.c:72:test_fw_cfg_max_cpus: assertion failed 
>(qfw_cfg_get_u16(fw_cfg, FW_CFG_MAX_CPUS) == max_cpus): (65535 == 1)
>ERROR:qemu/tests/fw_cfg-test.c:80:test_fw_cfg_numa: assertion failed 
>(qfw_cfg_get_u64(fw_cfg, FW_CFG_NUMA) == nb_nodes): (-1 == 0)
>ERROR:qemu/tests/fw_cfg-test.c:99:test_fw_cfg_boot_menu: assertion 
>failed (qfw_cfg_get_u16(fw_cfg, FW_CFG_BOOT_MENU) == boot_menu): (65535 
>== 0)
>
>
>(did you test your patch?)
>
>>   
>>   fw_cfg = pc_fw_cfg_init(s);
>>   
>> @@ -125,6 +135,7 @@ int main(int argc, char **argv)
>>   qtest_add_func("fw_cfg/max_cpus", test_fw_cfg_max_cpus);
>>   qtest_add_func("fw_cfg/numa", test_fw_cfg_numa);
>>   qtest_add_func("fw_cfg/boot_menu", test_fw_cfg_boot_menu);
>> +qtest_add_func("fw_cfg/reboot_timeout", test_fw_cfg_reboot_timeout);
>>   
>>   ret = g_test_run();
>>   
>> 

Re: [Qemu-devel] qemu memory manage question

[李强]

> -Original Message-
> From: Qemu-devel
> [mailto:qemu-devel-bounces+liqiang6-s=360...@nongnu.org] On Behalf Of
> jack.chen
> Sent: Monday, April 17, 2017 6:56 PM
> To: Peter Xu
> Cc: qemu
> Subject: Re: [Qemu-devel] qemu memory manage question
> 
> Thanks,from the path you have list to me,it can be  well explained,but
> according to the source code,in the end of kvm_init,kvm_memory_listener and
> kvm_io_listener were registered by memory_listener_register(),and  in the
> end of
> memory_listener_register(),listener_add_address_space() was called for each
> address_space,so the listener->region_add was executed then.I do not know
> what mistake I have made,can you explain it to me ?? thank you very much!
> 

They are callbacks.
Every change of memory topology will call these listeners, add 
subregion(Peter's example),
modify the property of memory, create address space for example. 

Thanks.

--
Li Qiang /the Gear Team, Qihoo 360 Inc


> 2017-04-17 18:26 GMT+08:00 Peter Xu :
> > On Mon, Apr 17, 2017 at 06:09:11PM +0800, jack.chen wrote:
> >> hello,I have some questions about memory allocation in qemu for
> >> virtual machine.I found when configure_accelerator function was
> >> called ,memory slots  were registered to KVM,but at that time
> >> address_space have not been initialized and ram have not been
> >> allocated,it is really confused me,Thanks a lot!!
> >
> > Here's how I understand it...
> >
> > configure_accelerator() does not register memory slots in KVM.
> > Instead, it registers memory listeners. See
> > kvm_memory_listener_register(), especially:
> >
> > kml->listener.region_add = kvm_region_add;
> >
> > That's the hook function to be called when there are new memory region
> > added to the system.
> >
> > Further, when RAM is initialzed, it'll modify the address space layout
> > of system_memory, and the registered listener of KVM (kvm_region_add)
> > will be invoked, it'll further sync with kvm. It should be in the
> > following path if you break at kvm_region_add in gdb:
> >
> > #0  0x557ba13a in kvm_region_add (listener=0x568330c0,
> > section=0x7fffd310) at /root/git/qemu/kvm-all.c:859
> > #1  0x557c1910 in address_space_update_topology_pass
> > (as=0x5629e240 ,
> old_view=0x567a7090,
> > new_view=0x568d3460, adding=true) at /root/git/qemu/memory.c:871
> > #2  0x557c19f3 in address_space_update_topology
> > (as=0x5629e240 ) at
> > /root/git/qemu/memory.c:886
> > #3  0x557c1b41 in memory_region_transaction_commit () at
> > /root/git/qemu/memory.c:922
> > #4  0x557c4bfd in memory_region_update_container_subregions
> > (subregion=0x568d2fc0) at /root/git/qemu/memory.c:2075
> > #5  0x557c4c64 in memory_region_add_subregion_common
> > (mr=0x567a5830, offset=0, subregion=0x568d2fc0) at
> > /root/git/qemu/memory.c:2085
> > #6  0x557c4ca0 in memory_region_add_subregion
> > (mr=0x567a5830, offset=0, subregion=0x568d2fc0) at
> > /root/git/qemu/memory.c:2093
> > #7  0x5583fd68 in pc_memory_init (pcms=0x567a4100,
> > system_memory=0x567a5830, rom_memory=0x568d21a0,
> > ram_memory=0x7fffd550) at /root/git/qemu/hw/i386/pc.c:1383
> > #8  0x55847363 in pc_q35_init (machine=0x567a4100) at
> > /root/git/qemu/hw/i386/pc_q35.c:147
> > #9  0x55847cac in pc_init_v2_9 (machine=0x567a4100) at
> > /root/git/qemu/hw/i386/pc_q35.c:310
> > #10 0x558f7cf8 in main (argc=11, argv=0x7fffda78,
> > envp=0x7fffdad8) at /root/git/qemu/vl.c:4557
> >
> > Hope this helps. Thanks.
> >
> > --
> > Peter Xu

Re: [Qemu-devel] [for-2.9 PATCH 3/3] 9pfs: drop useless loop in v9fs_reset()

[李强]

> -Original Message-
> From: Greg Kurz [mailto:gr...@kaod.org]
> Sent: Friday, March 31, 2017 7:28 PM
> To: qemu-devel@nongnu.org
> Cc: Eric Blake; 李强; Greg Kurz
> Subject: [for-2.9 PATCH 3/3] 9pfs: drop useless loop in v9fs_reset()
> 
> We don't need to wait for the PDU active list to be empty: virtfs_reset() 
> already
> takes care of that.
> 
> Signed-off-by: Greg Kurz <gr...@kaod.org>
> ---

Reviewed-by: Li Qiang <liqiang...@360.cn>

>  hw/9pfs/9p.c |5 +
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c index 86ed9065c4e2..16ef6bd5bd8c
> 100644
> --- a/hw/9pfs/9p.c
> +++ b/hw/9pfs/9p.c
> @@ -3601,6 +3601,7 @@ static void coroutine_fn virtfs_co_reset(void
> *opaque)
>  VirtfsCoResetData *data = opaque;
> 
>  virtfs_reset(>pdu);
> +assert(QLIST_EMPTY(>pdu.s->active_list));
>  data->done = true;
>  }
> 
> @@ -3609,10 +3610,6 @@ void v9fs_reset(V9fsState *s)
>  VirtfsCoResetData data = { .pdu = { .s = s }, .done = false };
>  Coroutine *co;
> 
> -while (!QLIST_EMPTY(>active_list)) {
> -aio_poll(qemu_get_aio_context(), true);
> -}
> -
>  co = qemu_coroutine_create(virtfs_co_reset, );
>  qemu_coroutine_enter(co);
> 

Re: [Qemu-devel] [for-2.9 PATCH 2/3] 9pfs: cancel active PDUs in virtfs_reset()

[李强]

> -Original Message-
> From: Greg Kurz [mailto:gr...@kaod.org]
> Sent: Friday, March 31, 2017 7:27 PM
> To: qemu-devel@nongnu.org
> Cc: Eric Blake; 李强; Greg Kurz
> Subject: [for-2.9 PATCH 2/3] 9pfs: cancel active PDUs in virtfs_reset()
> 
> According to the 9P spec [1], the version operation should abort any
> outstanding I/O and clunk all fids, so that a new session may be started in a
> clean state.
> 
> The current code tries to clunk and free fids, but it doesn't wait for active 
> PDUs
> to complete. This can cause an I/O to actually complete after the new session
> has begun, and confuse the client.
> 
> This patch modifies virtfs_reset() so that it explicitely cancels and waits 
> for
> inflight requests to terminate. All fids should thus be unreferenced and 
> ready to
> be freed. Let's make it clear with a an assertion.
> 
> [1] http://man.cat-v.org/plan_9/5/version
> 
> Signed-off-by: Greg Kurz <gr...@kaod.org>
> ---

Reviewed-by: Li Qiang <liqiang...@360.cn>

>  hw/9pfs/9p.c |   22 +-
>  1 file changed, 21 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c index cc109367b030..86ed9065c4e2
> 100644
> --- a/hw/9pfs/9p.c
> +++ b/hw/9pfs/9p.c
> @@ -536,9 +536,29 @@ static void coroutine_fn virtfs_reset(V9fsPDU *pdu)
> {
>  V9fsState *s = pdu->s;
>  V9fsFidState *fidp;
> +bool done = false;
> +
> +/* Drain any outstanding I/O */
> +while (!done) {
> +V9fsPDU *cancel_pdu;
> +
> +done = true;
> +QLIST_FOREACH(cancel_pdu, >active_list, next) {
> +if (cancel_pdu != pdu) {
> +done = false;
> +cancel_pdu->cancelled = 1;
> +qemu_co_queue_wait(_pdu->complete, NULL);
> +cancel_pdu->cancelled = 0;
> +pdu_free(cancel_pdu);
> +break;
> +}
> +}
> +}
> 
>  /* Free all fids */
>  while (s->fid_list) {
> +assert(!fidp->ref);
> +
>  /* Get fid */
>  fidp = s->fid_list;
>  fidp->ref++;
> @@ -670,7 +690,7 @@ static void coroutine_fn pdu_complete(V9fsPDU *pdu,
> ssize_t len)
> 
>  pdu_push_and_notify(pdu);
> 
> -/* Now wakeup anybody waiting in flush for this request */
> +/* Now wakeup anybody waiting in flush or reset for this request */
>  if (!qemu_co_queue_next(>complete)) {
>  pdu_free(pdu);
>  }

Re: [Qemu-devel] [for-2.9 PATCH 1/3] 9pfs: clear migration blocker at session reset

[李强]

> -Original Message-
> From: Greg Kurz [mailto:gr...@kaod.org]
> Sent: Friday, March 31, 2017 7:27 PM
> To: qemu-devel@nongnu.org
> Cc: Eric Blake; 李强; Greg Kurz
> Subject: [for-2.9 PATCH 1/3] 9pfs: clear migration blocker at session reset
> 
> The migration blocker survives a device reset: if the guest mounts a 9p share
> and then gets rebooted with system_reset, it will be unmigratable until it
> remounts and umounts the 9p share again.
> 
> This happens because the migration blocker is supposed to be cleared when we
> put the last reference on the root fid, but virtfs_reset() wrongly calls
> free_fid() instead of put_fid().
> 
> This patch fixes virtfs_reset() so that it honor the way fids are supposed to 
> be
> manipulated: first get a reference and later put it back when you're done.
> 
> Signed-off-by: Greg Kurz <gr...@kaod.org>
> ---

Reviewed-by: Li Qiang <liqiang...@360.cn>

>  hw/9pfs/9p.c |   11 ++-
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c index 48babce836b6..cc109367b030
> 100644
> --- a/hw/9pfs/9p.c
> +++ b/hw/9pfs/9p.c
> @@ -539,14 +539,15 @@ static void coroutine_fn virtfs_reset(V9fsPDU *pdu)
> 
>  /* Free all fids */
>  while (s->fid_list) {
> +/* Get fid */
>  fidp = s->fid_list;
> +fidp->ref++;
> +
> +/* Clunk fid */
>  s->fid_list = fidp->next;
> +fidp->clunked = 1;
> 
> -if (fidp->ref) {
> -fidp->clunked = 1;
> -} else {
> -free_fid(pdu, fidp);
> -}
> +put_fid(pdu, fidp);
>  }
>  }
> 

Re: [Qemu-devel] [PATCH] cirrus: fix off-by-one in cirrus_bitblt_rop_bkwd_transp_*_16

[李强]

> -Original Message-
> From: Qemu-devel
> [mailto:qemu-devel-bounces+liqiang6-s=360...@nongnu.org] On Behalf Of
> Gerd Hoffmann
> Sent: Friday, March 17, 2017 3:22 PM
> To: qemu-devel@nongnu.org
> Cc: Gerd Hoffmann
> Subject: [Qemu-devel] [PATCH] cirrus: fix off-by-one in
> cirrus_bitblt_rop_bkwd_transp_*_16
> 
> The switch from pointers to addresses (commit
> 026aeffcb4752054830ba203020ed6eb05bcaba8 and
> ffaf857778286ca54e3804432a2369a279e73aa7) added a off-by-one bug to
> 16bit backward blits.  Fix.
> 
> Reported-by: 李强 <liqiang...@360.cn>
> Signed-off-by: Gerd Hoffmann <kra...@redhat.com>

Reviewed-by: Li Qiang <liqiang...@360.cn>

> ---
>  hw/display/cirrus_vga_rop.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/display/cirrus_vga_rop.h b/hw/display/cirrus_vga_rop.h index
> c61a677..0841b9e 100644
> --- a/hw/display/cirrus_vga_rop.h
> +++ b/hw/display/cirrus_vga_rop.h
> @@ -219,7 +219,7 @@ glue(glue(cirrus_bitblt_rop_bkwd_transp_,
> ROP_NAME),_16)(CirrusVGAState *s,
>  srcpitch += bltwidth;
>  for (y = 0; y < bltheight; y++) {
>  for (x = 0; x < bltwidth; x+=2) {
> -ROP_OP_TR_16(s, dstaddr, cirrus_src16(s, srcaddr), transp);
> +ROP_OP_TR_16(s, dstaddr - 1, cirrus_src16(s, srcaddr - 1),
> + transp);
>  dstaddr -= 2;
>  srcaddr -= 2;
>  }
> --
> 1.8.3.1
> 

Re: [Qemu-devel] [PULL for-2.9 4/7] cirrus: add option to disable blitter

[李强]

Hello Gerd,

> -Original Message-
> From: Qemu-devel
> [mailto:qemu-devel-bounces+liqiang6-s=360...@nongnu.org] On Behalf Of
> Gerd Hoffmann
> Sent: Thursday, March 16, 2017 5:31 PM
> To: qemu-devel@nongnu.org
> Cc: Gerd Hoffmann
> Subject: [Qemu-devel] [PULL for-2.9 4/7] cirrus: add option to disable blitter
> 
> Ok, we have this beast in the cirrus code which is not used at all by modern
> guests, except when you try to find security holes in qemu.  So, add an option
> to disable blitter altogether.  Guests released within the last ten years 
> should
> not show any rendering issues if you turn off blitter support.
> 
> There are no known bugs in the cirrus blitter code.  But in the past we hoped 
> a
> few times already that we've finally nailed the last issue.  So having some 
> easy
> way to mitigate in case yet another blitter issue shows up certainly makes me
> sleep a bit better at night.
> 
> For completeness:  The by far better way to mitigate is to switch away from
> cirrus and use stdvga instead.  Or something more modern like virtio-vga in
> case your guest has support for it.
> 
> Signed-off-by: Gerd Hoffmann 
> Message-id: 1489494540-15745-1-git-send-email-kra...@redhat.com
> ---
>  hw/display/cirrus_vga.c | 9 +
>  1 file changed, 9 insertions(+)
> 
> diff --git a/hw/display/cirrus_vga.c b/hw/display/cirrus_vga.c index
> 6ffe64f..326d511 100644
> --- a/hw/display/cirrus_vga.c
> +++ b/hw/display/cirrus_vga.c
> @@ -205,6 +205,7 @@ typedef struct CirrusVGAState {
>  uint32_t cirrus_bank_base[2];
>  uint32_t cirrus_bank_limit[2];
>  uint8_t cirrus_hidden_palette[48];
> +bool enable_blitter;
>  int cirrus_blt_pixelwidth;
>  int cirrus_blt_width;
>  int cirrus_blt_height;
> @@ -960,6 +961,10 @@ static void cirrus_bitblt_start(CirrusVGAState * s)  {
>  uint8_t blt_rop;
> 
> +if (!s->enable_blitter) {
> +goto bitblt_ignore;
> +}
> +
>  s->vga.gr[0x31] |= CIRRUS_BLT_BUSY;
> 
>  s->cirrus_blt_width = (s->vga.gr[0x20] | (s->vga.gr[0x21] << 8)) + 1; @@
> -3024,6 +3029,8 @@ static void isa_cirrus_vga_realizefn(DeviceState *dev,
> Error **errp)  static Property isa_cirrus_vga_properties[] = {
>  DEFINE_PROP_UINT32("vgamem_mb", struct ISACirrusVGAState,
> cirrus_vga.vga.vram_size_mb, 4),
> +DEFINE_PROP_BOOL("blitter", struct ISACirrusVGAState,
> +   cirrus_vga.enable_blitter, true),
>  DEFINE_PROP_END_OF_LIST(),
>  };
> 
> @@ -3093,6 +3100,8 @@ static void pci_cirrus_vga_realize(PCIDevice *dev,
> Error **errp)  static Property pci_vga_cirrus_properties[] = {
>  DEFINE_PROP_UINT32("vgamem_mb", struct PCICirrusVGAState,
> cirrus_vga.vga.vram_size_mb, 4),
> +DEFINE_PROP_BOOL("blitter", struct PCICirrusVGAState,
> + cirrus_vga.enable_blitter, true),

The default is 'ENABLE'? I think there should be 'false'.


Thanks.

Qiang

>  DEFINE_PROP_END_OF_LIST(),
>  };
> 
> --
> 1.8.3.1
> 

Re: [Qemu-devel] [PATCH] 9pfs: make unmarshal V9fsString more robust

[李强]

On 2016-09-28 0:40 GMT+08:00 Greg Kurz  wrote:

>
> Talking about robustness was appropriate for your previous patches, but
> it does not really apply here since v9fs_iov_vunmarshal() does not have
> any issue with empty strings actually.
>
> I've changed the title to:
>
> 9pfs: allocate space for guest originated empty strings
>
> And while here, I've updated the changelog to provide a more detailed
> justification:
>
> ...

Thanks very much to point out the mistakes, I will do more next time.

BTW, need I resend this patch formally?

Thanks.

Re: [Qemu-devel] [PATCH] usb: ehci: fix memory leak in ehci_process_itd

[李强]

Ping!

2016-09-19 10:48 GMT+08:00 Li Qiang :

> From: Li Qiang 
>
> While processing isochronous transfer descriptors(iTD), if the page
> select(PG) field value is out of bands it will return. In this
> situation the ehci's sg list doesn't be freed thus leading a memory
> leak issue. This patch avoid this.
>
> Signed-off-by: Li Qiang 
> ---
>  hw/usb/hcd-ehci.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/hw/usb/hcd-ehci.c b/hw/usb/hcd-ehci.c
> index b093db7..f4ece9a 100644
> --- a/hw/usb/hcd-ehci.c
> +++ b/hw/usb/hcd-ehci.c
> @@ -1426,6 +1426,7 @@ static int ehci_process_itd(EHCIState *ehci,
>  if (off + len > 4096) {
>  /* transfer crosses page border */
>  if (pg == 6) {
> +qemu_sglist_destroy(>isgl);
>  return -1;  /* avoid page pg + 1 */
>  }
>  ptr2 = (itd->bufptr[pg + 1] & ITD_BUFPTR_MASK);
> --
> 1.8.3.1
>
>

[Qemu-devel] [PATCH] usb:xhci:fix memory leak in usb_xhci_exit

[李强]

If the xhci uses msix, it doesn't free the corresponding
memory, thus leading a memory leak issue. This patch avoid this.

Signed-off-by: Li Qiang 
---
hw/usb/hcd-xhci.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
index 188f954..281a2a5 100644
--- a/hw/usb/hcd-xhci.c
+++ b/hw/usb/hcd-xhci.c
@@ -3709,8 +3709,7 @@ static void usb_xhci_exit(PCIDevice *dev)
 /* destroy msix memory region */
 if (dev->msix_table && dev->msix_pba
 && dev->msix_entry_used) {
-memory_region_del_subregion(>mem, >msix_table_mmio);
-memory_region_del_subregion(>mem, >msix_pba_mmio);
+msix_uninit(dev, >mem, >mem);
 }
 usb_bus_release(>bus);
--
1.8.3.1

[Qemu-devel] 答复: [PATCH] net: vmxnet: check fragments count at pkt initialisation

[李强]

Hi Dmitry

> 
> > On 12 Aug 2016, at 04:21 AM, 李强 <liqiang...@360.cn> wrote:
> >
> > Hello Dmitry,
> >
> > I don't see the assert for 'max_frags' in vmxnet device emulation. Could you
> please point it out?
> 
> 
>  Hi,
> 
> I mean that max_frags for vmxnet3 device is a size of TX ring so assert
> introduced by this patch will fire all the time.
> 

Got it.

> >
> > In my PoC, I set it to '0x2000',  and in vmxnet_tx_pkt_init() the
> > 'p->raw' will be NULL because of an integer overflow(in x86). And this will
> bypass all the assert, and in vmxnet_tx_pkt_add_raw_fragment(), will cause an
> NULL pointer reference.
> 
> Yes, it is null because of memory allocation failure. However in real life
> scenarios it can not be that big unless TX ring is that big, and in that case 
> you’ll
> get memory allocation problem earlier (during TX ring allocation).
> 
> Additionally, one should not limit max_frags by maximum number of skb
> fragments because not all network backends produce skb’s.
> 

It is null not because of memory allocation failure but integer overflow. In 
x86, 0x2000 * sizeof *p->raw) will be rolled to 0, g_malloc(0) will return 
NULL. This is not a failure.
We can set vmxnet3 ' s->max_tx_frags' to any value from the guest kernel.

In vmxnet3_activate_device(), we can see the 'size' is read from the input of 
guest. We can set 'conf.txRingSize' by insmod a kernel module in guest. 
Actually, we can reset guest driver shared area and control all the data. 

   size = VMXNET3_READ_TX_QUEUE_DESCR32(qdescr_pa, conf.txRingSize);

vmxnet3_ring_init(>txq_descr[i].tx_ring, pa, size,
  sizeof(struct Vmxnet3_TxDesc), false);
VMXNET3_RING_DUMP(VMW_CFPRN, "TX", i, >txq_descr[i].tx_ring);

s->max_tx_frags += size;

> >
> > void vmxnet_tx_pkt_init(struct VmxnetTxPkt **pkt, uint32_t max_frags,
> >bool has_virt_hdr)
> > {
> >struct VmxnetTxPkt *p = g_malloc0(sizeof *p);
> >
> >p->vec = g_malloc((sizeof *p->vec) *
> >(max_frags + VMXNET_TX_PKT_PL_START_FRAG));
> >
> >p->raw = g_malloc((sizeof *p->raw) * max_frags);
> >
> >*pkt = p;
> > }
> >
> > IIUC, we should do assert in the device layer, in vmxnet3_activate_device() 
> > in
> vmxnet?
> 
> Not sure such an assert is needed at all. In general, QEMU code does not check
> memory allocation results.
> 

I think this is not related to memory allocation results but is related to 
integer overflow.

Thanks.


--
Li Qiang / Cloud Security Team, Qihoo 360 Inc

> >
> >> -邮件原件-
> >> 发件人: Dmitry Fleytman [mailto:dmi...@daynix.com]
> >> 发送时间: 2016年8月11日 16:16
> >> 收件人: P J P
> >> 抄送: Qemu developers; 李强; Jason Wang
> >> 主题: Re: [PATCH] net: vmxnet: check fragments count at pkt
> >> initialisation
> >>
> >>
> >>> On 11 Aug 2016, at 11:08 AM, Dmitry Fleytman <dmi...@daynix.com>
> wrote:
> >>>
> >>>
> >>> Acked-by: Dmitry Fleytman <dmi...@daynix.com>
> >>
> >> Oops, please ignore this ACK, I replied to the wrong e-mail.
> >>
> >> As far as I see max_frags for VMXNET3 is a size of device’s TX ring
> >> so this will always assert.
> >>
> >> I don’t think we need this limitation in the device code. Maximum
> >> number of fragments is an internal knowledge of network backend.
> >>
> >> ~Dmitry
> >>
> >>>
> >>>> On 10 Aug 2016, at 23:38 PM, P J P <ppan...@redhat.com> wrote:
> >>>>
> >>>> From: Li Qiang <liqiang...@360.cn>
> >>>>
> >>>> When net transport abstraction layer initialises the pkt, the
> >>>> maximum fragmentation count is not checked. This could lead to an
> >>>> integer overflow causing a NULL pointer dereference.
> >>>> Add check to avoid it.
> >>>>
> >>>> Reported-by: Li Qiang <liqiang...@360.cn>
> >>>> Signed-off-by: Prasad J Pandit <p...@fedoraproject.org>
> >>>> ---
> >>>> hw/net/net_tx_pkt.c | 3 +++
> >>>> 1 file changed, 3 insertions(+)
> >>>>
> >>>> diff --git a/hw/net/net_tx_pkt.c b/hw/net/net_tx_pkt.c index
> >>>> 53dfaa2..7ea3c17 100644
> >>>> --- a/hw/net/net_tx_pkt.c
> >>>> +++ b/hw/net/net_tx_pkt.c
> >>>> @@ -58,9 +58,12 @@ struct NetTxPkt {
> >>>>   bool is_loopback;
> >>>> };
> >>>>
> >>>> +#define NET_PKT_MAX_FRAGS16  /* ref: MAX_SKB_FRAGS in
> kernel
> >> driver */
> >>>> +
> >>>> void net_tx_pkt_init(struct NetTxPkt **pkt, PCIDevice *pci_dev,
> >>>>   uint32_t max_frags, bool has_virt_hdr) {
> >>>> +assert(max_frags <= NET_PKT_MAX_FRAGS);
> >>>>   struct NetTxPkt *p = g_malloc0(sizeof *p);
> >>>>
> >>>>   p->pci_dev = pci_dev;
> >>>> --
> >>>> 2.5.5
> >>>>
> >>>
> >

[Qemu-devel] 答复: [PATCH] net: vmxnet: check fragments count at pkt initialisation

[李强]

Hello Dmitry,

I don't see the assert for 'max_frags' in vmxnet device emulation. Could you 
please point it out?

In my PoC, I set it to '0x2000',  and in vmxnet_tx_pkt_init() the 'p->raw' 
will be NULL because of an integer overflow(in x86). And this will bypass all 
the assert, and in 
vmxnet_tx_pkt_add_raw_fragment(), will cause an NULL pointer reference.

void vmxnet_tx_pkt_init(struct VmxnetTxPkt **pkt, uint32_t max_frags,
bool has_virt_hdr)
{
struct VmxnetTxPkt *p = g_malloc0(sizeof *p);

p->vec = g_malloc((sizeof *p->vec) *
(max_frags + VMXNET_TX_PKT_PL_START_FRAG));

p->raw = g_malloc((sizeof *p->raw) * max_frags);

*pkt = p;
}

IIUC, we should do assert in the device layer, in vmxnet3_activate_device() in 
vmxnet?

> -邮件原件-
> 发件人: Dmitry Fleytman [mailto:dmi...@daynix.com]
> 发送时间: 2016年8月11日 16:16
> 收件人: P J P
> 抄送: Qemu developers; 李强; Jason Wang
> 主题: Re: [PATCH] net: vmxnet: check fragments count at pkt initialisation
> 
> 
> > On 11 Aug 2016, at 11:08 AM, Dmitry Fleytman <dmi...@daynix.com> wrote:
> >
> >
> > Acked-by: Dmitry Fleytman <dmi...@daynix.com>
> 
> Oops, please ignore this ACK, I replied to the wrong e-mail.
> 
> As far as I see max_frags for VMXNET3 is a size of device’s TX ring so this 
> will
> always assert.
> 
> I don’t think we need this limitation in the device code. Maximum number of
> fragments is an internal knowledge of network backend.
> 
> ~Dmitry
> 
> >
> >> On 10 Aug 2016, at 23:38 PM, P J P <ppan...@redhat.com> wrote:
> >>
> >> From: Li Qiang <liqiang...@360.cn>
> >>
> >> When net transport abstraction layer initialises the pkt, the maximum
> >> fragmentation count is not checked. This could lead to an integer
> >> overflow causing a NULL pointer dereference.
> >> Add check to avoid it.
> >>
> >> Reported-by: Li Qiang <liqiang...@360.cn>
> >> Signed-off-by: Prasad J Pandit <p...@fedoraproject.org>
> >> ---
> >> hw/net/net_tx_pkt.c | 3 +++
> >> 1 file changed, 3 insertions(+)
> >>
> >> diff --git a/hw/net/net_tx_pkt.c b/hw/net/net_tx_pkt.c index
> >> 53dfaa2..7ea3c17 100644
> >> --- a/hw/net/net_tx_pkt.c
> >> +++ b/hw/net/net_tx_pkt.c
> >> @@ -58,9 +58,12 @@ struct NetTxPkt {
> >>bool is_loopback;
> >> };
> >>
> >> +#define NET_PKT_MAX_FRAGS16  /* ref: MAX_SKB_FRAGS in kernel
> driver */
> >> +
> >> void net_tx_pkt_init(struct NetTxPkt **pkt, PCIDevice *pci_dev,
> >>uint32_t max_frags, bool has_virt_hdr) {
> >> +assert(max_frags <= NET_PKT_MAX_FRAGS);
> >>struct NetTxPkt *p = g_malloc0(sizeof *p);
> >>
> >>p->pci_dev = pci_dev;
> >> --
> >> 2.5.5
> >>
> >