pf: Convert struct pf_addr_wrap before sending it over netlink

[vegeta_tuxpowered.net]

Summary

The struct pf_addr_wrap when used inside of kernel operates on pointers to
tables or interfaces. When reading a ruleset the struct must contain
counters calculated from the aforementioned tables and interfaces. Both the
pointers and the resulting counters are stored in an union and thus can't be
present in the struct at the same time.

The original ioctl code handles this by making a copy of struct pf_addr_wrap
for pool addresses, accessing the table or interface structures by their
pointers, calculating the counter values and storing them in place of those
pointers in the copy. Then this copy is sent over ioctl.

Use this mechanism for netlink too. Create a copy of src/dst addresses. Use
the existing function pf_addr_copyout() to convert pointers to counters both
for src/dst and pool addresses.

[kp]

If I understand things correctly the commit message is a little misleading. There's no issue with the pointer values because we don't export those to userspace, but there does appear to be a mistake in nlattr_add_addr_wrap():437 where the condition doesn't match what's in pf_tbladdr_copyout().

We could just fix that, but your approach reduces code duplication (at the price of an extra bcopy()) as well as fix the bug, so it's probably the way to go.

How did that problem manifest (i.e. can we add a test case for it)?

[vegeta_tuxpowered.net]

In D46291#1055755, @kp wrote:

If I understand things correctly the commit message is a little misleading.

I'll add a note about unions to the commit message, hopefully that will make it clearer.

There's no issue with the pointer values because we don't export those to userspace

The structure pf_addr_wrap has the following union:

	union {
		struct pfi_dynaddr	*dyn;
		struct pfr_ktable	*tbl;
		int			 dyncnt;
		int			 tblcnt;
	}

which makes impossible to have both pointers and counters at the same time. When used in-kernel the union is treated as having pointers. When sent to userspace, via ioctl or netlink, it must be treated as having counters. We suffer because of having identical struct for in-kernel and userspace.

How did that problem manifest (i.e. can we add a test case for it)?

I've encountered errors with reading a ruleset with pfctl -qvvsr:

1. Rule pass in quick log on n_test_h_rtr route-to (n_srv_h_rtr <change_dst>) sticky-address from any to <dst> keep state, error in handling the rule pool:

pf_handle_get_addr()->nlattr_add_addr_wrap() in pf_nl.c line 438 caused kernel panic.

When a rule pool is being read and sent over netlink, the following code is called:

pf_handle_get_addr() {
	pf_ioctl_get_addr(&attrs) {
		pf_kpooladdr_to_pooladdr(pa, &pp->addr); <-- copies data to another "userspace" struct
		pf_addr_copyout(&pp->addr.addr); <-- removes the pointer, sets p.tblcnt
                PF_RULES_RUNLOCK();
	}
	nlattr_add_pool_addr() {
		nlattr_add_addr_wrap(nw, PF_PA_ADDR, &a->addr); <-- panic
		nlattr_add_string(nw, PF_PA_IFNAME, a->ifname);
	}
}

The panic in nlattr_add_addr_wrap() happens because it tries to access the pointers which are gone since pf_addr_copyout() has replaced them with counters. And there is no need to access them at all, the counters are already filled in.

2. Rule pass in quick log on n_test_h_rtr route-to (n_srv_h_rtr <change_dst>) sticky-address from any to <dst> keep state, error in handling the rule destination , when reading with -vv the counters were broken:

When a rule src or dst address is read and sent over netlink, the following code is called:

pf_handle_getrule() {
	nlattr_add_rule_addr() {
		nlattr_add_addr_wrap(nw, PF_RAT_ADDR, &r->addr);
        }
}

With the previous error for pools fixed, now src/dst addresses are not handled properly. Originally nlattr_add_addr_wrap() would follow the pointers and update PF_AT_DYNCNT and PF_AT_TBLCNT counters. Now it expects the counters to be already propery set. I make a copy of struct pf_addr_wrap and modify it to have counters instead of pointers by calling pf_addr_copyout().

[kp]

Ahh, I see. I can reproduce this, and I think I understand the issue now.

I am wondering if we shouldn't do a little more re-arranging to prevent future confusion like this. That is, I think the in-kernel structure shouldn't have the union at all, and we should separate what we use in the kernel from what we copy to userspace in the ioctl handler. (There's already an implicit separation for the new netlink code, because we don't tend to copy structures to userspace there, and some similar work was done for other structures when I changed counters to use counter_u64).

Sooner or later the ioctl layer will go away (I'm still hoping that'll be soon after we branch stable/15), and then this union will serve no purpose at all.
That's looking like it'll be a more significant change though, so we shouldn't gate this fix on that.

I'll also see about adding a test case for this.

[kp]

In D46291#1055806, @kp wrote:

I am wondering if we shouldn't do a little more re-arranging to prevent future confusion like this. That is, I think the in-kernel structure shouldn't have the union at all, and we should separate what we use in the kernel from what we copy to userspace in the ioctl handler. (There's already an implicit separation for the new netlink code, because we don't tend to copy structures to userspace there, and some similar work was done for other structures when I changed counters to use counter_u64).

I've had a go at that, and my current diff is pretty substantial already and it doesn't even build yet (so it would need more changes).
Given the aspiration to remove the ioctl code in the medium term I'm inclined to just make a note to revisit the union bits once that's done.

I'll also see about adding a test case for this.

I'll write the test case and then commit this and it.