Traffic shaper and ACK packets

[Waschbuesch]

Hi there,

I noticed something weird when trying to prioritize ACK packages.
Selecting "tcp (ACK packets only)" in the proto drop-down results in almost all tcp traffic being matched.
Doing something similar in m0n0wall or even the firewall solution that shall not be named, did not result in comparable behavior.
Though, with those solutions I could (and did) specify the packet size to something very small so only empty ACKs where prioritized. That does not seem to be an option in the OPNsense shaper currently?

If "tcp (ACK packets only)" matches any packet having the ACK flag set, then that is not (to me at least) particularly useful...

[AdSchellevis]

Hi,

yes, it matches all tcp traffic with ACK set, which is indeed most of the traffic.
M0n0wall used the same technology (tcpflags in ipfw), but offered the option to select or deselect any of them.

It was added a very long time ago (https://github.com/opnsense/core/issues/528), but doesn't look very useful indeed.

There are a couple of options, open a feature request to add flags as an option, so you can select which ones need to be set, maybe accompanied by a switch to expect exactly these flags (negate the rest). Selecting both which must be set and which may not be set (like m0n0 did) could also be an option, but in our layout can be a bit confusing.

The other option is to fix the flags on this option to something more logical (tcpflags ack,!psh ?)

Adding flag selection and handling migrations is obviously more work, currently it's not very high on my list of things todo, but we can always discuss if there's a very good reason why we should really want to prioritise this.

Best regards,

Ad

[franco]

I think TCP stacks have gotten smarter so that pure "ACK" priority isn't really useful (anymore). Technically this is neither a bug or a documentation issue as the expectation is clear and the outcome is correct.


Cheers,
Franco

[Waschbuesch]

Thanks to both of you for your replies.

@franco I agree that there is currently no discrepancy w/r to the documentation or the behavior. My point was that the functionality as it is defined right now is not particularily useful.

@AdSchellevis I guess my hope would be that eventually the shaper rule editor will allow for a more complete subset of the choices that the FW rule editor offers.
That would not only be true for tcp flags, but also specifying the type of icmp packet instead of picking them all, etc.

But certainly, all of this is just 'nice to have'.

Martin

[franco]

https://github.com/opnsense/core/issues/528

People need to know what they ask for and also follow up on the functionality if it is not adequate. 


Cheers,
Franco

[Waschbuesch]

Very OT:

There is this one (weirder than average) episode of X-Files where Mulder meets a genie and actually has three wishes.
His first one is: peace on earth
after which he is the only living human being left on the planet.
So, yes. You should be specific when you ask for something. ;-)

[franco]

True. I only want to point out that we talk about things that others did intent otherwise or wasn't completed for one reason or another. Tracking the origin might help going forward. In the case of this filtering a solution that is a bit more clever around ACK packets is probably better than adding lots of filters that only very few people will be able to configure.

At least that would be my preference in this case.


Cheers,
Franco

[Waschbuesch]

The current code:

Code: [Select]

tcpflags ack
Ad suggested:

Code: [Select]

tcpflags ack,!pshwhich I think would be an improvement.

My suggestion would be to try and narrow it down to packets doing nothing but ack by excluding larger payloads. E.g.:

Code: [Select]

tcpflags ack iplen 52
Shall I open an issue for this?

[AdSchellevis]

sure, we can work out the details in the ticket and patch when there's consensus about the improvement, the current feature isn't very useful.

[FingerlessGloves]

Where did we get too with this? just getting iplen on the rule to overcome the problem?

[franco]

Seems so, yes. https://github.com/opnsense/core/pull/4181


Cheers,
Franco

[FingerlessGloves]

Ah, so it looks like only the new iplen was added, instead of/as well as amending the protocol "tcp (ACK packets only)".

Shame it wasn't both 🤭

I've added iplen of 52 and looks much better now, although without the research makes it look like the option is broken..

Thoughts?

[franco]

Problem is it does what it says, but it's not expected behaviour when the ACK is being piggybacked on the payload.

Maybe makes sense to push this to the option, but that makes verification harder now that iplen is in there.

IMO iplen < 100 is probably sufficient for this to account for some slop in the stack.


Cheers,
Franco

[FingerlessGloves]

Yeah I see that side of it too.

Might be worth adding a note or something to the docs instead, if this is the preferred route, since you say its working but not as expected.

Thanks for the clarification, so you would recommend upping the iplen from 52 to 100?