1. Patchew
  2. QEMU
  3. View series

[PATCH v3] net: remove an assert call in eth_get_gso_type

P J P posted 1 patch 3 years, 6 months ago
Test checkpatch passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20201021060550.1652896-1-ppandit@redhat.com
Maintainers: Dmitry Fleytman <dmitry.fleytman@gmail.com>, Jason Wang <jasowang@redhat.com>
net/eth.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
[PATCH v3] net: remove an assert call in eth_get_gso_type
Posted by P J P 3 years, 6 months ago 
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
Re: [PATCH v3] net: remove an assert call in eth_get_gso_type
Posted by Jason Wang 3 years, 6 months ago 
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


>
>
Re: [PATCH v3] net: remove an assert call in eth_get_gso_type
Posted by P J P 3 years, 6 months ago 
+-- 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
Re: [PATCH v3] net: remove an assert call in eth_get_gso_type
Posted by Jason Wang 3 years, 6 months ago 
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
>
>
Re: [PATCH v3] net: remove an assert call in eth_get_gso_type
Posted by Peter Maydell 3 years, 6 months ago 
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
Re: [PATCH v3] net: remove an assert call in eth_get_gso_type
Posted by Jason Wang 3 years, 6 months ago 
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
>

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.