Missing skb->dst with flow offloading

Missing skb->dst with flow offloading

[Jason A. Donenfeld]

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

Re: Missing skb->dst with flow offloading

[Pablo Neira Ayuso]

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.

Re: Missing skb->dst with flow offloading

[Jason A. Donenfeld]

Hey Pablo,

On Wed, May 30, 2018 at 8:05 PM, Pablo Neira Ayuso <pablo@netfilter.org> 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

Re: Missing skb->dst with flow offloading

[Pablo Neira Ayuso]

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 <pablo@netfilter.org> 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-devel@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.

Re: Missing skb->dst with flow offloading

[Jason A. Donenfeld]

On Wed, May 30, 2018 at 8:24 PM, Pablo Neira Ayuso <pablo@netfilter.org> 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.