| Message ID | 20201021060550.1652896-1-ppandit@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] net: remove an assert call in eth_get_gso_type | expand |
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 > >
+-- 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
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 > >
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
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 --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; }