| Message ID | 162523574862.28549.11301540064982906102.stgit@wsfd-netdev64.ntdv.lab.eng.bos.redhat.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev] netdev-linux: Ignore TSO packets when TSO is not enabled for userspace | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot | success | github build: passed |
Hi, Thanks for fixing this bug! On Fri, Jul 02, 2021 at 10:22:36AM -0400, Eelco Chaudron wrote: > When TSO is disabled from a userspace forwarding datapath perspective, > but TSO has been wrongly enabled on the kernel side, log a warning > message, and drop the packet. With the current implementation, > OVS will crash. > > Fixes: 73858f9db ("netdev-linux: Prepend the std packet in the TSO packet") > Signed-off-by: Eelco Chaudron <echaudro@redhat.com> > --- > lib/netdev-linux.c | 17 +++++++++++++++++ > 1 file changed, 17 insertions(+) > > diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c > index 07ece0c7f..6dc98cae5 100644 > --- a/lib/netdev-linux.c > +++ b/lib/netdev-linux.c > @@ -1303,6 +1303,23 @@ netdev_linux_batch_rxq_recv_sock(struct netdev_rxq_linux *rx, int mtu, > continue; > } > > + if (mmsgs[i].msg_hdr.msg_flags & MSG_TRUNC) { > + /* Data is truncated, so the packet is corrupted, and needs to be > + * dropped. This can happen if TSO/GRO is enabled in the kernel, > + * but not in userspace, i.e. there is no dp buffer to store the > + * full packet. */ > + struct netdev *netdev_ = netdev_rxq_get_netdev(&rx->up); > + struct netdev_linux *netdev = netdev_linux_cast(netdev_); > + > + dp_packet_delete(buffers[i]); > + dp_packet_delete(rx->aux_bufs[i]); It needs to set rx->aux_bufs[i] to NULL so that the caller can allocate a new packet, otherwise the free pointer will be re-used next time. Another option is to not free that aux_bufs and let it be re-used next time (same happens in the block above when mmsgs[i].msg_len < ETH_HEADER_LEN. fbl > + netdev->rx_dropped += 1; > + VLOG_WARN_RL(&rl, > + "%s: Dropped packet: Too big. GRO/TSO enabled?", > + netdev_get_name(netdev_)); > + continue; > + } > + > if (mmsgs[i].msg_len > std_len) { > /* Build a single linear TSO packet by prepending the data from > * std_len buffer to the aux_buf. */ >
On 3 Jul 2021, at 15:41, Flavio Leitner wrote: > Hi, > > Thanks for fixing this bug! > > On Fri, Jul 02, 2021 at 10:22:36AM -0400, Eelco Chaudron wrote: >> When TSO is disabled from a userspace forwarding datapath perspective, >> but TSO has been wrongly enabled on the kernel side, log a warning >> message, and drop the packet. With the current implementation, >> OVS will crash. >> >> Fixes: 73858f9db ("netdev-linux: Prepend the std packet in the TSO packet") >> Signed-off-by: Eelco Chaudron <echaudro@redhat.com> >> --- >> lib/netdev-linux.c | 17 +++++++++++++++++ >> 1 file changed, 17 insertions(+) >> >> diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c >> index 07ece0c7f..6dc98cae5 100644 >> --- a/lib/netdev-linux.c >> +++ b/lib/netdev-linux.c >> @@ -1303,6 +1303,23 @@ netdev_linux_batch_rxq_recv_sock(struct netdev_rxq_linux *rx, int mtu, >> continue; >> } >> >> + if (mmsgs[i].msg_hdr.msg_flags & MSG_TRUNC) { >> + /* Data is truncated, so the packet is corrupted, and needs to be >> + * dropped. This can happen if TSO/GRO is enabled in the kernel, >> + * but not in userspace, i.e. there is no dp buffer to store the >> + * full packet. */ >> + struct netdev *netdev_ = netdev_rxq_get_netdev(&rx->up); >> + struct netdev_linux *netdev = netdev_linux_cast(netdev_); >> + >> + dp_packet_delete(buffers[i]); >> + dp_packet_delete(rx->aux_bufs[i]); > > It needs to set rx->aux_bufs[i] to NULL so that the caller can > allocate a new packet, otherwise the free pointer will be re-used > next time. > > Another option is to not free that aux_bufs and let it be re-used > next time (same happens in the block above when > mmsgs[i].msg_len < ETH_HEADER_LEN. I opted for the second solution, as it made to code look better. Thanks! > > fbl > >> + netdev->rx_dropped += 1; >> + VLOG_WARN_RL(&rl, >> + "%s: Dropped packet: Too big. GRO/TSO enabled?", >> + netdev_get_name(netdev_)); >> + continue; >> + } >> + >> if (mmsgs[i].msg_len > std_len) { >> /* Build a single linear TSO packet by prepending the data from >> * std_len buffer to the aux_buf. */ >>
diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c index 07ece0c7f..6dc98cae5 100644 --- a/lib/netdev-linux.c +++ b/lib/netdev-linux.c @@ -1303,6 +1303,23 @@ netdev_linux_batch_rxq_recv_sock(struct netdev_rxq_linux *rx, int mtu, continue; } + if (mmsgs[i].msg_hdr.msg_flags & MSG_TRUNC) { + /* Data is truncated, so the packet is corrupted, and needs to be + * dropped. This can happen if TSO/GRO is enabled in the kernel, + * but not in userspace, i.e. there is no dp buffer to store the + * full packet. */ + struct netdev *netdev_ = netdev_rxq_get_netdev(&rx->up); + struct netdev_linux *netdev = netdev_linux_cast(netdev_); + + dp_packet_delete(buffers[i]); + dp_packet_delete(rx->aux_bufs[i]); + netdev->rx_dropped += 1; + VLOG_WARN_RL(&rl, + "%s: Dropped packet: Too big. GRO/TSO enabled?", + netdev_get_name(netdev_)); + continue; + } + if (mmsgs[i].msg_len > std_len) { /* Build a single linear TSO packet by prepending the data from * std_len buffer to the aux_buf. */
When TSO is disabled from a userspace forwarding datapath perspective, but TSO has been wrongly enabled on the kernel side, log a warning message, and drop the packet. With the current implementation, OVS will crash. Fixes: 73858f9db ("netdev-linux: Prepend the std packet in the TSO packet") Signed-off-by: Eelco Chaudron <echaudro@redhat.com> --- lib/netdev-linux.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+)