Hello,
On Wed, Dec 18, 2019 at 12:24:57AM +0100, Alexander Bluhm wrote:
> On Mon, Dec 16, 2019 at 03:42:27PM +0100, Alexandr Nedvedicky wrote:
> > > I think this is a "do as I want" kind of thing. If I use pf(4) to redirect
> > > traffic to a different address the
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.
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...
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
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
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
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@
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
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
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,
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
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.
>
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
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
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
Hello,
below is a complete change I'd like to commit. Diff below enables pfctl to
recursively flush objects (tables, rules, anchors) from PF. The same change
has been discussed last spring [1]. But the discussion died and all effort
dropped down on the floor. I'd like to restart the effort now.
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
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
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
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
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
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)
>
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
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.
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
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
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
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
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",
Hello,
On Tue, Dec 24, 2019 at 03:27:44PM +0100, Alexander Bluhm wrote:
> Hi,
>
> The loopback check in ip6_input_if() seems needless. The ::1
> destination address is in the routing table and will be identified
> as any other local address. Better use the generic IP input path.
> I see no
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
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
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
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
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
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
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@
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
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
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:
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
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
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
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
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
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@
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
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
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
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
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
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
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
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.
>
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
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
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
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.
> > >
> >
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
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)
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.
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
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
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.
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
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
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
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
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.
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
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
>
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);
>
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
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, ) !=
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
> > > +++
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,
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:
>
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
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
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
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
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.
&
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
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
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
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
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
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
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
>
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.
> > >
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
> ===
>
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
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
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
Hello,
looks good to me. I think this should be committed
as-is. I have just one question,
On Mon, Jun 14, 2021 at 01:58:06PM +1000, David Gwynne wrote:
> @@ -1931,6 +1933,9 @@ pfsync_defer(struct pf_state *st, struct
> {
> struct pfsync_softc *sc = pfsyncif;
> struct
Hello,
> > it seems to me we may leak `pd` we took at line 1944. The leak
> > happens in case timeout_del() return zero.
> >
> > diff below ignores the return value from timeout_del() and makes
> > sure we always call pfsync_undefefer()
> >
> > I have not seen such panic in
Hello,
On Wed, Jun 16, 2021 at 02:19:24PM +1000, David Gwynne wrote:
>
>
> > On 14 Jun 2021, at 19:12, Alexandr Nedvedicky
> > wrote:
> >
> > Hello,
> >
> > looks good to me. I think this should be committed
> > as-is. I have just one quest
Hello,
>
> as above, copyout with a sleeping lock is fine.
>
> the whole point of my change is to give us the ability to lock in the
> forwarding path separately to locking in the ioctl path. half of that is
> so we can copyout safely. the other half is to avoid letting the ioctl
> path block
Hello David,
I'm still not sure if your change is ultimate fix, or just significantly
minimizes risk of the bug. If I understand things right, the problem we are
trying to solve:
DIOCGETSTATES we have in current, grabs NET_LOCK() and pf_state_lock
as a reader.
it then walks through
Hello,
new diff still looks good to me. I could just catch typos
in comment.
>
> I've been running versions of this diff in production at work, and have
> hit a few panics and asserts. All the issues we've hit should be
> addressed in this diff.
>
> The first issue was that pfsync
301 - 400 of 609 matches
Mail list logo
