| Message ID | 162548620436.40409.579366497986013480.stgit@wsfd-netdev64.ntdv.lab.eng.bos.redhat.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev,v2] 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 |
On Mon, Jul 05, 2021 at 07:57:41AM -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> > --- Nice, thanks Eelco. Acked-by: Flavio Leitner <fbl@sysclose.org>
Hi Ian, This one affects TSO which I think you have interest. If you're okay with the patch, could you please merge it? Thanks, fbl On Mon, Jul 05, 2021 at 07:57:41AM -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> > --- > v2: Fixed rx->aux_bufs[i] to allow reuse > > lib/netdev-linux.c | 20 +++++++++++++++++--- > 1 file changed, 17 insertions(+), 3 deletions(-) > > diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c > index 07ece0c7f..d5e693464 100644 > --- a/lib/netdev-linux.c > +++ b/lib/netdev-linux.c > @@ -1292,14 +1292,28 @@ netdev_linux_batch_rxq_recv_sock(struct netdev_rxq_linux *rx, int mtu, > for (i = 0; i < retval; i++) { > struct dp_packet *pkt; > > - if (mmsgs[i].msg_len < ETH_HEADER_LEN) { > + if (mmsgs[i].msg_hdr.msg_flags & MSG_TRUNC > + || mmsgs[i].msg_len < ETH_HEADER_LEN) { > struct netdev *netdev_ = netdev_rxq_get_netdev(&rx->up); > struct netdev_linux *netdev = netdev_linux_cast(netdev_); > > + /* The rx->aux_bufs[i] will be re-used next time. */ > dp_packet_delete(buffers[i]); > netdev->rx_dropped += 1; > - VLOG_WARN_RL(&rl, "%s: Dropped packet: less than ether hdr size", > - netdev_get_name(netdev_)); > + 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. */ > + VLOG_WARN_RL(&rl, > + "%s: Dropped packet: Too big. GRO/TSO enabled?", > + netdev_get_name(netdev_)); > + } else { > + VLOG_WARN_RL(&rl, > + "%s: Dropped packet: less than ether hdr size", > + netdev_get_name(netdev_)); > + } > + > continue; > } > > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
On 7/8/21 2:16 PM, Flavio Leitner wrote: > > Hi Ian, > > This one affects TSO which I think you have interest. > > If you're okay with the patch, could you please merge it? I applied it. > > Thanks, > fbl > > On Mon, Jul 05, 2021 at 07:57:41AM -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> >> --- >> v2: Fixed rx->aux_bufs[i] to allow reuse >> >> lib/netdev-linux.c | 20 +++++++++++++++++--- >> 1 file changed, 17 insertions(+), 3 deletions(-) Thanks, Eelco and Flavio! I extended the commit message a bit with more details why exactly this happens. I also added a different Fixes tag, because the actual culprit for the issue is that commit 2109841b7984 ("Use batch process recv for tap and raw socket in netdev datapath") dropped the (retval > size) check without providing an alternative while migrating from recvmsg to recvmmsg. This resulted in construction of a malformed dp_packet with size larger than the allocated space. The crash due to NULL aux_bufs was introduced later by commit 73858f9db. Applied and backported down to 2.13. Best regards, Ilya Maximets.
On Fri, Jul 09, 2021 at 10:08:39PM +0200, Ilya Maximets wrote: > On 7/8/21 2:16 PM, Flavio Leitner wrote: > > On Mon, Jul 05, 2021 at 07:57:41AM -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> > >> --- > >> v2: Fixed rx->aux_bufs[i] to allow reuse > >> > >> lib/netdev-linux.c | 20 +++++++++++++++++--- > >> 1 file changed, 17 insertions(+), 3 deletions(-) > Thanks, Eelco and Flavio! I extended the commit message a bit with more > details why exactly this happens. I also added a different Fixes tag, > because the actual culprit for the issue is that commit 2109841b7984 > ("Use batch process recv for tap and raw socket in netdev datapath") > dropped the (retval > size) check without providing an alternative while > migrating from recvmsg to recvmmsg. This resulted in construction of > a malformed dp_packet with size larger than the allocated space. > The crash due to NULL aux_bufs was introduced later by commit 73858f9db. Yup, thanks for improving the commit. > Applied and backported down to 2.13. That's great! Thanks,
diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c index 07ece0c7f..d5e693464 100644 --- a/lib/netdev-linux.c +++ b/lib/netdev-linux.c @@ -1292,14 +1292,28 @@ netdev_linux_batch_rxq_recv_sock(struct netdev_rxq_linux *rx, int mtu, for (i = 0; i < retval; i++) { struct dp_packet *pkt; - if (mmsgs[i].msg_len < ETH_HEADER_LEN) { + if (mmsgs[i].msg_hdr.msg_flags & MSG_TRUNC + || mmsgs[i].msg_len < ETH_HEADER_LEN) { struct netdev *netdev_ = netdev_rxq_get_netdev(&rx->up); struct netdev_linux *netdev = netdev_linux_cast(netdev_); + /* The rx->aux_bufs[i] will be re-used next time. */ dp_packet_delete(buffers[i]); netdev->rx_dropped += 1; - VLOG_WARN_RL(&rl, "%s: Dropped packet: less than ether hdr size", - netdev_get_name(netdev_)); + 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. */ + VLOG_WARN_RL(&rl, + "%s: Dropped packet: Too big. GRO/TSO enabled?", + netdev_get_name(netdev_)); + } else { + VLOG_WARN_RL(&rl, + "%s: Dropped packet: less than ether hdr size", + netdev_get_name(netdev_)); + } + continue; }
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> --- v2: Fixed rx->aux_bufs[i] to allow reuse lib/netdev-linux.c | 20 +++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-)