Re: [Qemu-devel] [PATCH 4/4] rtl8139: fix Pointer to local outside scope
On 2014/11/20 15:50, Jason Wang wrote: Maybe just initialize iov unconditionally at the beginning and check dot1q_buf instead of iov for the rest of the functions. (Need deal with size ETHER_ADDR_LEN * 2) More complicated, because we can't initialize iov when size ETHER_ADDR_LEN * 2. Best regards, -Gonglei Probably not: you can just do something like: if (dot1q_buf size ETHER_ADDR_LEN * 2) { dot1q_buf = NULL; } and check dot1q_buf afterwards. Or just drop the packet since its size was less than mininum frame length that Ethernet allows. Sorry, I don't understand. But, what's your meaning initialize iov unconditionally at the beginning? Best regards, -Gonglei
Re: [Qemu-devel] [PATCH 4/4] rtl8139: fix Pointer to local outside scope
On 11/20/2014 04:05 PM, Gonglei wrote: On 2014/11/20 15:50, Jason Wang wrote: Maybe just initialize iov unconditionally at the beginning and check dot1q_buf instead of iov for the rest of the functions. (Need deal with size ETHER_ADDR_LEN * 2) More complicated, because we can't initialize iov when size ETHER_ADDR_LEN * 2. Best regards, -Gonglei Probably not: you can just do something like: if (dot1q_buf size ETHER_ADDR_LEN * 2) { dot1q_buf = NULL; } and check dot1q_buf afterwards. Or just drop the packet since its size was less than mininum frame length that Ethernet allows. Sorry, I don't understand. But, what's your meaning initialize iov unconditionally at the beginning? Something like: @@ -1774,7 +1774,12 @@ static uint32_t rtl8139_RxConfig_read(RTL8139State *s) static void rtl8139_transfer_frame(RTL8139State *s, uint8_t *buf, int size, int do_interrupt, const uint8_t *dot1q_buf) { -struct iovec *iov = NULL; +struct iovec iov[3] = { +{ .iov_base = buf, .iov_len = ETHER_ADDR_LEN * 2 }, +{ .iov_base = (void *) dot1q_buf, .iov_len = VLAN_HLEN }, +{ .iov_base = buf + ETHER_ADDR_LEN * 2, + .iov_len = size - ETHER_ADDR_LEN * 2 }, +}; and assign dot1q_buf to NULL is size is not ok. Just a suggestion, your call.
Re: [Qemu-devel] [PATCH 4/4] rtl8139: fix Pointer to local outside scope
On 2014/11/20 16:11, Jason Wang wrote: On 11/20/2014 04:05 PM, Gonglei wrote: On 2014/11/20 15:50, Jason Wang wrote: Maybe just initialize iov unconditionally at the beginning and check dot1q_buf instead of iov for the rest of the functions. (Need deal with size ETHER_ADDR_LEN * 2) More complicated, because we can't initialize iov when size ETHER_ADDR_LEN * 2. Best regards, -Gonglei Probably not: you can just do something like: if (dot1q_buf size ETHER_ADDR_LEN * 2) { dot1q_buf = NULL; } and check dot1q_buf afterwards. Or just drop the packet since its size was less than mininum frame length that Ethernet allows. Sorry, I don't understand. But, what's your meaning initialize iov unconditionally at the beginning? Something like: @@ -1774,7 +1774,12 @@ static uint32_t rtl8139_RxConfig_read(RTL8139State *s) static void rtl8139_transfer_frame(RTL8139State *s, uint8_t *buf, int size, int do_interrupt, const uint8_t *dot1q_buf) { -struct iovec *iov = NULL; +struct iovec iov[3] = { +{ .iov_base = buf, .iov_len = ETHER_ADDR_LEN * 2 }, +{ .iov_base = (void *) dot1q_buf, .iov_len = VLAN_HLEN }, +{ .iov_base = buf + ETHER_ADDR_LEN * 2, + .iov_len = size - ETHER_ADDR_LEN * 2 }, +}; and assign dot1q_buf to NULL is size is not ok. If size ETHER_ADDR_LEN * 2, .iov_len = size - ETHER_ADDR_LEN * 2 will be negative value; and if dot1q_buf is NULL, .iov_base = (void *) dot1q_buf will be NULL too. Any side-effect? Just a suggestion, your call. Thanks, Jason :) Best regards, -Gonglei
Re: [Qemu-devel] [PATCH 4/4] rtl8139: fix Pointer to local outside scope
On 11/20/2014 04:18 PM, Gonglei wrote: On 2014/11/20 16:11, Jason Wang wrote: On 11/20/2014 04:05 PM, Gonglei wrote: On 2014/11/20 15:50, Jason Wang wrote: Maybe just initialize iov unconditionally at the beginning and check dot1q_buf instead of iov for the rest of the functions. (Need deal with size ETHER_ADDR_LEN * 2) More complicated, because we can't initialize iov when size ETHER_ADDR_LEN * 2. Best regards, -Gonglei Probably not: you can just do something like: if (dot1q_buf size ETHER_ADDR_LEN * 2) { dot1q_buf = NULL; } and check dot1q_buf afterwards. Or just drop the packet since its size was less than mininum frame length that Ethernet allows. Sorry, I don't understand. But, what's your meaning initialize iov unconditionally at the beginning? Something like: @@ -1774,7 +1774,12 @@ static uint32_t rtl8139_RxConfig_read(RTL8139State *s) static void rtl8139_transfer_frame(RTL8139State *s, uint8_t *buf, int size, int do_interrupt, const uint8_t *dot1q_buf) { -struct iovec *iov = NULL; +struct iovec iov[3] = { +{ .iov_base = buf, .iov_len = ETHER_ADDR_LEN * 2 }, +{ .iov_base = (void *) dot1q_buf, .iov_len = VLAN_HLEN }, +{ .iov_base = buf + ETHER_ADDR_LEN * 2, + .iov_len = size - ETHER_ADDR_LEN * 2 }, +}; and assign dot1q_buf to NULL is size is not ok. If size ETHER_ADDR_LEN * 2, .iov_len = size - ETHER_ADDR_LEN * 2 will be negative value; and if dot1q_buf is NULL, .iov_base = (void *) dot1q_buf will be NULL too. Any side-effect? Then you need check dot1q_buf instead of iov after. Iov won't be used if dot1q_buf is NULL. Just a suggestion, your call. Thanks, Jason :) Best regards, -Gonglei
Re: [Qemu-devel] [PATCH 4/4] rtl8139: fix Pointer to local outside scope
On 2014/11/20 16:24, Jason Wang wrote: On 11/20/2014 04:18 PM, Gonglei wrote: On 2014/11/20 16:11, Jason Wang wrote: On 11/20/2014 04:05 PM, Gonglei wrote: On 2014/11/20 15:50, Jason Wang wrote: Maybe just initialize iov unconditionally at the beginning and check dot1q_buf instead of iov for the rest of the functions. (Need deal with size ETHER_ADDR_LEN * 2) More complicated, because we can't initialize iov when size ETHER_ADDR_LEN * 2. Best regards, -Gonglei Probably not: you can just do something like: if (dot1q_buf size ETHER_ADDR_LEN * 2) { dot1q_buf = NULL; } and check dot1q_buf afterwards. Or just drop the packet since its size was less than mininum frame length that Ethernet allows. Sorry, I don't understand. But, what's your meaning initialize iov unconditionally at the beginning? Something like: @@ -1774,7 +1774,12 @@ static uint32_t rtl8139_RxConfig_read(RTL8139State *s) static void rtl8139_transfer_frame(RTL8139State *s, uint8_t *buf, int size, int do_interrupt, const uint8_t *dot1q_buf) { -struct iovec *iov = NULL; +struct iovec iov[3] = { +{ .iov_base = buf, .iov_len = ETHER_ADDR_LEN * 2 }, +{ .iov_base = (void *) dot1q_buf, .iov_len = VLAN_HLEN }, +{ .iov_base = buf + ETHER_ADDR_LEN * 2, + .iov_len = size - ETHER_ADDR_LEN * 2 }, +}; and assign dot1q_buf to NULL is size is not ok. If size ETHER_ADDR_LEN * 2, .iov_len = size - ETHER_ADDR_LEN * 2 will be negative value; and if dot1q_buf is NULL, .iov_base = (void *) dot1q_buf will be NULL too. Any side-effect? Then you need check dot1q_buf instead of iov after. Iov won't be used if dot1q_buf is NULL. But that's hacking IMHO. Let's don't do this. ;) Just a suggestion, your call. Thanks, Jason :) Best regards, -Gonglei
Re: [Qemu-devel] [PATCH 4/4] rtl8139: fix Pointer to local outside scope
On 20/11/2014 09:24, Jason Wang wrote: On 11/20/2014 04:18 PM, Gonglei wrote: On 2014/11/20 16:11, Jason Wang wrote: On 11/20/2014 04:05 PM, Gonglei wrote: On 2014/11/20 15:50, Jason Wang wrote: Maybe just initialize iov unconditionally at the beginning and check dot1q_buf instead of iov for the rest of the functions. (Need deal with size ETHER_ADDR_LEN * 2) More complicated, because we can't initialize iov when size ETHER_ADDR_LEN * 2. Best regards, -Gonglei Probably not: you can just do something like: if (dot1q_buf size ETHER_ADDR_LEN * 2) { dot1q_buf = NULL; } and check dot1q_buf afterwards. Or just drop the packet since its size was less than mininum frame length that Ethernet allows. Sorry, I don't understand. But, what's your meaning initialize iov unconditionally at the beginning? Something like: @@ -1774,7 +1774,12 @@ static uint32_t rtl8139_RxConfig_read(RTL8139State *s) static void rtl8139_transfer_frame(RTL8139State *s, uint8_t *buf, int size, int do_interrupt, const uint8_t *dot1q_buf) { -struct iovec *iov = NULL; +struct iovec iov[3] = { +{ .iov_base = buf, .iov_len = ETHER_ADDR_LEN * 2 }, +{ .iov_base = (void *) dot1q_buf, .iov_len = VLAN_HLEN }, +{ .iov_base = buf + ETHER_ADDR_LEN * 2, + .iov_len = size - ETHER_ADDR_LEN * 2 }, +}; and assign dot1q_buf to NULL is size is not ok. If size ETHER_ADDR_LEN * 2, .iov_len = size - ETHER_ADDR_LEN * 2 will be negative value; and if dot1q_buf is NULL, .iov_base = (void *) dot1q_buf will be NULL too. Any side-effect? Then you need check dot1q_buf instead of iov after. Iov won't be used if dot1q_buf is NULL. Or just ignore the rules and use an initializer in the middle of the code: if (size ETHER_ADDR_LEN * 2) { dot1q_buf = NULL; } struct iovec iov[3] = { ... }; and then check dot1q_buf instead of iov. Plenty of choices, choose your favorite. Paolo
Re: [Qemu-devel] [PATCH 4/4] rtl8139: fix Pointer to local outside scope
On 20/11/2014 06:57, arei.gong...@huawei.com wrote: From: Gonglei arei.gong...@huawei.com Coverity spot: Assigning: iov = struct iovec [3]({{buf, 12UL}, {(void *)dot1q_buf, 4UL}, {buf + 12, size - 12}}) (address of temporary variable of type struct iovec [3]). out_of_scope: Temporary variable of type struct iovec [3] goes out of scope. Pointer to local outside scope (RETURN_LOCAL) use_invalid: Using iov, which points to an out-of-scope temporary variable of type struct iovec [3]. Signed-off-by: Gonglei arei.gong...@huawei.com --- hw/net/rtl8139.c | 36 1 file changed, 12 insertions(+), 24 deletions(-) diff --git a/hw/net/rtl8139.c b/hw/net/rtl8139.c index 8b8a1b1..426171b 100644 --- a/hw/net/rtl8139.c +++ b/hw/net/rtl8139.c @@ -1775,6 +1775,8 @@ static void rtl8139_transfer_frame(RTL8139State *s, uint8_t *buf, int size, int do_interrupt, const uint8_t *dot1q_buf) { struct iovec *iov = NULL; +size_t buf2_size; +uint8_t *buf2 = NULL; if (!size) { @@ -1789,35 +1791,21 @@ static void rtl8139_transfer_frame(RTL8139State *s, uint8_t *buf, int size, { .iov_base = buf + ETHER_ADDR_LEN * 2, .iov_len = size - ETHER_ADDR_LEN * 2 }, }; -} -if (TxLoopBack == (s-TxConfig TxLoopBack)) -{ -size_t buf2_size; -uint8_t *buf2; - -if (iov) { -buf2_size = iov_size(iov, 3); -buf2 = g_malloc(buf2_size); -iov_to_buf(iov, 3, 0, buf2, buf2_size); -buf = buf2; -} +buf2_size = iov_size(iov, 3); +buf2 = g_malloc(buf2_size); +iov_to_buf(iov, 3, 0, buf2, buf2_size); +buf = buf2; +} This makes rtl8139 even slower than it is for the vlantag case, using a bounce buffer for every packet. Perhaps another solution could be to do struct iovec *iov = NULL; struct iovec vlan_iov[3]; ... if (dot1q_buf size = ETHER_ADDR_LEN * 2) { ... memcpy(vlan_iov, (struct iovec[3]) { ... }, sizeof(vlan_iov)); iov = vlan_iov; } (I think vlan_iov = (struct iovec[3]) { ... }; does not work, but I may be wrong). Stefan, what do you think? Paolo +if (TxLoopBack == (s-TxConfig TxLoopBack)) { DPRINTF(+++ transmit loopback mode\n); rtl8139_do_receive(qemu_get_queue(s-nic), buf, size, do_interrupt); - -if (iov) { -g_free(buf2); -} -} -else -{ -if (iov) { -qemu_sendv_packet(qemu_get_queue(s-nic), iov, 3); -} else { -qemu_send_packet(qemu_get_queue(s-nic), buf, size); -} +} else { +qemu_send_packet(qemu_get_queue(s-nic), buf, size); } + +g_free(buf2); } static int rtl8139_transmit_one(RTL8139State *s, int descriptor)
Re: [Qemu-devel] [PATCH 4/4] rtl8139: fix Pointer to local outside scope
On 11/20/2014 02:29 PM, Paolo Bonzini wrote: On 20/11/2014 06:57, arei.gong...@huawei.com wrote: From: Gonglei arei.gong...@huawei.com Coverity spot: Assigning: iov = struct iovec [3]({{buf, 12UL}, {(void *)dot1q_buf, 4UL}, {buf + 12, size - 12}}) (address of temporary variable of type struct iovec [3]). out_of_scope: Temporary variable of type struct iovec [3] goes out of scope. Pointer to local outside scope (RETURN_LOCAL) use_invalid: Using iov, which points to an out-of-scope temporary variable of type struct iovec [3]. Signed-off-by: Gonglei arei.gong...@huawei.com --- hw/net/rtl8139.c | 36 1 file changed, 12 insertions(+), 24 deletions(-) diff --git a/hw/net/rtl8139.c b/hw/net/rtl8139.c index 8b8a1b1..426171b 100644 --- a/hw/net/rtl8139.c +++ b/hw/net/rtl8139.c @@ -1775,6 +1775,8 @@ static void rtl8139_transfer_frame(RTL8139State *s, uint8_t *buf, int size, int do_interrupt, const uint8_t *dot1q_buf) { struct iovec *iov = NULL; +size_t buf2_size; +uint8_t *buf2 = NULL; if (!size) { @@ -1789,35 +1791,21 @@ static void rtl8139_transfer_frame(RTL8139State *s, uint8_t *buf, int size, { .iov_base = buf + ETHER_ADDR_LEN * 2, .iov_len = size - ETHER_ADDR_LEN * 2 }, }; -} -if (TxLoopBack == (s-TxConfig TxLoopBack)) -{ -size_t buf2_size; -uint8_t *buf2; - -if (iov) { -buf2_size = iov_size(iov, 3); -buf2 = g_malloc(buf2_size); -iov_to_buf(iov, 3, 0, buf2, buf2_size); -buf = buf2; -} +buf2_size = iov_size(iov, 3); +buf2 = g_malloc(buf2_size); +iov_to_buf(iov, 3, 0, buf2, buf2_size); +buf = buf2; +} This makes rtl8139 even slower than it is for the vlantag case, using a bounce buffer for every packet. Perhaps another solution could be to do struct iovec *iov = NULL; struct iovec vlan_iov[3]; ... if (dot1q_buf size = ETHER_ADDR_LEN * 2) { ... memcpy(vlan_iov, (struct iovec[3]) { ... }, sizeof(vlan_iov)); iov = vlan_iov; } (I think vlan_iov = (struct iovec[3]) { ... }; does not work, but I may be wrong). Stefan, what do you think? Paolo Maybe just initialize iov unconditionally at the beginning and check dot1q_buf instead of iov for the rest of the functions. (Need deal with size ETHER_ADDR_LEN * 2)
Re: [Qemu-devel] [PATCH 4/4] rtl8139: fix Pointer to local outside scope
On 2014/11/20 14:55, Jason Wang wrote: On 11/20/2014 02:29 PM, Paolo Bonzini wrote: On 20/11/2014 06:57, arei.gong...@huawei.com wrote: From: Gonglei arei.gong...@huawei.com Coverity spot: Assigning: iov = struct iovec [3]({{buf, 12UL}, {(void *)dot1q_buf, 4UL}, {buf + 12, size - 12}}) (address of temporary variable of type struct iovec [3]). out_of_scope: Temporary variable of type struct iovec [3] goes out of scope. Pointer to local outside scope (RETURN_LOCAL) use_invalid: Using iov, which points to an out-of-scope temporary variable of type struct iovec [3]. Signed-off-by: Gonglei arei.gong...@huawei.com --- hw/net/rtl8139.c | 36 1 file changed, 12 insertions(+), 24 deletions(-) diff --git a/hw/net/rtl8139.c b/hw/net/rtl8139.c index 8b8a1b1..426171b 100644 --- a/hw/net/rtl8139.c +++ b/hw/net/rtl8139.c @@ -1775,6 +1775,8 @@ static void rtl8139_transfer_frame(RTL8139State *s, uint8_t *buf, int size, int do_interrupt, const uint8_t *dot1q_buf) { struct iovec *iov = NULL; +size_t buf2_size; +uint8_t *buf2 = NULL; if (!size) { @@ -1789,35 +1791,21 @@ static void rtl8139_transfer_frame(RTL8139State *s, uint8_t *buf, int size, { .iov_base = buf + ETHER_ADDR_LEN * 2, .iov_len = size - ETHER_ADDR_LEN * 2 }, }; -} -if (TxLoopBack == (s-TxConfig TxLoopBack)) -{ -size_t buf2_size; -uint8_t *buf2; - -if (iov) { -buf2_size = iov_size(iov, 3); -buf2 = g_malloc(buf2_size); -iov_to_buf(iov, 3, 0, buf2, buf2_size); -buf = buf2; -} +buf2_size = iov_size(iov, 3); +buf2 = g_malloc(buf2_size); +iov_to_buf(iov, 3, 0, buf2, buf2_size); +buf = buf2; +} This makes rtl8139 even slower than it is for the vlantag case, using a bounce buffer for every packet. Perhaps another solution could be to do Indeed. Your approach is better. :) struct iovec *iov = NULL; struct iovec vlan_iov[3]; ... if (dot1q_buf size = ETHER_ADDR_LEN * 2) { ... memcpy(vlan_iov, (struct iovec[3]) { ... }, sizeof(vlan_iov)); iov = vlan_iov; } (I think vlan_iov = (struct iovec[3]) { ... }; does not work, Yes. the same reason with the original issue. but I may be wrong). Stefan, what do you think? Paolo Maybe just initialize iov unconditionally at the beginning and check dot1q_buf instead of iov for the rest of the functions. (Need deal with size ETHER_ADDR_LEN * 2) More complicated, because we can't initialize iov when size ETHER_ADDR_LEN * 2. Best regards, -Gonglei
Re: [Qemu-devel] [PATCH 4/4] rtl8139: fix Pointer to local outside scope
On 11/20/2014 03:12 PM, Gonglei wrote: On 2014/11/20 14:55, Jason Wang wrote: On 11/20/2014 02:29 PM, Paolo Bonzini wrote: On 20/11/2014 06:57, arei.gong...@huawei.com wrote: From: Gonglei arei.gong...@huawei.com Coverity spot: Assigning: iov = struct iovec [3]({{buf, 12UL}, {(void *)dot1q_buf, 4UL}, {buf + 12, size - 12}}) (address of temporary variable of type struct iovec [3]). out_of_scope: Temporary variable of type struct iovec [3] goes out of scope. Pointer to local outside scope (RETURN_LOCAL) use_invalid: Using iov, which points to an out-of-scope temporary variable of type struct iovec [3]. Signed-off-by: Gonglei arei.gong...@huawei.com --- hw/net/rtl8139.c | 36 1 file changed, 12 insertions(+), 24 deletions(-) diff --git a/hw/net/rtl8139.c b/hw/net/rtl8139.c index 8b8a1b1..426171b 100644 --- a/hw/net/rtl8139.c +++ b/hw/net/rtl8139.c @@ -1775,6 +1775,8 @@ static void rtl8139_transfer_frame(RTL8139State *s, uint8_t *buf, int size, int do_interrupt, const uint8_t *dot1q_buf) { struct iovec *iov = NULL; +size_t buf2_size; +uint8_t *buf2 = NULL; if (!size) { @@ -1789,35 +1791,21 @@ static void rtl8139_transfer_frame(RTL8139State *s, uint8_t *buf, int size, { .iov_base = buf + ETHER_ADDR_LEN * 2, .iov_len = size - ETHER_ADDR_LEN * 2 }, }; -} -if (TxLoopBack == (s-TxConfig TxLoopBack)) -{ -size_t buf2_size; -uint8_t *buf2; - -if (iov) { -buf2_size = iov_size(iov, 3); -buf2 = g_malloc(buf2_size); -iov_to_buf(iov, 3, 0, buf2, buf2_size); -buf = buf2; -} +buf2_size = iov_size(iov, 3); +buf2 = g_malloc(buf2_size); +iov_to_buf(iov, 3, 0, buf2, buf2_size); +buf = buf2; +} This makes rtl8139 even slower than it is for the vlantag case, using a bounce buffer for every packet. Perhaps another solution could be to do Indeed. Your approach is better. :) struct iovec *iov = NULL; struct iovec vlan_iov[3]; ... if (dot1q_buf size = ETHER_ADDR_LEN * 2) { ... memcpy(vlan_iov, (struct iovec[3]) { ... }, sizeof(vlan_iov)); iov = vlan_iov; } (I think vlan_iov = (struct iovec[3]) { ... }; does not work, Yes. the same reason with the original issue. but I may be wrong). Stefan, what do you think? Paolo Maybe just initialize iov unconditionally at the beginning and check dot1q_buf instead of iov for the rest of the functions. (Need deal with size ETHER_ADDR_LEN * 2) More complicated, because we can't initialize iov when size ETHER_ADDR_LEN * 2. Best regards, -Gonglei Probably not: you can just do something like: if (dot1q_buf size ETHER_ADDR_LEN * 2) { dot1q_buf = NULL; } and check dot1q_buf afterwards. Or just drop the packet since its size was less than mininum frame length that Ethernet allows.