Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

kernel: netfilter: swap iifidx and oifidx #2266

Open
wants to merge 1 commit into
base: master
from

Conversation

@psyborg55
Copy link
Contributor

commented Jul 24, 2019

Since upstream commit netfilter: nft_flow_offload: fix interaction
with vrf slave device flow offload has been broken on 4.19
Simple swap of iifidx and oifidx restores it to a working state.
Fixes FS#2389
Tested on Archer C7 v1

Signed-off-by: Tomislav Požega pozega.tomislav@gmail.com

kernel: netfilter: swap iifidx and oifidx
Since upstream commit netfilter: nft_flow_offload: fix interaction
with vrf slave device flow offload has been broken on 4.19
Simple swap of iifidx and oifidx restores it to a working state.
Fixes FS#2389
Tested on Archer C7 v1

Signed-off-by: Tomislav Požega <pozega.tomislav@gmail.com>
@dengqf6

This comment has been minimized.

Copy link
Contributor

commented Jul 24, 2019

Please send the patch upstream first, then backport it here

@ynezz ynezz added kernel fix labels Jul 24, 2019

@psyborg55

This comment has been minimized.

Copy link
Contributor Author

commented Jul 24, 2019

not sure if it is proper fix for upstream, thus placed it into hack dir. maybe it should be discussed with netfilter maintainers first

@ynezz

This comment has been minimized.

Copy link
Member

commented Jul 24, 2019

not sure if it is proper fix for upstream, thus placed it into hack dir

Indeed, I've the same feeling, patches over patches, not a fan of this approach exactly. We probably need to adjust the patches accordingly instead of this workaround.

maybe it should be discussed with netfilter maintainers first

Yes, that would be welcome, perhaps @ummakynes ?

@exuuwen

This comment has been minimized.

Copy link

commented Jul 26, 2019

I think the original peice of code is correct. The iifdx should be set as the index of oppo-dir-route's dev. and the oifdx should be set as the index of the dst's dev.
Can you tell us what is the problem and howto reproduce?

@ynezz

This comment has been minimized.

Copy link
Member

commented Jul 26, 2019

@exuuwen Thanks a lot for showing here!

Can you tell us what is the problem and howto reproduce?

I think, that there is issue with our patches(hacks) related to the flow offloading support, which simply needs to be adjusted after your upstream changes:

More background leading to this fix attempt:

@exuuwen

This comment has been minimized.

Copy link

commented Jul 26, 2019

@ynezz The upstream flow table is not support hardware offload. So the three patch you posted is not upstream. I thin there is no problem in my upstream patch software offload mode. Maybe the three hardware offload patches is not work with that? I can check it with the three patch

@exuuwen

This comment has been minimized.

Copy link

commented Jul 26, 2019

@ynezz
So u not using the nftables. u should set the this dst and other dst like in net/netfilter/nft_flow_offload.c @nft_flow_route

I think the problem is in the target/linux/generic/hack-4.19/650-netfilter-add-xt_OFFLOAD-target.patch. In line 304, why it get the this route dst through "ct->tuplehash[dir].tuple.src.u3.ip"?

294 +static struct dst_entry *
295 +xt_flowoffload_dst(const struct nf_conn *ct, enum ip_conntrack_dir dir,
296 + const struct xt_action_param *par)
297 +{
298 + struct dst_entry *dst = NULL;
299 + struct flowi fl;
300 +
301 + memset(&fl, 0, sizeof(fl));
302 + switch (xt_family(par)) {
303 + case NFPROTO_IPV4:
304 + fl.u.ip4.daddr = ct->tuplehash[dir].tuple.src.u3.ip;
305 + break;
306 + case NFPROTO_IPV6:
307 + fl.u.ip6.saddr = ct->tuplehash[dir].tuple.dst.u3.in6;
308 + fl.u.ip6.daddr = ct->tuplehash[dir].tuple.src.u3.in6;
309 + break;
310 + }
311 +
312 + nf_route(xt_net(par), &dst, &fl, false, xt_family(par));
313 +
314 + return dst;
315 +}
316 +
317 +static int
318 +xt_flowoffload_route(struct sk_buff *skb, const struct nf_conn *ct,
319 + const struct xt_action_param *par,
320 + struct nf_flow_route *route, enum ip_conntrack_dir dir)
321 +{
322 + struct dst_entry *this_dst, *other_dst;
323 +
324 + this_dst = xt_flowoffload_dst(ct, dir, par);
325 + other_dst = xt_flowoffload_dst(ct, !dir, par);
326 + if (!this_dst || !other_dst)
327 + return -ENOENT;
328 +
329 + if (dst_xfrm(this_dst) || dst_xfrm(other_dst))
330 + return -EINVAL;
331 +
332 + route->tuple[dir].dst = this_dst;
333 + route->tuple[!dir].dst = other_dst;

@MOZGIII

This comment has been minimized.

Copy link

commented Aug 8, 2019

Hello, I created a kernel bug some time ago for the related issue: https://bugzilla.kernel.org/show_bug.cgi?id=204507
Would be great if people with more OpenWRT development experience participated there, as my knowledge might not be enough to properly assist.

@psyborg55

This comment has been minimized.

Copy link
Contributor Author

commented Aug 27, 2019

i think this should be merged, at least to prevent more bugreports related to FO, e.g. https://bugs.openwrt.org/index.php?do=details&task_id=2467&order=id&sort=desc
when there is proper fix in upstream this patch can be easily dropped

@jeffsf

This comment has been minimized.

Copy link
Contributor

commented Sep 18, 2019

Confirmed working on ipq40xx EA8300 with very significant gains in throughput seen (over 300 Mbps increase in multi-stream, TCP throughput).

SQM continues to operate, balancing the streams as expected.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.