* [PATCH v3] net: remove an assert call in eth_get_gso_type
@ 2020-10-21 6:05 P J P
2020-10-21 7:44 ` Jason Wang
0 siblings, 1 reply; 6+ messages in thread
From: P J P @ 2020-10-21 6:05 UTC (permalink / raw)
To: Jason Wang
Cc: Peter Maydell, Gaoning Pan, Philippe Mathieu-Daudé,
QEMU Developers, Prasad J Pandit
From: Prasad J Pandit <pjp@fedoraproject.org>
eth_get_gso_type() routine returns segmentation offload type based on
L3 protocol type. It calls g_assert_not_reached if L3 protocol is
unknown, making the following return statement unreachable. Remove the
g_assert call, it maybe triggered by a guest user.
Reported-by: Gaoning Pan <pgn@zju.edu.cn>
Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
---
net/eth.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
Update v3: use LOG_GUEST_ERROR mask and %0x04PRIx16 conversion.
-> https://lists.nongnu.org/archive/html/qemu-devel/2020-10/msg05759.html
-> https://lists.nongnu.org/archive/html/qemu-devel/2020-10/msg05752.html
diff --git a/net/eth.c b/net/eth.c
index 0c1d413ee2..eee77071f9 100644
--- a/net/eth.c
+++ b/net/eth.c
@@ -16,6 +16,7 @@
*/
#include "qemu/osdep.h"
+#include "qemu/log.h"
#include "net/eth.h"
#include "net/checksum.h"
#include "net/tap.h"
@@ -71,9 +72,8 @@ eth_get_gso_type(uint16_t l3_proto, uint8_t *l3_hdr, uint8_t l4proto)
return VIRTIO_NET_HDR_GSO_TCPV6 | ecn_state;
}
}
-
- /* Unsupported offload */
- g_assert_not_reached();
+ qemu_log_mask(LOG_GUEST_ERROR, "%s: probably not GSO frame, "
+ "unknown L3 protocol: 0x%04"PRIx16"\n", __func__, l3_proto);
return VIRTIO_NET_HDR_GSO_NONE | ecn_state;
}
--
2.26.2
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH v3] net: remove an assert call in eth_get_gso_type
2020-10-21 6:05 [PATCH v3] net: remove an assert call in eth_get_gso_type P J P
@ 2020-10-21 7:44 ` Jason Wang
2020-10-21 9:23 ` P J P
0 siblings, 1 reply; 6+ messages in thread
From: Jason Wang @ 2020-10-21 7:44 UTC (permalink / raw)
To: P J P
Cc: Peter Maydell, Gaoning Pan, Philippe Mathieu-Daudé,
QEMU Developers, Prasad J Pandit
On 2020/10/21 下午2:05, P J P wrote:
> From: Prasad J Pandit <pjp@fedoraproject.org>
>
> eth_get_gso_type() routine returns segmentation offload type based on
> L3 protocol type. It calls g_assert_not_reached if L3 protocol is
> unknown, making the following return statement unreachable. Remove the
> g_assert call, it maybe triggered by a guest user.
>
> Reported-by: Gaoning Pan <pgn@zju.edu.cn>
> Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
> ---
> net/eth.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> Update v3: use LOG_GUEST_ERROR mask and %0x04PRIx16 conversion.
> -> https://lists.nongnu.org/archive/html/qemu-devel/2020-10/msg05759.html
> -> https://lists.nongnu.org/archive/html/qemu-devel/2020-10/msg05752.html
>
> diff --git a/net/eth.c b/net/eth.c
> index 0c1d413ee2..eee77071f9 100644
> --- a/net/eth.c
> +++ b/net/eth.c
> @@ -16,6 +16,7 @@
> */
>
> #include "qemu/osdep.h"
> +#include "qemu/log.h"
> #include "net/eth.h"
> #include "net/checksum.h"
> #include "net/tap.h"
> @@ -71,9 +72,8 @@ eth_get_gso_type(uint16_t l3_proto, uint8_t *l3_hdr, uint8_t l4proto)
> return VIRTIO_NET_HDR_GSO_TCPV6 | ecn_state;
> }
> }
> -
> - /* Unsupported offload */
> - g_assert_not_reached();
> + qemu_log_mask(LOG_GUEST_ERROR, "%s: probably not GSO frame, "
> + "unknown L3 protocol: 0x%04"PRIx16"\n", __func__, l3_proto);
>
> return VIRTIO_NET_HDR_GSO_NONE | ecn_state;
> }
> --
> 2.26.2
Hi Prasad:
If I understand the code correctly. It should not be a guest error,
since guest is allowed to send a packet other than IPV4(6).
Thanks
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3] net: remove an assert call in eth_get_gso_type
2020-10-21 7:44 ` Jason Wang
@ 2020-10-21 9:23 ` P J P
2020-10-26 3:30 ` Jason Wang
2020-10-26 9:59 ` Peter Maydell
0 siblings, 2 replies; 6+ messages in thread
From: P J P @ 2020-10-21 9:23 UTC (permalink / raw)
To: Jason Wang
Cc: Peter Maydell, Gaoning Pan, Philippe Mathieu-Daudé, QEMU Developers
+-- On Wed, 21 Oct 2020, Jason Wang wrote --+
| It should not be a guest error, since guest is allowed to send a packet
| other than IPV4(6).
* Ah...sigh! :(
* I very hesitantly used guest_error mask, since it was g_assert-ing before.
To me both guest_error and log_unimp seem mismatching. Because no GSO is
also valid IIUC. That's why in patch v2 I used plain qemu_log(). But plain
qemu_log is also not good it seems.
* I'm okay either way. Please let me know which one to use. OR I'm fine if you
fix it while merging upstream too.
Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3] net: remove an assert call in eth_get_gso_type
2020-10-21 9:23 ` P J P
@ 2020-10-26 3:30 ` Jason Wang
2020-10-26 9:59 ` Peter Maydell
1 sibling, 0 replies; 6+ messages in thread
From: Jason Wang @ 2020-10-26 3:30 UTC (permalink / raw)
To: P J P
Cc: Peter Maydell, Gaoning Pan, Philippe Mathieu-Daudé, QEMU Developers
On 2020/10/21 下午5:23, P J P wrote:
> +-- On Wed, 21 Oct 2020, Jason Wang wrote --+
> | It should not be a guest error, since guest is allowed to send a packet
> | other than IPV4(6).
>
> * Ah...sigh! :(
>
> * I very hesitantly used guest_error mask, since it was g_assert-ing before.
> To me both guest_error and log_unimp seem mismatching. Because no GSO is
> also valid IIUC. That's why in patch v2 I used plain qemu_log(). But plain
> qemu_log is also not good it seems.
>
> * I'm okay either way. Please let me know which one to use. OR I'm fine if you
> fix it while merging upstream too.
I see.
I applied the patch as is.
Thanks
>
>
> Thank you.
> --
> Prasad J Pandit / Red Hat Product Security Team
> 8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3] net: remove an assert call in eth_get_gso_type
2020-10-21 9:23 ` P J P
2020-10-26 3:30 ` Jason Wang
@ 2020-10-26 9:59 ` Peter Maydell
2020-10-28 2:25 ` Jason Wang
1 sibling, 1 reply; 6+ messages in thread
From: Peter Maydell @ 2020-10-26 9:59 UTC (permalink / raw)
To: P J P
Cc: Jason Wang, Gaoning Pan, Philippe Mathieu-Daudé, QEMU Developers
On Wed, 21 Oct 2020 at 10:23, P J P <ppandit@redhat.com> wrote:
>
> +-- On Wed, 21 Oct 2020, Jason Wang wrote --+
> | It should not be a guest error, since guest is allowed to send a packet
> | other than IPV4(6).
>
> * Ah...sigh! :(
>
> * I very hesitantly used guest_error mask, since it was g_assert-ing before.
> To me both guest_error and log_unimp seem mismatching. Because no GSO is
> also valid IIUC. That's why in patch v2 I used plain qemu_log(). But plain
> qemu_log is also not good it seems.
Well, as I said last time round, the right function depends on what
is going on here. If this is "the fallback code path is fine, it
might just be a bit inefficient", then either no logging or use
a tracepoint. If this is "the guest is allowed to send this packet
but we're going to mishandle it" then use LOG_UNIMP.
thanks
-- PMM
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3] net: remove an assert call in eth_get_gso_type
2020-10-26 9:59 ` Peter Maydell
@ 2020-10-28 2:25 ` Jason Wang
0 siblings, 0 replies; 6+ messages in thread
From: Jason Wang @ 2020-10-28 2:25 UTC (permalink / raw)
To: Peter Maydell, P J P
Cc: Gaoning Pan, Philippe Mathieu-Daudé, QEMU Developers
On 2020/10/26 下午5:59, Peter Maydell wrote:
> On Wed, 21 Oct 2020 at 10:23, P J P <ppandit@redhat.com> wrote:
>> +-- On Wed, 21 Oct 2020, Jason Wang wrote --+
>> | It should not be a guest error, since guest is allowed to send a packet
>> | other than IPV4(6).
>>
>> * Ah...sigh! :(
>>
>> * I very hesitantly used guest_error mask, since it was g_assert-ing before.
>> To me both guest_error and log_unimp seem mismatching. Because no GSO is
>> also valid IIUC. That's why in patch v2 I used plain qemu_log(). But plain
>> qemu_log is also not good it seems.
> Well, as I said last time round, the right function depends on what
> is going on here. If this is "the fallback code path is fine, it
> might just be a bit inefficient", then either no logging or use
> a tracepoint. If this is "the guest is allowed to send this packet
> but we're going to mishandle it" then use LOG_UNIMP.
Ok, rethink about this. I think at least 802.1Q is a valid option for GSO.
So I decide to apply the path with LOG_UNIMP.
Thanks
>
> thanks
> -- PMM
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-10-28 2:26 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-21 6:05 [PATCH v3] net: remove an assert call in eth_get_gso_type P J P
2020-10-21 7:44 ` Jason Wang
2020-10-21 9:23 ` P J P
2020-10-26 3:30 ` Jason Wang
2020-10-26 9:59 ` Peter Maydell
2020-10-28 2:25 ` Jason Wang
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.