Re: [External] : [ICMP] IP options lead to malformed reply

2021-03-29 Thread Alexandr Nedvedicky
Hello, On Mon, Mar 29, 2021 at 11:56:40AM +, Schreilechner, Dominik wrote: > > > > > I think the changes in ip_insertoptions() can be dropped completely, > > > because the if-statement uses ip-ip_hl, which might not be initialized. > > > > yes, you are right, I think we should rather go

Re: [External] : [ICMP] IP options lead to malformed reply

2021-03-29 Thread Alexandr Nedvedicky
Hello, On Mon, Mar 29, 2021 at 08:44:26AM +, Schreilechner, Dominik wrote: > Hi, > > > > > yes, you are right. below is updated diff I would like to commit. > > > > > > Appart from that, adding a special task seems the way to go. > > > > I think so too. Alternative way would be to

Re: [External] : [ICMP] IP options lead to malformed reply

2021-03-29 Thread Alexandr Nedvedicky
Hello, > > @@ -790,6 +795,7 @@ ip_insertoptions(struct mbuf *m, struct mbuf *opt, int > > *phlen) > > memcpy(ip + 1, p->ipopt_list, optlen); > > *phlen = sizeof(struct ip) + optlen; > > ip->ip_len = htons(ntohs(ip->ip_len) + optlen); > > + ip->ip_hl += (optlen >>

Re: [External] : [ICMP] IP options lead to malformed reply

2021-03-26 Thread Alexandr Nedvedicky
Hello, > > You missed the initialization of ipsendraw_mq. Otherwise, ICMP with and > without IP options work for me with the diff. > > I don't know how this is usually handled here and it is probably a bit > nit-picky, but I introduced a new function to remove the duplicate code > in

Re: [External] : [ICMP] IP options lead to malformed reply

2021-03-25 Thread Alexandr Nedvedicky
Hello, > > 1) ip_insertoptions() does not update length of IP header (ip_hl) > > > > 2) ip_hl is being overridden anyway later in ip_output() called > > by ip_send_dispatch() to send ICMP error packet out. Looks > > like ip_send_dispatch() should use IP_RAWOUTPUT

Re: [External] : [ICMP] IP options lead to malformed reply

2021-03-24 Thread Alexandr Nedvedicky
Hello, > We really need to fix ip_send() such the output task will handle IP options > properly. took a look at bug reported by Dominik earlier today. Looks like there are two issues: 1) ip_insertoptions() does not update length of IP header (ip_hl) 2) ip_hl is being

Re: [External] : [ICMP] IP options lead to malformed reply

2021-03-24 Thread Alexandr Nedvedicky
Hello Dominik, thanks for reporting a bug. I'll take a look at it later today. your proposed fix re-introduces a recursion to PF, which we want to avoid, hence we let icmp_send() to dispatch outbound ICMP response to task. We really need to fix ip_send() such the output task will handle IP

Re: [External] : pf route-to: only run pf_test when packets enter and leave the stack

2021-02-03 Thread Alexandr Nedvedicky
Hello, > pass in on em0 from v.x.y.z/n to a.b.c.d/m \ > route-to o.p.q.r nat-to (em2) > > > then this needs to be converted to two rules: > > > > match in on em0 from v.x.y.z/n to a.b.c.d/m nat-to(em2) > > pass in on em0 from v.x.y.z/n to a.b.c.d/m route-to o.p.q.r

Re: [External] : pf route-to: only run pf_test when packets enter and leave the stack

2021-02-02 Thread Alexandr Nedvedicky
Hello, On Tue, Feb 02, 2021 at 02:52:52PM +1000, David Gwynne wrote: > > however, like most things relating to route-to/reply-to/dup-to, im > pretty sure at this point it's not used a lot, so the impact is minimal. > a lot of changes in this space have already been made, so adding another >

Re: [External] : pf: route-to IPs, not interfaces

2021-01-28 Thread Alexandr Nedvedicky
Hello David, thanks for nice wrap up of the story... > > this change does the following: > > - stores the route info in the state instead of the pf rule > > this allows route-to to keep working when the ruleset changes, and > allows route-to info to be sent over pfsync. there's enough

Re: [External] : handle PFRULE_ONCE before pfsync may defer tx of the packet

2021-01-27 Thread Alexandr Nedvedicky
Hello, On Thu, Jan 28, 2021 at 11:47:30AM +1000, David Gwynne wrote: > i think these code chunks are around the wrong way. > > pfsync may want to defer the transmission of a packet. it does this so > it can try and get a state over to a peer firewall before a host may > send a reply to the peer,

Re: [External] : if pf_route{,6} route isn't valid, generate an icmp error

2021-01-26 Thread Alexandr Nedvedicky
Hello, On Wed, Jan 27, 2021 at 04:41:01PM +1000, David Gwynne wrote: > at the moment if the route is invalid, we drop the packet. this > generates an icmp error. > > ok? looks good to me. ok sashan > > Index: pf.c > === >

Re: [External] : don't run dup-to generated packets through pf_test in pf_route{,6}

2021-01-26 Thread Alexandr Nedvedicky
On Wed, Jan 27, 2021 at 11:31:27AM +1000, David Gwynne wrote: > this was discussed as part of the big route-to issues thread. i think > it's easy to break out and handle separately now. > > the diff does what the subject line says. it seems to work as expected > for me. i don't see weird state

Re: [External] : tiny pf_route{,6} tweak

2021-01-26 Thread Alexandr Nedvedicky
On Wed, Jan 27, 2021 at 11:13:12AM +1000, David Gwynne wrote: > when pf_route (and pf_route6) are supposed to handle forwarding the > packet (ie, for route-to or reply-to rules), they take the mbuf > away from the calling code path. this is done by clearing the mbuf > pointer in the pf_pdesc

Re: [External] : Re: pf route-to issues

2021-01-26 Thread Alexandr Nedvedicky
Hello, On Tue, Jan 26, 2021 at 12:33:25PM +0100, Alexander Bluhm wrote: > On Tue, Jan 26, 2021 at 10:39:30AM +1000, David Gwynne wrote: > > > But what about dup-to? The packet is duplicated for both directions. > > > I guess the main use case for dup-to is implementing a monitor port. > > >

Re: [External] : Re: pf route-to issues

2021-01-26 Thread Alexandr Nedvedicky
Hello, On Tue, Jan 26, 2021 at 10:39:30AM +1000, David Gwynne wrote: > On Mon, Jan 25, 2021 at 04:19:11PM +0100, Alexander Bluhm wrote: > > On Fri, Jan 22, 2021 at 06:07:59PM +1000, David Gwynne wrote: > > > --- sys/conf/GENERIC 30 Sep 2020 14:51:17 - 1.273 > > > +++

Re: [External] : Re: pf route-to issues

2021-01-26 Thread Alexandr Nedvedicky
Hello, > > > goto bad; > > > > here we do 'goto bad', which does not call if_put(). > > yes it does. the whole chunk with the diff applied is: > > done: > if (s->rt != PF_DUPTO) > pd->m = NULL; > if_put(ifp); > rtfree(rt); >

Re: [External] : Re: pf route-to issues

2021-01-26 Thread Alexandr Nedvedicky
Hello, > > i'll need help with "match on em0 route-to $if_em0_peer". or we can do > that later separately? may be can we keep this line in pf_route() untouched at least for now: 6041 6042 if (pd->kif->pfik_ifp != ifp) { 6043 if (pf_test(AF_INET, PF_OUT, ifp, ) !=

Re: [External] : Re: pf route-to issues

2021-01-26 Thread Alexandr Nedvedicky
Hello, > > > > > > I'm not sure if proposed scenario real. Let's assume there > > is a PF box with three NICs running on this awkward set up > > > > em1 ... 192.168.1.10 > > > > em0 > > > > em2 ... 192.168.1.10 > > > > em0 is attached

Re: [External] : Re: pf route-to issues

2021-01-25 Thread Alexandr Nedvedicky
Hello, pf_route() might leak a refence to ifp. > Index: sys/net/pf.c > === > RCS file: /cvs/src/sys/net/pf.c,v > retrieving revision 1.1101 > diff -u -p -r1.1101 pf.c > --- sys/net/pf.c 19 Jan 2021 22:22:23 - 1.1101 >

Re: [External] : Re: pf route-to issues

2021-01-25 Thread Alexandr Nedvedicky
Hello, On Mon, Jan 25, 2021 at 03:21:29PM +0100, Alexander Bluhm wrote: > Hi, > > Some personal thoughts. I am happy when pf route-to gets simpler. > Especially I have never understood what this address@interface > syntax is used for. > > I cannot estimate what configuration is used by our

Re: [External] : Re: pf route-to issues

2021-01-25 Thread Alexandr Nedvedicky
hello, > > > +pf_route(struct pf_pdesc *pd, struct pf_state *s) > ... > > + if (pd->dir == PF_IN) { > > if (pf_test(AF_INET, PF_OUT, ifp, ) != PF_PASS) > > Yes, this is the correct logic. When the packet comes in, pf > overrides forwarding, tests the out rules, and sends it.

Re: [External] : Re: pf route-to issues

2021-01-25 Thread Alexandr Nedvedicky
Hello, > > > > If I understand the idea right, then basically 'match out on em0' > > figures out the new 'outbound interface' so either 'pass out on em1...' > > or > > 'pass out on em2...' will kick in. In other words: > > > > depending on the destination picked up from

Re: [External] : Re: pf route-to issues

2021-01-25 Thread Alexandr Nedvedicky
Hello, > > > > I understand that simple is better here, so I won't object > > if we will lean towards simplified model above. However I still > > would like to share my view on current PF. > > > > the way I understand how things (should) work currently is fairly > > simple: >

Re: [External] : Re: pf route-to issues

2021-01-24 Thread Alexandr Nedvedicky
Hello, > > ok. i don't know how to split up the rest of the change though. > > here's an updated diff that includes the rest of the kernel changes and > the pfctl and pf.conf tweaks. > > it's probably useful for me to try and explain at a high level what > i think the semantics should be,

Re: [External] : Re: tell pfctl(8) route-to and reply-to accept next-hop only

2021-01-24 Thread Alexandr Nedvedicky
hello, On Fri, Jan 22, 2021 at 05:32:47PM +1000, David Gwynne wrote: > I tried this diff, and it broke the ability to use dynamic addresses. > ie, the following rules should work: > > pass in on gre52 inet proto icmp route-to (gre49:peer) > pass in on vmx0 inet proto icmp route-to (gre:peer)

Re: tcpdump pflog af and rewritten addresses

2021-01-19 Thread Alexandr Nedvedicky
Hello, On Mon, Jan 18, 2021 at 07:47:56PM +0100, Alexander Bluhm wrote: > Hi, > > tcpdump pflog with addresses rewritten by rdr-to, nat-to, or af-to > is broken. > > 1. Fix address family of the packet in af-to rules: > > before: > 19:26:37.620926 169.254.0.14 > 169.254.0.14: icmp: echo

Re: pflog remove translation

2021-01-19 Thread Alexandr Nedvedicky
Hello, On Mon, Jan 18, 2021 at 04:47:08PM +0100, Alexander Bluhm wrote: > Hi, > > pflog(4) tries to log the translated packet with rdr-to, nat-to, > and af-to applied. Therefore it creates a mbuf chain on the stack > with a partial copy. This might have been a good idea for plain > IPv4 10

Re: tell pfctl(8) route-to and reply-to accept next-hop only

2021-01-15 Thread Alexandr Nedvedicky
Hello, On Fri, Jan 15, 2021 at 06:26:48PM +0100, Alexander Bluhm wrote: > On Tue, Jan 12, 2021 at 08:45:22PM +0100, Alexandr Nedvedicky wrote: > > I think bluhm@ and dlg@ have committed part of that change already. > > I have only commited a refactoring change. Next step in

tell pfctl(8) route-to and reply-to accept next-hop only

2021-01-12 Thread Alexandr Nedvedicky
Hello, proposed diff follows stuff discussed here [1] (pf route-to issues). I think we've reached a consensus to change route-to/reply-to such the only supported option will be next-hop (and list and table of next-hop addresses). I think bluhm@ and dlg@ have committed part of that change

Re: pf log user and group

2021-01-11 Thread Alexandr Nedvedicky
Hello, looks good to me. OK sashan On Mon, Jan 11, 2021 at 05:49:10PM +0100, Alexander Bluhm wrote: > Hi, > > Sometimes an uid is logged in pflog(4) although the logopt of the > rule does not specify it. Check the option again for the log rule > in case another rule has triggered a socket

Re: pf route-to issues

2021-01-08 Thread Alexandr Nedvedicky
Hello, > > revision 1.294 > date: 2003/01/02 01:56:56; author: dhartmei; state: Exp; lines: +27 -49; > When route-to/reply-to is used in combination with address translation, > pf_test() may be called twice for the same packet. In this case, make > sure the

Re: pf route-to issues

2021-01-07 Thread Alexandr Nedvedicky
Hello, sorry to come back after while... On Tue, Jan 05, 2021 at 10:05:39PM +1000, David Gwynne wrote: > On Mon, Jan 04, 2021 at 06:37:54PM +0100, Alexander Bluhm wrote: > > On Mon, Jan 04, 2021 at 11:21:50PM +1000, David Gwynne wrote: > > > this chunk pops out as a standalone change. > > > > >

Re: pf route-to issues

2021-01-04 Thread Alexandr Nedvedicky
Hello, On Mon, Jan 04, 2021 at 06:37:54PM +0100, Alexander Bluhm wrote: > On Mon, Jan 04, 2021 at 11:21:50PM +1000, David Gwynne wrote: > > this chunk pops out as a standalone change. > > > > having pf_find_state() return PF_PASS here means the callers short > > circuit and let the packet go

Re: pf route-to issues

2021-01-04 Thread Alexandr Nedvedicky
Hello, > My diff removes the kif here ... > > > > > - if (rv == 0) { > > > > - s->rt_kif = r->route.kif; > > > > + if (rv == 0) > > > > s->natrule.ptr = r; > > > > - } > > ... and the {}. > > Anyway, it should not be commited without the

Re: pf route-to issues

2021-01-04 Thread Alexandr Nedvedicky
Hello, I'm sorry I was not clear enough in my earlier email. On Mon, Jan 04, 2021 at 03:56:45PM +0100, Alexander Bluhm wrote: > On Mon, Jan 04, 2021 at 03:26:15PM +0100, Alexandr Nedvedicky wrote: > > you refactoring diff requires a minor finishing touch to keep the > > stuff comp

Re: pf route-to issues

2021-01-04 Thread Alexandr Nedvedicky
Hello, On Mon, Jan 04, 2021 at 01:57:24PM +0100, Alexander Bluhm wrote: > On Mon, Jan 04, 2021 at 11:46:16AM +0100, Alexandr Nedvedicky wrote: > > > let's put this in and then i'll have a look. ok by me. > > bluhm's diff is fine with me. > > Refactoring is commite

Re: pf route-to issues

2021-01-04 Thread Alexandr Nedvedicky
Hello, On Mon, Jan 04, 2021 at 11:21:50PM +1000, David Gwynne wrote: > On Mon, Jan 04, 2021 at 01:57:24PM +0100, Alexander Bluhm wrote: > > On Mon, Jan 04, 2021 at 11:46:16AM +0100, Alexandr Nedvedicky wrote: > > > > let's put this in and then i'll have a look. ok by me. &

Re: pf route-to issues

2021-01-04 Thread Alexandr Nedvedicky
Hello, there is one more thing, which just came up on my mind. > > so i want to change route-to in pfctl so it takes a nexthop instead > of an interface. you could argue that pf already lets you do this, > because there's some bs nexthop@interface syntax. my counter argument > is that the

Re: pf route-to issues

2021-01-04 Thread Alexandr Nedvedicky
Hello, > > the stack should provide it on a 4 byte boundary, but it has uint64_t > members. however, it is also __packed, so the compiler makes no > assumptions about alignment. > > > Is there anything else that can be split out easily? > > let's put this in and then i'll have a look. ok by

Re: IPv6 pf_test EACCES

2020-12-22 Thread Alexandr Nedvedicky
On Mon, Dec 21, 2020 at 11:34:04PM +0100, Alexander Bluhm wrote: > Hi, > > A while ago we decided to pass EACCES to uerland if pf blocks a > packet. IPv6 still has the old EHOSTUNREACH code. > > Use the same errno for dropped IPv6 packets as in IPv4. > > ok? > looks good to me. OK sashan

Re: Enhancing (some) PF state

2020-12-18 Thread Alexandr Nedvedicky
Hello Sven, your change makes me wonder: 'what is the actual problem you are trying to solve'? the reason I'm asking is that latency is just one factor, which contributes to TCP connection performance. The other factor (and perhaps more important) is to guess amount of retransmitted data.

Re: rw_lock_held() & friends

2020-12-07 Thread Alexandr Nedvedicky
On Mon, Dec 07, 2020 at 10:18:04PM +0100, Alexandr Nedvedicky wrote: > On Mon, Dec 07, 2020 at 12:22:22PM -0800, Philip Guenther wrote: > > On Mon, Dec 7, 2020 at 11:16 AM Alexandr Nedvedicky < > > alexandr.nedvedi...@oracle.com> wrote: > > > > > What's the

Re: rw_lock_held() & friends

2020-12-07 Thread Alexandr Nedvedicky
On Mon, Dec 07, 2020 at 12:22:22PM -0800, Philip Guenther wrote: > On Mon, Dec 7, 2020 at 11:16 AM Alexandr Nedvedicky < > alexandr.nedvedi...@oracle.com> wrote: > > > What's the plan for rw_write_held()? Currently macro is true, if whoever > > holds > > t

Re: rw_lock_held() & friends

2020-12-07 Thread Alexandr Nedvedicky
Hello, What's the plan for rw_write_held()? Currently macro is true, if whoever holds the lock. > > +#define rw_write_held(rwl) (rw_status(rwl) == RW_WRITE) I would expect something like instead: #define rw_write_held(rwl) (RWLOCK_OWNER(rwl) == curproc)

Re: PF synproxy should act on inbound packets only

2020-12-03 Thread Alexandr Nedvedicky
Hello, > > Just a style nit. Other errors do not put stdin:1 in brackes. One > line per error. In pf.conf the rule direction matters. What about > > stdin:1 warning: synproxy used for inbound rules only, ignored for outbound > thanks, I like your suggestion. below is updated diff.

PF synproxy should act on inbound packets only

2020-12-01 Thread Alexandr Nedvedicky
Hello, the issue described here has been hit bu Stuart some time ago. feel free to stop reading if you don't care/use pf(4) synproxy. let's assume there are rules which allow just surfing web over http: block all pass proto tcp from any to any port = 80 synproxy state pass proto

Re: push NET_LOCK() down in pf_ioctl.c

2020-10-19 Thread Alexandr Nedvedicky
Hello Klemens, > > the change is fairly large, but mostly mechanical. > Relocating malloc(9) and pool(9) seems good but other pf_*() calls are > now locked inconsistently after your diff. > > Given that only reason about "allocations" this is either an oversight > and should be fixed or you

Re: pf route-to issues

2020-10-19 Thread Alexandr Nedvedicky
Hello, > > > > it seems to me 'route-to vs. pfsync' still needs more thought. the > > next-hop IP address in route-to may be different for each PF box > > linked by pfsync(4). To be honest I have no answer to address this > > issue at the moment. > > i have thought about that

Re: pf route-to issues

2020-10-19 Thread Alexandr Nedvedicky
Hello, disclaimer: I have no chance to run pfsync on production, I'm very inexperienced with pfsync(4). > > the third problem is that pf_route relies on information from rules to > work correctly. this is a problem in a pfsync environment because you > cannot have the same ruleset on both

push NET_LOCK() down in pf_ioctl.c

2020-10-16 Thread Alexandr Nedvedicky
Hello, I've just found a forgotten diff in my tree. The diff pushes the NET_LCOK() further down in PF driver ioctl() path. The idea is to avoid sleeping while holding a NET_LOCK(). this typically may happen when we need to allocate memory. The diff is the first step as it takes care of

Re: net/pfvar.h: MAXPATHLEN -> PATH_MAX

2020-10-13 Thread Alexandr Nedvedicky
Hello, On Tue, Oct 13, 2020 at 10:31:28PM +0200, Christian Weisgerber wrote: > In revision 1.407 of , deraadt@ replaced MAXPATHLEN > with PATH_MAX so userland wouldn't have to pull in . > > In 1.466, sashan@ accidentally slipped one MAXPATHLEN back in, > because its use is ubiquitous on the

Re: Non-const basename: sbin/pfctl

2020-10-13 Thread Alexandr Nedvedicky
Hello, On Tue, Oct 13, 2020 at 10:52:58PM +0200, Christian Weisgerber wrote: > Accommodate a basename(3) that takes a non-const parameter and may > in fact modify the string buffer. > > The length of anchor has already been checked in main(). > > ok? looks OK to me. sashan > > Index:

Re: diff: pfctl: error message for nonexisting rtable

2020-10-01 Thread Alexandr Nedvedicky
Hello, On Wed, Sep 30, 2020 at 11:02:28PM +0200, Klemens Nanni wrote: > On Sun, Sep 20, 2020 at 07:29:38PM +0200, Klemens Nanni wrote: > > Rebased diff after yasouka's pfctl commit; it still takes care of > > rdomains only, but I'd appreciate folks using `on rdomain' in their > > pf.conf test

Re: pf: remove ptr_array from struct pf_ruleset

2020-08-24 Thread Alexandr Nedvedicky
Hello, > > Admins using `once' rules are hopefully aware of this caveat already, > but now the checksum actually indicates out-of-sync rulesets and does > no longer present the same checksum for different rulesets. > > Feedback? OK? > > OK sashan@

Re: pf: route-to least-states

2020-07-28 Thread Alexandr Nedvedicky
Hello Yasuoka, On Wed, Jul 29, 2020 at 01:55:20AM +0900, YASUOKA Masahiko wrote: > Hi, > > Let me add another fix of previous. > > ok? OK. thanks for taking care of that. I've entirely missed 1 vs. -1 return value, when reviewing your change. regards sashan

Re: pf: route-to least-states

2020-07-28 Thread Alexandr Nedvedicky
Hello Yasuoka, On Wed, Jul 29, 2020 at 01:22:48AM +0900, YASUOKA Masahiko wrote: > Hi, > > Previous commit has a wrong part.. > > ok? > > Fix previous commit which referred wrong address. would it make sense to move the block, you've introduced earler under the !PF_AZERO() branch just

Re: pf: route-to least-states

2020-07-24 Thread Alexandr Nedvedicky
Hello Yasuoka, > > Yes. > > Let me simplify the block for "least-states". > thanks for your explanation. It helped me to understand the code. I'm OK with your fix. thanks and regards sashan

Re: pf: route-to least-states

2020-07-23 Thread Alexandr Nedvedicky
Hello Yasuoka, > - interface is not selected properly if selected table entry specifies > an interface. to be honest I don't quite understand what's going on here. can you share some details of configuration/scenario, which triggers the bug your diff is fixing? the part of

Re: pf: route-to {random,srchash} in an anchor

2020-07-23 Thread Alexandr Nedvedicky
Hello Yasuok, On Thu, Jul 23, 2020 at 08:01:18PM +0900, YASUOKA Masahiko wrote: > Hi, > > Last month, I fixed the problem "route-to least-state" in an anchor > didn't work. > > https://marc.info/?t=15911745782=1=2 > > I noticed the same problem happens on "random" and "srchash" as well. >

Re: pfctl.8: mention hostid and checksum for -s info

2020-07-20 Thread Alexandr Nedvedicky
Hello, On Mon, Jul 20, 2020 at 03:23:41PM +0200, Klemens Nanni wrote: > Getting the checksum with pfctl(8) is either in your finger's muscle > memory or takes guess work as the manual doesn't mention it. > > I grepped the code to see that I need `-s info' with `-v'. > > (Setting) hostid is

Re: pf: remove ptr_array from struct pf_ruleset

2020-07-20 Thread Alexandr Nedvedicky
Hello Klemens. I took a closer look at your change and related area. Below is an alternate way to fix the bug you've found. there are few details worth to note: ptr_array matters to main ruleset only, because it is the only ruleset covered by MD5 sum for pfsync. it looks like we

Re: pf: remove ptr_array from struct pf_ruleset

2020-07-14 Thread Alexandr Nedvedicky
Hello Klemens, > `ptr_array' is allocated momentarily through mallocarray(9) and gets > filled with the TAILQ entries, the sole user pfsync(4) can then access > the list of rules by index to simply pick the n-th rule in a ruleset > during state insertion. > > I came here due to the zero size

Re: pf "route-to least-state" in an anchor doesn't work

2020-06-03 Thread Alexandr Nedvedicky
Hello Yasuoka, I'm OK with your change. However I would like to ask you to do yet another test. I wonder if things will eventually work on unfixed PF if rules will be constructed as follows: pfctl -a test -t LB -T add 10.0.0.11@pair102 echo 'pass in on rdomain 102 quick proto tcp to

Re: mcx(4) checksum offload

2020-05-18 Thread Alexandr Nedvedicky
Hello Jonathan, I think it makes sense to turn it on on inbound path. we certainly get a performance boost if HW verifies header checksums for us. I'm not sure about outbound path. chksum offload on outbound side got killed back in 2016 (I think). All that logic behind dealing with various HW

Re: pfctl_parser.c vs. __KAME

2020-05-03 Thread Alexandr Nedvedicky
Hello JCA, thanks for the pointers. > about this specific piece of code in pfctl, but the "kame hack" is still > here and you really want to double check before removing such chunks in ...so pfctl is the wrong place to start with removal. thanks and regards sashan

pfctl_parser.c vs. __KAME

2020-05-03 Thread Alexandr Nedvedicky
Hello, the question has popped up while on recent code review of some Solaris specific bug fixes: do we still need a code in diff below or is it OK to proceed and commit the diff? The chunk below uses bytes 2 and 3 to derive a scope of link local address. It seems to me those bytes/octets are

Re: Simplify NET_LOCK() variations

2020-04-28 Thread Alexandr Nedvedicky
Hello, > That's a bug, updated diff below. > OK I see. the diff looks better then. > If there's a consensus that this is a way to move forward, it would make > sense to commit it after unlock. > I have not spot anything else. I think this change should go in. OK sashan@

Re: Simplify NET_LOCK() variations

2020-04-16 Thread Alexandr Nedvedicky
Hello, On Thu, Apr 16, 2020 at 10:57:42AM +0200, Martin Pieuchot wrote: > On 14/04/20(Tue) 10:08, Martin Pieuchot wrote: > > On 13/04/20(Mon) 03:20, Alexandr Nedvedicky wrote: > > > On Sun, Apr 12, 2020 at 07:02:43PM +0200, Mark Kettenis wrote: > > > > > From: &qu

Re: Simplify NET_LOCK() variations

2020-04-12 Thread Alexandr Nedvedicky
Hallo, On Sun, Apr 12, 2020 at 07:02:43PM +0200, Mark Kettenis wrote: > > From: "Theo de Raadt" > > Date: Sun, 12 Apr 2020 10:28:59 -0600 > > > > > + if ((p->p_flag & P_SYSTEM) && > > > + (strncmp(p->p_p->ps_comm, "softnet", 7) == 0)) > > > > Wow that is ugly. > > A better

'pfctl -L state.file' grabs state lock recursively

2020-04-03 Thread Alexandr Nedvedicky
Hello, The issue has been found by Chris Cappuccio. The good news is it can not be triggered by default. To trigger the bug one has to build kernel with 'WITH_PF_LOCK' option. PF_STATE_ENTER_WRITE(), which is no-op by default, becomes operational, when WITH_PF_LOCK is defined. the 'pfctl -L

pf_state_key_link_reverse() needs atomic ops -- resending

2020-04-03 Thread Alexandr Nedvedicky
Hello, my apologize to resend the same diff [1]. I'm not sure I got OK or not. It can be the case the privately received OK got lost. The change is required to allow multiple instances of pf_test() running concurrently. Without this change in, PF trips 'KASSERT(sk->reverse == NULL);' thanks and

change to pfsync(4) reduces a PF_LOCK scope

2020-03-16 Thread Alexandr Nedvedicky
Hello, patch below is a fairly large change, which reduces a scope of PF_LOCK(). Whenever pf(4) needs to send state table update to its peer, it must grab a PF_LOCK() to serialize access to internal pfsync(4) queues. The patch below adds a mutex to every queue pfsync's queue, so PF_LOCK() is no

Re: sbin/pfctl: use TAILQ_CONCAT(3)

2020-01-27 Thread Alexandr Nedvedicky
Hello, On Mon, Jan 27, 2020 at 08:54:19PM +0100, Björn Ketelaars wrote: > Replace custom TAILQ concatenation loop by TAILQ_CONCAT(3). > > Comments/OK? looks good to me. OK sashan@

Re: vlan: use SMR_SLIST instead of SRP to protect instance lists

2020-01-26 Thread Alexandr Nedvedicky
Hello, > > I agree we should protect against that - if I add a sleep there, I can easily > cause a panic with 'ifconfig vlan10 destroy & ifconfig vlan10 destroy'. > I think that's a separate issue though, as the same window is present in > the existing code. > that's true. it's

Re: vlan: use SMR_SLIST instead of SRP to protect instance lists

2020-01-26 Thread Alexandr Nedvedicky
Hello, > > I'm not sure about your diff see below. > > visa@ pointed out that PF_LOCK will introduce a potential sleep inside > if_vinput(), > making this unsafe, so here's a less optimistic diff where only the vlan list > traversal is inside the SMR read section, and vlan_softcs are still

Re: SMP friendly pfsync(4) ready for testing

2020-01-26 Thread Alexandr Nedvedicky
Hello, here is a revised diff, which makes my change to play nicely with witness. Hrvoje found a panic with stack below when running my diff with witness enabled: witness: acquiring duplicate lock of same type: ">sc_mtx[q]" 1st >sc_mtx[q] 2nd >sc_mtx[q] Starting stack trace...

Re: vlan: use SMR_SLIST instead of SRP to protect instance lists

2020-01-26 Thread Alexandr Nedvedicky
Hello, On Sun, Jan 26, 2020 at 10:06:24AM +1100, Jonathan Matthew wrote: > Converting vlan(4) to use SMR instead of SRP to protect the instance lists > is pretty simple. vlan_input() doesn't keep a reference to vlan_softcs > outside > the read critical section, so we don't need to reference

Re: pf: remove 'one shot rules'

2020-01-25 Thread Alexandr Nedvedicky
Hello, mikeb@ and me were poking about same idea some time ago (?2016?). But the idea never turned to diff. If I remember correct the only meaningful use case we could come up with for once rules is [t]ftp-proxy. But neither one seems to use once rules at all. I'm OK with removing 'once' rules.

Re: MSI-X & Interrupting CPU > 0

2020-01-23 Thread Alexandr Nedvedicky
Hello, > > 3. How does interrupting multiple CPUs influence packet processing in > the softnet thread? Is any knowledge required (CPU affinity?) to > have an optimum processing when multiple softnet threads are used? > I think it's hard to tell in advance. IMO we should try to

Re: SMP friendly pfsync(4) ready for testing

2020-01-22 Thread Alexandr Nedvedicky
Hello, sorry for typo in my earlier mail. > cd sys/arch/`uname -p`/conf > echo 'option WIT_PF_LOCK' >> GENERIC.MP > config GENERIC.MP > cd ../compile/GENERIC.MP/ > make clean > make do s/WIT_PF_LOCK/WITH_PF_LOCK in the snippet above. my apologize.

SMP friendly pfsync(4) ready for testing

2020-01-21 Thread Alexandr Nedvedicky
Hello, below is a diff, which makes pfsync(4) more pf_lock friendly. The diff is sitting in my tree for some time. Hrvoje did some preliminary testing already. He could trigger some sparks and smoke, which I could put off. However don't get too much excited about the change yet. At this phase I

Re: Unbreak X:Y user/group spec in pf.conf

2020-01-16 Thread Alexandr Nedvedicky
Hello, > > +.Pp > > +Note that users 1000 and 1500 are excluded from the pass rule. > > The last line above is a little hard to parse - I think a "positive > example" would be clearer, i.e. something like this: > > .Pp > The example below permits users with uid between 1000 and 1500 > to open

Re: Unbreak X:Y user/group spec in pf.conf

2020-01-16 Thread Alexandr Nedvedicky
Hello, > > +of uids, which match the pass rule. The > New sentences on its own line. I'd say > > Note that users 1000 and 1500 are excluded from the pass rule. > yes, new sentence on the new line. and your wording sounds better. > > +.Cm : > The port paragraph marks up those

Re: Unbreak X:Y user/group spec in pf.conf

2020-01-16 Thread Alexandr Nedvedicky
Hello, > > (2Theo: yes, I'm lazy, sorry :) ) > > I agree, that "X:Y" syntax for "user" could be confusing, and "X> simply ugly. I do not have a silver bullet here, though. > > If you oppose the proposed change, I'll add "... except 'uid1:uid2' syntax, > which could be mistakenly interpreted as

Re: Unbreak X:Y user/group spec in pf.conf

2020-01-16 Thread Alexandr Nedvedicky
Hello, > > > Looks like Vadim found a bug and I'll take a look at the patch > > he has sent. > Where do you see a bug? > at description of 'user' match the pf.conf(5) reads as follows: User and group IDs can be specified as either numbers or names. The syntax is

Re: Unbreak X:Y user/group spec in pf.conf

2020-01-16 Thread Alexandr Nedvedicky
Hello, just to clarify the user and group match in pf.conf On Wed, Jan 15, 2020 at 11:14:43PM -0700, Theo de Raadt wrote: > I'll bite, using text from your regress. > > > +pass out proto tcp all user 1234:12345 flags S/SA > > +pass out proto tcp all user 0:12345 flags S/SA > > +pass out proto

Re: pfctl: Fail on missing anchor

2020-01-15 Thread Alexandr Nedvedicky
Hello, On Thu, Jan 16, 2020 at 12:15:39AM +0100, Klemens Nanni wrote: > There is no reason to continue on anchor specific paths if the given > anchor does not exist, so fix the last occurences better error messages: > > # pfctl -a regress\* -Fa > Anchor 'regress' not found. >

Re: pfctl: unify error message for nonexisting anchors

2020-01-15 Thread Alexandr Nedvedicky
Hello, > There are other occasions as well but those probably need additional > tweaks, so here's the first round. > > Feedback? OK? I like the idea. Just have one 'bike shedding' suggestion: rename pfr_strerror() to pf_strerror() and move it from pfctl_radix.c to pfctl.c

Re: pfctl: Refine error message

2020-01-15 Thread Alexandr Nedvedicky
On Wed, Jan 15, 2020 at 04:44:55PM +0100, Klemens Nanni wrote: > While code in pf/pfctl confusingly uses either anchor or ruleset > depending on the context, pfctl(8) (both manual and user interface) > should be consistent. There should be no difference between anchor and > ruleset for users,

Re: pfctl: Merge radix_perror() into simpler warnx()/errx() usage

2020-01-15 Thread Alexandr Nedvedicky
On Wed, Jan 15, 2020 at 04:26:20PM +0100, Klemens Nanni wrote: > Less nesting for clearer code. > > OK? > > The macro backslashes got adjusted but diff with `-w' for ease of review. > OK sashan

Re: pfctl - allow recursive flush operations [ 5/5 ]

2020-01-14 Thread Alexandr Nedvedicky
Hello > > > > > > This reads simpler and clearer to me, what do you think? > > > > OK, I'll buy if (...) from you, but '+ 1' must stay there, > > because it is for a '\0' terminator. So let's go for this: > > len = strlen(pr->name) + 1; > > if (pr->path[0]) > > len

Re: pfctl - allow recursive flush operations [ 5/5 ]

2020-01-14 Thread Alexandr Nedvedicky
Hello, On Tue, Jan 14, 2020 at 03:02:09PM +0100, Klemens Nanni wrote: > On Mon, Jan 13, 2020 at 10:47:27PM +0100, Alexandr Nedvedicky wrote: > > @@ -2182,7 +2182,7 @@ pfctl_walk_get(int opts, struct pfioc_ruleset *pr, > > void *warg) > > if (pfra == NULL) >

Re: pfctl - allow recursive flush operations [ 5/5 ]

2020-01-14 Thread Alexandr Nedvedicky
Hello, > > + unsigned int len; > strlen(3) returns size_t, malloc(3) takes it. makes sense> > > > + struct pfr_anchors *anchors; > > + > > + anchors = (struct pfr_anchors *) warg; > > + > > + pfra = malloc(sizeof(*pfra)); > > + if (pfra == NULL) > > + err(1, "%s",

tripping assert in pf_state_key_link_reverse()

2020-01-14 Thread Alexandr Nedvedicky
Hello, Some time ago I've tripped over ASSERT() in pf_state_key_link_reverse(), when testing my changes to pfsync. I was using HP 710 systems with 8 cores I got from Hrvoje. The panic happened rarely when running performance tesst over 10Gb interfaces. At time of panic the firewall was running

Re: pfctl - allow recursive flush operations [ 3/5 ]

2020-01-13 Thread Alexandr Nedvedicky
Hello, > > @@ -2109,7 +2112,20 @@ pfctl_debug(int dev, u_int32_t level, int opts) > > } > > > > int > > -pfctl_show_anchors(int dev, int opts, char *anchorname) > > +pfctl_walk_show(int opts, struct pfioc_ruleset *pr, void *warg) > > +{ > > + if (pr->path[0]) { > > + if

Re: pfctl - allow recursive flush operations [ 1/5 ]

2020-01-13 Thread Alexandr Nedvedicky
Hello, this is the last piece, which makes everything fit together. The change introduces three 'nuke()' functions: - pfctl_call_clearrules() - pfctl_call_cleartables() - pfctl_call_clearanchors() Those are callbacks for pfctl_recurse() we've introduced earlier. Those functions just

Re: pfctl - allow recursive flush operations [ 2/5 ]

2020-01-13 Thread Alexandr Nedvedicky
Hello, change introduces a pfctl_recurse() function we use to implement recursive flush operation. The pfctl_recurse() is built from two pieces: - pfctl_walk_get(), which itself is a new callback for pfctl_walk() we introduced in earlier patch. The pfctl_walk_get() puts every

Re: pfctl - allow recursive flush operations [ 3/5 ]

2020-01-13 Thread Alexandr Nedvedicky
Hello, change below splits 'pfctl_show_anchors()' to two functions: - pfctl_walk_anchors() function, which can traverse all anchor tree loaded to PF driver and - pfctl_walk_show(), which is a call back for pfctl_walk_anchors() it just prints anchor name this change is a

Re: pfctl - allow recursive flush operations [ 4/5 ]

2020-01-13 Thread Alexandr Nedvedicky
Hello, pfctl diff bellow enables me to relax err(3) and errx(3) to warnings where needed. The idea is to introduce PF_OPT_IGNFAIL and pfctl_err() and pfctl_errx() wrappers around err(3) and errx(3) functions. the wrappers, when acting in warning mode (PF_OPT_IGNFAIL set), would also set exit_val

  1   2   3   4   5   >