pf: Avoid logging state creation failures unless requested

[markj]

Summary

pd.act.log is applied unconditionally, but the intent in commit
886396f1b1a7 was to log only if the rule specifically requested it.
Thus, check the rule and associated NAT rule before setting
PF_LOG_FORCE.

Extend the regression test added in commit 886396f1b1a7 to check that we
don't log anything if a state creation failure occurs for a rule without
logging configured.

Fixes: 886396f1b1a7 ("pf: Force logging if pf_create_state() fails")

[kp]

This seems inconsistent with what we do in other situations, e.g. if pf_setup_pdesc() or vlan_set_pcp() fail.
In those cases (and a few others) we consider a failure to be sufficient reason to log, even if logging wasn't explicitly requested.

[markj]

In D49352#1125486, @kp wrote:

This seems inconsistent with what we do in other situations, e.g. if pf_setup_pdesc() or vlan_set_pcp() fail.
In those cases (and a few others) we consider a failure to be sufficient reason to log, even if logging wasn't explicitly requested.

This case is a bit different though, as pf_create_state() can fail for administrative reasons, e.g., if the rule has an associated limit. pf_setup_pdesc() and vlan_set_pcp() fail due to kernel memory allocation failures and malformed packets, which are always abnormal conditions where unconditional logging makes more sense.

What do you think of having pf_create_state() pass reason back up to the caller and using its value to decide whether to log? That is, if the value is PFRES_SRCLIMIT or MAXSTATES (and perhaps STATEINS), log only if the rule says so, otherwise log unconditionally.

[kp]

In D49352#1125548, @markj wrote:

This case is a bit different though, as pf_create_state() can fail for administrative reasons, e.g., if the rule has an associated limit. pf_setup_pdesc() and vlan_set_pcp() fail due to kernel memory allocation failures and malformed packets, which are always abnormal conditions where unconditional logging makes more sense.

We can fail pf_setup_pdesc() for a number of reasons. Some of them are indeed allocation problems, but others are also 'this packet is invalid', e.g. when it's a fragmented jumbo packet, or has multiple fragmentation headers, a route header type 0, invalid UDP port number, or a number of SCTP protocol violations.

What do you think of having pf_create_state() pass reason back up to the caller and using its value to decide whether to log? That is, if the value is PFRES_SRCLIMIT or MAXSTATES (and perhaps STATEINS), log only if the rule says so, otherwise log unconditionally.

That's make the error handling significantly more complex, in code that's already significantly complex.

It's not quite clear to me what problem we're trying to solve here.