All of lore.kernel.org
 help / color / mirror / Atom feed
* [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.