Re: [OpenWrt-Devel] Missing skb->dst with flow offloading
On Wed, May 30, 2018 at 8:24 PM, Pablo Neira Ayuso wrote: > May it crash the kernel because it's assuming is set? If so, then > I'd appreciate if you send us a patch to I suspect it won't crash, but the pmtu might wind up wrong / not calculated. > Please, use the nf-next.git tree to patch nf_flow_offload_ip_hook() > and nf_flow_offload_ip6_hook(), it's rather late, we'll request a > -stable submission for this if needed. Given the above, I'll submit a patch, though I don't suppose it will be necessary for -stable. ___ openwrt-devel mailing list openwrt-devel@lists.openwrt.org https://lists.openwrt.org/listinfo/openwrt-devel
Re: [OpenWrt-Devel] Missing skb->dst with flow offloading
On Wed, May 30, 2018 at 08:14:42PM +0200, Jason A. Donenfeld wrote: > Hey Pablo, > > On Wed, May 30, 2018 at 8:05 PM, Pablo Neira Ayuso > wrote: > > If there a more drivers in-tree that need this, we may add > > skb_dst_set_noref() calls to _hook function in the flowtable codebase. > > Can I, then, take that as an implicit acknowledgement that this > observed behavior on OpenWRT is to be expected with the current state > of events, and that I should patch my driver accordingly? > > As one example of this in tree, take a look at vxlan -- it's using it > for the mtu/pmtu exactly as WireGuard does. May it crash the kernel because it's assuming is set? If so, then I'd appreciate if you send us a patch to netfilter-de...@vger.kernel.org. Please, use the nf-next.git tree to patch nf_flow_offload_ip_hook() and nf_flow_offload_ip6_hook(), it's rather late, we'll request a -stable submission for this if needed. Thanks. ___ openwrt-devel mailing list openwrt-devel@lists.openwrt.org https://lists.openwrt.org/listinfo/openwrt-devel
Re: [OpenWrt-Devel] Missing skb->dst with flow offloading
Hey Pablo, On Wed, May 30, 2018 at 8:05 PM, Pablo Neira Ayuso wrote: > If there a more drivers in-tree that need this, we may add > skb_dst_set_noref() calls to _hook function in the flowtable codebase. Can I, then, take that as an implicit acknowledgement that this observed behavior on OpenWRT is to be expected with the current state of events, and that I should patch my driver accordingly? As one example of this in tree, take a look at vxlan -- it's using it for the mtu/pmtu exactly as WireGuard does. Regards, Jason ___ openwrt-devel mailing list openwrt-devel@lists.openwrt.org https://lists.openwrt.org/listinfo/openwrt-devel
Re: [OpenWrt-Devel] Missing skb->dst with flow offloading
Hi Jason, On Wed, May 30, 2018 at 02:01:05AM +0200, Jason A. Donenfeld wrote: > Hey Pablo, > > Some OpenWRT people have reported to me that there's a crash when > enabling flow offloading, because I rely on skb_dst(skb) being > non-null in ndo_start_xmit. The fix in my code for this is very > simple: > > - mtu = dst_mtu(skb_dst(skb)); > + dst = skb_dst(skb); > + mtu = dst ? dst_mtu(dst) : dev->mtu; > > I can make this change, but I wanted to be certain first that omitting > the dst in the skb is intentional on your part. (If so, there might be > other drivers to fix as well.) In tracing this, it looks like a packet > that's forwarded from a flow offloaded interface to a virtual > interface gets diverted immediately via neigh_xmit, where it is then > passed to a virtual interface via dev_queue_xmit. I can't see anywhere > along this path a call to skb_dst_set. Perhaps this is intended, as > flow offloading is supposed to skip the routing table? Or is there an > oversight in the new flow offloading code? > > I'd appreciate your input, so that I can make the appropriate change > -- or not -- to my code. If there a more drivers in-tree that need this, we may add skb_dst_set_noref() calls to _hook function in the flowtable codebase. ___ openwrt-devel mailing list openwrt-devel@lists.openwrt.org https://lists.openwrt.org/listinfo/openwrt-devel
[OpenWrt-Devel] Missing skb->dst with flow offloading
Hey Pablo, Some OpenWRT people have reported to me that there's a crash when enabling flow offloading, because I rely on skb_dst(skb) being non-null in ndo_start_xmit. The fix in my code for this is very simple: - mtu = dst_mtu(skb_dst(skb)); + dst = skb_dst(skb); + mtu = dst ? dst_mtu(dst) : dev->mtu; I can make this change, but I wanted to be certain first that omitting the dst in the skb is intentional on your part. (If so, there might be other drivers to fix as well.) In tracing this, it looks like a packet that's forwarded from a flow offloaded interface to a virtual interface gets diverted immediately via neigh_xmit, where it is then passed to a virtual interface via dev_queue_xmit. I can't see anywhere along this path a call to skb_dst_set. Perhaps this is intended, as flow offloading is supposed to skip the routing table? Or is there an oversight in the new flow offloading code? I'd appreciate your input, so that I can make the appropriate change -- or not -- to my code. Regards, Jason ___ openwrt-devel mailing list openwrt-devel@lists.openwrt.org https://lists.openwrt.org/listinfo/openwrt-devel
