diff mbox series

[v3] net: remove an assert call in eth_get_gso_type

Message ID 20201021060550.1652896-1-ppandit@redhat.com
State Superseded
Headers show
Series [v3] net: remove an assert call in eth_get_gso_type | expand

Commit Message

Prasad Pandit Oct. 21, 2020, 6:05 a.m. UTC 
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

--
2.26.2

Comments

Jason Wang Oct. 21, 2020, 7:44 a.m. UTC | #1 
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


>
>
Prasad Pandit Oct. 21, 2020, 9:23 a.m. UTC | #2 
+-- 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
Jason Wang Oct. 26, 2020, 3:30 a.m. UTC | #3 
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
>
>
Peter Maydell Oct. 26, 2020, 9:59 a.m. UTC | #4 
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
Jason Wang Oct. 28, 2020, 2:25 a.m. UTC | #5 
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
>
diff mbox series

Patch

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;
 }