Re: [PATCH] [0/1] pf refactoring

[Mike Belopuhov]

On 19 August 2016 at 16:34, Richard Procter  wrote:
> Hi Mike,
>
> On Fri, 19 Aug 2016, Mike Belopuhov wrote:
>
>> I've looked through it and couldn't find anything wrong with it.
>
> Thanks.
>
>> I do however find pacthing of values in pf_translate_icmp_af
>> unneccessary since we'll be throwing away the original header
>> anyway.
>
> Do you mean e.g. circa line 2602, pf.c:pf_translate_icmp_af()
> (HEAD + complete diff)?
>
> pf_patch_8(pd, >icmp_type, type, PF_HI);
> pf_patch_8(pd, >icmp_code, code, PF_LO);
> pf_patch_16(pd, >icmp_nextmtu, htons(mtu));
> if (ptr >= 0)
> pf_patch_32(pd, >icmp_void, htonl(ptr));
>
> My understanding is that the ICMP v4 and v6 headers are so similar that pf
> af-to hacks the one into the other; the code above is filling an ICMPv4
> header with v6 values; the updates are not redundant. It muddled me a bit
> when I was converting it.
>

I see now that you're not recalculating checksums after pf_translate_af,
but rather update them in place.  As long as it works, it's fine with me :)

>>
>> OK mikeb for the diff.
>
> best,
> Richard.

Re: [PATCH] [0/1] pf refactoring

[Richard Procter]

Hi Mike, 

On Fri, 19 Aug 2016, Mike Belopuhov wrote:

> I've looked through it and couldn't find anything wrong with it.

Thanks. 

> I do however find pacthing of values in pf_translate_icmp_af
> unneccessary since we'll be throwing away the original header
> anyway.

Do you mean e.g. circa line 2602, pf.c:pf_translate_icmp_af() 
(HEAD + complete diff)?

pf_patch_8(pd, >icmp_type, type, PF_HI);
pf_patch_8(pd, >icmp_code, code, PF_LO);
pf_patch_16(pd, >icmp_nextmtu, htons(mtu));
if (ptr >= 0)
pf_patch_32(pd, >icmp_void, htonl(ptr));

My understanding is that the ICMP v4 and v6 headers are so similar that pf 
af-to hacks the one into the other; the code above is filling an ICMPv4 
header with v6 values; the updates are not redundant. It muddled me a bit 
when I was converting it.

> 
> OK mikeb for the diff.

best, 
Richard. 

Re: [PATCH] [0/1] pf refactoring

[Mike Belopuhov]

On 19 August 2016 at 11:33, Richard Procter  wrote:
> Hi,
>
> I've reduced the pf refactor (phase two) to two patches, which I'll be
> committing in 24 hours or so unless there are any objections.
>
> I'm confident it won't, but supposing post-commit these have in
> fact blown up, my first suspect would be the afto paths.
>

I've looked through it and couldn't find anything wrong with it.
I do however find pacthing of values in pf_translate_icmp_af
unneccessary since we'll be throwing away the original header
anyway.

OK mikeb for the diff.

>
> This patch removes pf_translate_ap().
> The 'working' appears at http://203.79.107.124/ in the 012*.diff files.
>
> best,
> Richard.
>

[PATCH] [0/1] pf refactoring

[Richard Procter]

Hi, 

I've reduced the pf refactor (phase two) to two patches, which I'll be 
committing in 24 hours or so unless there are any objections.

I'm confident it won't, but supposing post-commit these have in 
fact blown up, my first suspect would be the afto paths.


This patch removes pf_translate_ap(). 
The 'working' appears at http://203.79.107.124/ in the 012*.diff files.

best, 
Richard. 

Index: net/pf.c
===
--- net.orig/pf.c
+++ net/pf.c
@@ -154,8 +154,6 @@ int  pf_check_tcp_cksum(struct mbuf *,
sa_family_t);
 static __inline voidpf_cksum_fixup(u_int16_t *, u_int16_t, u_int16_t,
u_int8_t);
-voidpf_translate_ap(struct pf_pdesc *, struct pf_addr *,
-   u_int16_t *, struct pf_addr *, u_int16_t);
 voidpf_cksum_fixup_a(u_int16_t *, const struct pf_addr *,
const struct pf_addr *, sa_family_t, u_int8_t);
 int pf_modulate_sack(struct pf_pdesc *,
@@ -1910,16 +1908,6 @@ pf_patch_32(struct pf_pdesc *pd, u_int32
 }
 
 void
-pf_translate_ap(struct pf_pdesc *pd, struct pf_addr *a, u_int16_t *p,
-struct pf_addr *an, u_int16_t pn)
-{
-   if (pd->af == pd->naf)
-   pf_translate_a(pd, a, an);
-   if (p != NULL)
-   pf_patch_16(pd, p, pn);
-}
-
-void
 pf_patch_32_unaligned(struct pf_pdesc *pd, void *f, u_int32_t v, bool hi)
 {
u_int8_t *fb = (u_int8_t*)f;
@@ -4009,12 +3997,16 @@ pf_translate(struct pf_pdesc *pd, struct
case IPPROTO_TCP:
if (afto || PF_ANEQ(saddr, pd->src, pd->af) ||
*pd->sport != sport) {
-   pf_translate_ap(pd, pd->src, pd->sport, saddr, sport);
+   if (pd->af == pd->naf)
+   pf_translate_a(pd, pd->src, saddr);
+   pf_patch_16(pd, pd->sport, sport);
rewrite = 1;
}
if (afto || PF_ANEQ(daddr, pd->dst, pd->af) ||
*pd->dport != dport) {
-   pf_translate_ap(pd, pd->dst, pd->dport, daddr, dport);
+   if (pd->af == pd->naf)
+   pf_translate_a(pd, pd->dst, daddr);
+   pf_patch_16(pd, pd->dport, dport);
rewrite = 1;
}
break;
@@ -4022,12 +4014,16 @@ pf_translate(struct pf_pdesc *pd, struct
case IPPROTO_UDP:
if (afto || PF_ANEQ(saddr, pd->src, pd->af) ||
*pd->sport != sport) {
-   pf_translate_ap(pd, pd->src, pd->sport, saddr, sport);
+   if (pd->af == pd->naf)
+   pf_translate_a(pd, pd->src, saddr);
+   pf_patch_16(pd, pd->sport, sport);
rewrite = 1;
}
if (afto || PF_ANEQ(daddr, pd->dst, pd->af) ||
*pd->dport != dport) {
-   pf_translate_ap(pd, pd->dst, pd->dport, daddr, dport);
+   if (pd->af == pd->naf)
+   pf_translate_a(pd, pd->dst, daddr);
+   pf_patch_16(pd, pd->dport, dport);
rewrite = 1;
}
break;
@@ -4078,11 +4074,11 @@ pf_translate(struct pf_pdesc *pd, struct
rewrite = 1;
} else {
if (PF_ANEQ(saddr, pd->src, pd->af)) {
-   pf_translate_ap(pd, pd->src, NULL, saddr, 0);
+   pf_translate_a(pd, pd->src, saddr);
rewrite = 1;
}
if (PF_ANEQ(daddr, pd->dst, pd->af)) {
-   pf_translate_ap(pd, pd->dst, NULL, daddr, 0);
+   pf_translate_a(pd, pd->dst, daddr);
rewrite = 1;
}
}
@@ -4113,11 +4109,11 @@ pf_translate(struct pf_pdesc *pd, struct
 #ifdef INET6
case AF_INET6:
if (!afto && PF_ANEQ(saddr, pd->src, pd->af)) {
-   pf_translate_ap(pd, pd->src, NULL, saddr, 0);
+   pf_translate_a(pd, pd->src, saddr);
rewrite = 1;
}
if (!afto && PF_ANEQ(daddr, pd->dst, pd->af)) {
-   pf_translate_ap(pd, pd->dst, NULL, daddr, 0);
+   pf_translate_a(pd, pd->dst, daddr);
rewrite = 1;
}
break;
@@ -4738,18 +4734,24 @@ pf_test_state(struct pf_pdesc *pd, struc
 #endif /* INET6 */
 
if (afto || PF_ANEQ(pd->src,