[v2] net: remove an assert call in eth_get_gso_type

[PATCH v2] 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, as 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 | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

Update v2: add qemu_log()
  -> https://lists.nongnu.org/archive/html/qemu-devel/2020-10/msg05576.html

diff --git a/net/eth.c b/net/eth.c
index 0c1d413ee2..fd76e349eb 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,7 @@ 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("Probably not GSO frame, unknown L3 protocol: %hd\n", l3_proto);

     return VIRTIO_NET_HDR_GSO_NONE | ecn_state;
 }
--
2.26.2

Re: [PATCH v2] net: remove an assert call in eth_get_gso_type

Posted by Philippe Mathieu-Daudé 3 years, 6 months ago

On 10/20/20 4:00 PM, 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, as 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 | 5 ++---
>   1 file changed, 2 insertions(+), 3 deletions(-)
> 
> Update v2: add qemu_log()
>    -> https://lists.nongnu.org/archive/html/qemu-devel/2020-10/msg05576.html
> 
> diff --git a/net/eth.c b/net/eth.c
> index 0c1d413ee2..fd76e349eb 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,7 @@ 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("Probably not GSO frame, unknown L3 protocol: %hd\n", l3_proto);

Not sure why you choose decimal, the usual format is "0x%04"PRIx16.
Otherwise:
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> 
>       return VIRTIO_NET_HDR_GSO_NONE | ecn_state;
>   }
> --
> 2.26.2
>

Re: [PATCH v2] net: remove an assert call in eth_get_gso_type

Posted by Peter Maydell 3 years, 6 months ago

On Tue, 20 Oct 2020 at 15:05, P J P <ppandit@redhat.com> 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, as 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 | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
>
> Update v2: add qemu_log()
>   -> https://lists.nongnu.org/archive/html/qemu-devel/2020-10/msg05576.html
>
> diff --git a/net/eth.c b/net/eth.c
> index 0c1d413ee2..fd76e349eb 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,7 @@ 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("Probably not GSO frame, unknown L3 protocol: %hd\n", l3_proto);

It's generally not a good idea to use qemu_log() without a
particular mask, as then it will get printed if the user turns
on any logging but not otherwise.

If the guest must have done something wrong to get us here:
 use LOG_GUEST_ERROR
If this is some functionality we ought to implement but have
not, and so something will now be broken:
 use LOG_UNIMP
If the fallback for what happens in this situation is fine,
and maybe it's just suboptimal performance, or an unusual
case that might be interesting to know about but which
we're handling within the spec:
 consider a tracepoint instead

thanks
-- PMM

Re: [PATCH v2] net: remove an assert call in eth_get_gso_type

Posted by P J P 3 years, 6 months ago

+-- On Tue, 20 Oct 2020, Peter Maydell wrote --+
| If the guest must have done something wrong to get us here:
|  use LOG_GUEST_ERROR

+-- On Tue, 20 Oct 2020, Philippe Mathieu-Daudé wrote --+
| Not sure why you choose decimal, the usual format is "0x%04"PRIx16.

Sent patch v3 with above updates.

Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D

Re: [PATCH v2] net: remove an assert call in eth_get_gso_type

Posted by Philippe Mathieu-Daudé 3 years, 6 months ago

Hi Peter, Stefan,

On 10/20/20 5:05 PM, Peter Maydell wrote:
> On Tue, 20 Oct 2020 at 15:05, P J P <ppandit@redhat.com> 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, as 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 | 5 ++---
>>   1 file changed, 2 insertions(+), 3 deletions(-)
>>
>> Update v2: add qemu_log()
>>    -> https://lists.nongnu.org/archive/html/qemu-devel/2020-10/msg05576.html
>>
>> diff --git a/net/eth.c b/net/eth.c
>> index 0c1d413ee2..fd76e349eb 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,7 @@ 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("Probably not GSO frame, unknown L3 protocol: %hd\n", l3_proto);
> 
> It's generally not a good idea to use qemu_log() without a
> particular mask, as then it will get printed if the user turns
> on any logging but not otherwise.
> 
> If the guest must have done something wrong to get us here:
>   use LOG_GUEST_ERROR
> If this is some functionality we ought to implement but have
> not, and so something will now be broken:
>   use LOG_UNIMP
> If the fallback for what happens in this situation is fine,
> and maybe it's just suboptimal performance, or an unusual
> case that might be interesting to know about but which
> we're handling within the spec:
>   consider a tracepoint instead

During the last 2 years I've been sending patches touching
various QEMU areas, but I never used qemu_log(). I always
used:
- qemu_log_mask(LOG_GUEST_ERROR/LOG_UNIMP, ...
- error_report/warn_report from "qemu/error-report.h"
- error_setg* from "qapi/error.h"
- trace events

$ git grep qemu_log\( | wc -l
661

This function seems used mostly by very old code.

It is declared in "qemu/log-for-trace.h" which looks like
an internal API.

Should we add a checkpatch rule to refuse new uses of qemu_log()?

Regards,

Phil.

Re: [PATCH v2] 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, Philippe Mathieu-Daudé wrote --+
| $ git grep qemu_log\( | wc -l
| 661
| 
| This function seems used mostly by very old code.
| It is declared in "qemu/log-for-trace.h" which looks like an internal API.
| 
| Should we add a checkpatch rule to refuse new uses of qemu_log()?

* That'll help, if it's not meant to be called directly.

* Better still would be to make qemu_log() static, called only via 
  qemu_log_mask(). That way compiler will show an error if qemu_log() is 
  called directly.

* While at it, could it be made to print '__func__' string by default?


Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D

Re: [PATCH v2] 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 07:29, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
> During the last 2 years I've been sending patches touching
> various QEMU areas, but I never used qemu_log(). I always
> used:
> - qemu_log_mask(LOG_GUEST_ERROR/LOG_UNIMP, ...
> - error_report/warn_report from "qemu/error-report.h"
> - error_setg* from "qapi/error.h"
> - trace events
>
> $ git grep qemu_log\( | wc -l
> 661
>
> This function seems used mostly by very old code.
>
> It is declared in "qemu/log-for-trace.h" which looks like
> an internal API.
>
> Should we add a checkpatch rule to refuse new uses of qemu_log()?

The major use for qemu_log() is when you're constructing
a multi-line log message or a log message which needs to
do some expensive calculations to work out what it is going
to print. In that case the pattern is:

    if (qemu_loglevel_mask(LOG_WHATEVER)) {
        int x = do_my_expensive_calculations();
        qemu_log("line one: foo: 0x%x\n", x);
        for (some loop over a list) {
           qemu_log("and another line per list item\n");
        }
    }

For really complicated logging you might abstract out
the middle bit into functions which call qemu_log()
directly and which are only called inside a check that
some particular log level is enabled.

The uses in tcg/tcg.c are examples of this pattern.

The thing to avoid is a plain qemu_log() call which is
not already guarded by some check on the log-level mask.
You're right that the really common case is fine with
just qemu_log_mask(), but sometimes you need to be
able to split up the "is log level X enabled" and
"log" parts of the task.

thanks
-- PMM

Re: [PATCH v2] net: remove an assert call in eth_get_gso_type

Posted by Alexander Bulekov 3 years, 6 months ago

Also reported here in May: https://bugs.launchpad.net/qemu/+bug/1878067
-Alex

On 201020 1930, 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, as 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 | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> Update v2: add qemu_log()
>   -> https://lists.nongnu.org/archive/html/qemu-devel/2020-10/msg05576.html
> 
> diff --git a/net/eth.c b/net/eth.c
> index 0c1d413ee2..fd76e349eb 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,7 @@ 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("Probably not GSO frame, unknown L3 protocol: %hd\n", l3_proto);
> 
>      return VIRTIO_NET_HDR_GSO_NONE | ecn_state;
>  }
> --
> 2.26.2
> 
>