Re: git: c3fbd9c6212c - main - Revert "stand: Remove double words in source code comments"

2023-04-18 Thread Matteo Riondato

On 2023-04-18 at 02:09 EDT, Gordon Bergling  wrote:


The branch main has been updated by gbe:

URL: 
https://cgit.FreeBSD.org/src/commit/?id=c3fbd9c6212cad2634f5958ffec0370166ec7204

commit c3fbd9c6212cad2634f5958ffec0370166ec7204
Author: Gordon Bergling 
AuthorDate: 2023-04-18 06:08:35 +
Commit: Gordon Bergling 
CommitDate: 2023-04-18 06:08:35 +

   Revert "stand: Remove double words in source code comments"

   The sentence, "The base address that we the boot0 code to to run it."
   is correct.

   Reported by:jrtc27

   This reverts commit b12ccd0bb1b37f32e972bb3e945e4025fe409e2f.
---
stand/i386/boot0/Makefile | 2 +-
stand/libsa/zfs/zfsimpl.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/stand/i386/boot0/Makefile b/stand/i386/boot0/Makefile
index dad078fd6f71..1453f17751b9 100644
--- a/stand/i386/boot0/Makefile
+++ b/stand/i386/boot0/Makefile
@@ -36,7 +36,7 @@ BOOT_BOOT0_FLAGS?=0x8f
# 0xb6 (182d) corresponds to 10 seconds.
BOOT_BOOT0_TICKS?=  0xb6

-# The base address that we the boot0 code to run it.  Don't change this
+# The base address that we the boot0 code to to run it.  Don't change this


I really think this comment needs a verb (load), as observed by jrtc27@.

Even better (?), as danfe@ suggested, it should possibly be rephrased 
as:


"The base address where we load the boot0 code to run it. Don't change 
this."


or something along those lines.

Thanks,
Matteo


signature.asc
Description: PGP signature


Re: git: 6a70a0c8bfa6 - main - Document implicit dependencies of the mlx5(4) & friends.

2022-08-12 Thread Matteo Riondato

On 2022-08-11 at 19:33 EDT, Maxim Sobolev  wrote:


The branch main has been updated by sobomax:

URL: 
https://cgit.FreeBSD.org/src/commit/?id=6a70a0c8bfa6fe8a2739527e5c822aec985e83e9

commit 6a70a0c8bfa6fe8a2739527e5c822aec985e83e9
Author: Maxim Sobolev 
AuthorDate: 2022-08-11 23:28:27 +
Commit: Maxim Sobolev 
CommitDate: 2022-08-11 23:33:09 +

   Document implicit dependencies of the mlx5(4) & friends.

   MFC after:  2 weeks
---
sys/amd64/conf/GENERIC | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/sys/amd64/conf/GENERIC b/sys/amd64/conf/GENERIC
index 53c6043a0146..9178abba36cc 100644
--- a/sys/amd64/conf/GENERIC
+++ b/sys/amd64/conf/GENERIC
@@ -260,7 +260,8 @@ device		le			# AMD Am7900 LANCE and 
Am79C9xx PCnet

device  ti  # Alteon Networks Tigon I/II gigabit 
Ethernet

# Nvidia/Mellanox Connect-X 4 and later, Ethernet only
-# mlx5ib requires ibcore infra and is not included by default
+#  o requires COMPAT_LINUXKPI and xz(4)
+#  o mlx5ib requires ibcore infra and is not included by default


Hi Maxim,

May I suggest editing the above line to "mlx5ib requires ibcore infra, 
thus it is not included here" (replacing "and" with "thus" being the 
important part).


The mlx5en(4) page, in SYNOPSIS, lists also option RATELIMIT, option 
KERN_TLS, and device firmware. Are these needed? I imagine that "device 
firmware" certainly is (thus should perhaps be listed in the comment 
above), but what about the options?



device  mlx5# Base driver
device  mlxfw   # Firmware update
device  mlx5en  # Ethernet driver


Thanks,
Matteo


signature.asc
Description: PGP signature


Re: git: cfa1a1308709 - main - pfctl: fix recrusive printing of ethernet anchors

2022-10-17 Thread Matteo Riondato

On 2022-10-07 at 06:13 EDT, Kristof Provost  wrote:

On 3 Oct 2022, at 18:13, Bryan Drewery wrote: 

I think there's still a problem here.

pfctl -a '*' -sr works 
pfctl -a 'name/*' -sr does not.


So I’ve looked at this a bit more, and I am now going to back away 
from the whole anchor thing, and try to pretend I didn’t see any of 
the tentacled horrors that lurk within.


To give you an idea of the issues, loading the following ruleset:

anchor "foo" {
anchor "bar" {
pass in
}
}

does exactly what you’d expect:

# pfctl -sr -a "*"
anchor "foo" all {
  anchor "bar" all {
pass in all flags S/SA keep state
  }
}
# pfctl -sr -a "foo/*"
anchor "bar" all {
  pass in all flags S/SA keep state
}

However, if we `pfctl -Fr` to flush all rules:

# pfctl -Fr
rules cleared
# pfctl -sr -a "*"
# pfctl -sr -a "foo/*"
anchor "bar" all {
  pass in all flags S/SA keep state
}



How is one supposed to know which rules are really loaded in this case?

Printing of rules with anchors being broken (I even get a segmentation 
fault with 'pfctl -a "*" -sr -vv') makes debugging rulesets very hard.


Partially, the question I also have is: is printing of rules broken, or 
is flushing of rules broken, or a third thing? =)



Unloading pf to actually delete the bar anchor, and then we set:

anchor “foo”

And then

# echo "pass" | pfctl -g -f - -a "foo/bar"
# pfctl -sr -a "*"
anchor "foo" all {
}
# pfctl -sr -a "foo/*"
# pfctl -sr -a "foo/bar"
pass all flags S/SA keep state

There are a lot of issues there, and it’ll take a lot of time and 
effort to root them out. My plan is to drink heavily and attempt to 
forget.


Kristof 

Thanks,
Matteo


signature.asc
Description: PGP signature


Re: git: cfa1a1308709 - main - pfctl: fix recrusive printing of ethernet anchors

2022-10-18 Thread Matteo Riondato

On 2022-10-18 at 04:44 EDT, Kristof Provost  wrote:


On 17 Oct 2022, at 19:37, Matteo Riondato wrote:

On 2022-10-07 at 06:13 EDT, Kristof Provost  wrote:

On 3 Oct 2022, at 18:13, Bryan Drewery wrote:

I think there's still a problem here.

pfctl -a '*' -sr works pfctl -a 'name/*' -sr does not.

So I’ve looked at this a bit more, and I am now going to back away 
from the whole anchor thing, and try to pretend I didn’t see any  of 
the tentacled horrors that lurk within.


To give you an idea of the issues, loading the following ruleset:

anchor "foo" {
anchor "bar" {
pass in
}
}

does exactly what you’d expect:

# pfctl -sr -a "*"
anchor "foo" all {
  anchor "bar" all {
pass in all flags S/SA keep state
  }
}
# pfctl -sr -a "foo/*"
anchor "bar" all {
  pass in all flags S/SA keep state
}

However, if we `pfctl -Fr` to flush all rules:

# pfctl -Fr
rules cleared
# pfctl -sr -a "*"
# pfctl -sr -a "foo/*"
anchor "bar" all {
  pass in all flags S/SA keep state
}



How is one supposed to know which rules are really loaded in this 
case?


Printing of rules with anchors being broken (I even get a segmentation 
fault with 'pfctl -a "*" -sr -vv') makes debugging rulesets very hard.


`pfctl -a "*" -sr` should always produce the expected results, at least 
as far as I know.


The last example above clearly shows that `pfctl -a "*"` does *not* 
always produce the expected results: after flushing the rules in the 
root anchor with `pfctl -Fr`, `pfctl -sr -a '*'` does *not* produce the 
expected results, which would have been


anchor "foo" all {
  anchor "bar" all {
pass in all flags S/SA keep state
}
}

I'm wondering what gets printed if you load a ruleset such as:

block all
anchor "foo" all {
  pass in all proto udp
  anchor "bar" all {
pass in all proto tcp flags S/SA keep state
}
}

Then print it with `pfctl -sr -a '*'`, then do a `pfctl -Fr`, and then 
do another `pfctl -sr -a '*'`, followed by a `pfctl -sr -a "foo/*"`. I 
expect to see, again nothing for the `pfctl -sr -a '*'` after the flush, 
and something like the following for the `pfctl -sr -a "foo/*"`:


anchor "foo" all {
  pass in all proto udp
  anchor "bar" all {
pass in all proto tcp flags S/SA keep state
}
}
which at least would (partially?) confirm that `pfctl -Fr` only flushes 
rules in the root anchor.


I’d be very interested in seeing a test case where that core dumps, 
because that is indeed very annoying, and might be something I can fix.


I'll try to come up with a minimally reproducible case (it is totally 
reproducible with my settings, but I'd like to give you something 
smaller).


Partially, the question I also have is: is printing of rules broken, 
or is flushing of rules broken, or a third thing? =)


To the extent that I currently understand this problem I believe the 
issue is that we’re not always stepping into child anchors to print 
them. I believe they do get evaluated when the rules are processed. So 
it’s the printing that’s broken. The flushing is .. not broken, but may 
not do what you’d expect. When  we flush we only flush the root anchor, 
and other anchors can remain.  I think that’s the main source of the 
strange behaviour I’ve  described.


Assuming that indeed `pfctl -Fr` really only acts on the root anchor, 
what makes me worried is whether rules inside child anchors are really 
evaluated *after* a flush of those in the root anchor with `pfctl -Fr`, 
but checking if they really are evaluated should be an easy test.


Thanks,
Matteo


signature.asc
Description: PGP signature


Re: git: a67b925ff3e5 - main - mail: make The Dragonfly Mail Agent (dma) the default mta.

2022-11-08 Thread Matteo Riondato

On 2022-11-07 at 06:50 EST, Baptiste Daroussin  wrote:

diff --git a/libexec/rc/rc.conf b/libexec/rc/rc.conf
index a71a3fa4063d..91b99780eae6 100644
--- a/libexec/rc/rc.conf
+++ b/libexec/rc/rc.conf
@@ -596,7 +596,7 @@ allscreens_kbdflags=""# Set this kbdcontrol mode for 
all virtual screens
mta_start_script="/etc/rc.sendmail"
# Script to start your chosen MTA, called by /etc/rc.
# Settings for /etc/rc.sendmail and /etc/rc.d/sendmail:
-sendmail_enable="NO" # Run the sendmail inbound daemon (YES/NO).
+sendmail_enable="NONE"   # Run the sendmail inbound daemon (YES/NO).


The comment on the above line should probably be updated: inexperienced 
users may not know that there is a difference between "NO" and "NONE".



sendmail_pidfile="/var/run/sendmail.pid"  # sendmail pid file
sendmail_procname="/usr/sbin/sendmail"# sendmail process name
sendmail_flags="-L sm-mta -bd -q30m" # Flags to sendmail (as a server)


Thanks,
Matteo


signature.asc
Description: PGP signature


Re: git: b7104f19147f - main - sendmail: document that sendmail_enable can be set to NONE

2022-11-08 Thread Matteo Riondato

On 2022-11-08 at 12:11 EST, Mark Millard  wrote:
Baptiste Daroussin  wrote on Date: Tue, 08 Nov 
2022 13:41:29 UTC :



On Tue, Nov 08, 2022 at 08:07:46AM -0500, Matteo Riondato wrote:
> On 2022-11-07 at 06:50 EST, Baptiste Daroussin  
> wrote:

> > diff --git a/libexec/rc/rc.conf b/libexec/rc/rc.conf
> > index a71a3fa4063d..91b99780eae6 100644
> > --- a/libexec/rc/rc.conf
> > +++ b/libexec/rc/rc.conf
> > @@ -596,7 +596,7 @@ allscreens_kbdflags="" # Set this kbdcontrol 
> > mode for all virtual screens

> > mta_start_script="/etc/rc.sendmail"
> > # Script to start your chosen MTA, called by /etc/rc.
> > # Settings for /etc/rc.sendmail and /etc/rc.d/sendmail:
> > -sendmail_enable="NO" # Run the sendmail inbound daemon (YES/NO).
> > +sendmail_enable="NONE" # Run the sendmail inbound daemon 
> > (YES/NO).

>
> The comment on the above line should probably be updated: 
> inexperienced
> users may not know that there is a difference between "NO" and 
> "NONE".

>
> > sendmail_pidfile="/var/run/sendmail.pid" # sendmail pid file
> > sendmail_procname="/usr/sbin/sendmail" # sendmail process name
> > sendmail_flags="-L sm-mta -bd -q30m" # Flags to sendmail (as a 
> > server)


You are not the first to report that, I have updated rc.conf a to add 
"the NONE" value.


Also, from the commit:

QUOTE
-sendmail_enable="NONE" # Run the sendmail inbound daemon (YES/NO).
+sendmail_enable="NONE" # Run the sendmail inbound daemon 
(YES/NO/NONE).

END QUOTE

My guess is Matteo was after something more like the following text 
from your commit notes:


QUOTE
setting sendmail_enable to NONE (which is now the default) turns all 
the sendmail_*_enable variables to NO

END QUOTE

In other words, documenting what is different about NONE vs. NO 
beyond the spelling distinction.


Indeed the issue that I intended to raise was that, AFAIK, there is no 
documentation, for NONE, except if one reads and understands 
/etc/rc.d/sendmail. It is something that is known by people "in the 
know", and probably mentioned by a some online tutorials, but I don't 
think there is anything "official".


Even the handbook, in the section about changing MTA
(https://docs.freebsd.org/en/books/handbook/mail/#mail-changingmta) does 
not mention NONE, but explicitly sets the four variables to NO.


(Side note: it seems that when this change will be integrated in some 
-STABLE branch, or when 14 becomes -STABLE, a large update to the 
relevant sections of the Handbook will be needed)


Thanks,
Matteo


signature.asc
Description: PGP signature


Re: git: b7104f19147f - main - sendmail: document that sendmail_enable can be set to NONE

2022-11-08 Thread Matteo Riondato

On 2022-11-08 at 14:17 EST, Baptiste Daroussin  wrote:


Indeed the issue that I intended to raise was that, AFAIK, there is no 
documentation, for NONE, except if one reads and understands 
/etc/rc.d/sendmail. It is something that is known by people "in the 
know", and probably mentioned by a some online tutorials, but I don't 
think there is anything "official".


Even the handbook, in the section about changing MTA 
(https://docs.freebsd.org/en/books/handbook/mail/#mail-changingmta) 
does not mention NONE, but explicitly sets the four variables to NO.



It is documented as deprecated in rc.sendmail(8)


Sorry for missing this: this man page is not linked from sendmail(8), 
nor from rc(8), nor mentioned in the script that it describes, although 
it is linked from rc.conf(5) (and from no other manpage, as far as I can 
tell).


If NONE is deprecated, then perhaps it would have been better to set the 
four variables to NO in the modifcation.


Also, I wonder whether the value of "mta_start_script" in rc.conf should 
be changed to reflect that the default MTA is no longer sendmail, and 
also what the role of /etc/rc.d/othermta should be (and whether it 
should be updated as well).


Thanks,
Matteo


signature.asc
Description: PGP signature


Re: git: 0b1adc42a15c - main - rc.sendmail: remove unused script

2022-11-09 Thread Matteo Riondato

On 2022-11-09 at 10:57 EST, Baptiste Daroussin  wrote:


The branch main has been updated by bapt:

URL: 
https://cgit.FreeBSD.org/src/commit/?id=0b1adc42a15caea0cffbc962ca6f9e3e7b576834

commit 0b1adc42a15caea0cffbc962ca6f9e3e7b576834 
Author: Baptiste Daroussin  
AuthorDate: 2022-11-09 15:33:03 + 
Commit: Baptiste Daroussin  
CommitDate: 2022-11-09 15:55:18 +


   rc.sendmail: remove unused script

   20 years ago the use of rc.sendmail script was dropped in favor 
   of /etc/rc.d/sendmail, it is time to retire the script entirely 
   now.


Thank you for doing this.

The one lingering issue I see is that there is now no documentation for 
the rc.conf sendmail_* variables: they were documented in rc.sendmail.8 
and rc.conf.5 referred to it, but your previous commit 
6a2d6a569b1350f46998a2446fc89059b160fd13 removed that reference from 
rc.conf.8, and this commit removes rc.sendmail.8.


(Sorry for the noise if you're already working on it).

Thanks,
Matteo


signature.asc
Description: PGP signature


Re: git: 0b1adc42a15c - main - rc.sendmail: remove unused script

2022-11-10 Thread Matteo Riondato

On 2022-11-09 at 14:29 EST, Baptiste Daroussin  wrote:
Le 9 novembre 2022 18:49:47 GMT+01:00, Matteo Riondato 
 a écrit :
The one lingering issue I see is that there is now no documentation 
for the rc.conf sendmail_* variables: they were documented in 
rc.sendmail.8 and rc.conf.5 referred to it, but your previous commit 
6a2d6a569b1350f46998a2446fc89059b160fd13 removed that reference from 
rc.conf.8, and this commit removes rc.sendmail.8.


(Sorry for the noise if you're already working on it).


I prefer too much noise than something forgotten in silence and 
actually this is no noise


Such documentation is supposed to be in  rc.conf(5). It would probably 
be a good idea to push the documentation there.


If someone is interested be my guest, otherwise I will look into it in 
the next couple of weeks.


Thank you for taking care of this one so quickly.

A few rough corners I still see:

1) sendmail_cert_create should perhaps be set to NO in rc.conf, or the 
call to sendmail_cert_create() in rc.d/sendmail should also depend on 
some form of sendmail being enabled. The former action seems easier.


2) the role of rc.d/othermta is quite bizarre, especially now that it 
checks for a non-existing script (/etc/rc.sendmail). I wonder whether it 
should be retired, together with the rc.conf variable mta_start_script 
(which now is set to a non-existing script). It feels like a relic from 
a long gone past.


3) the comments on some of the sendmail_* variables in rc.conf mention a 
generic "MTA", but perhaps they should say "sendmail", as that's not the 
one-and-only MTA.


Thanks,
Matteo


signature.asc
Description: PGP signature


Re: git: 0b1adc42a15c - main - rc.sendmail: remove unused script

2022-11-10 Thread Matteo Riondato

On 2022-11-10 at 09:18 EST, Baptiste Daroussin  wrote:

On Thu, Nov 10, 2022 at 09:01:06AM -0500, Matteo Riondato wrote:


A few rough corners I still see:

1) sendmail_cert_create should perhaps be set to NO in rc.conf, or 
the call to sendmail_cert_create() in rc.d/sendmail should also 
depend on some form of sendmail being enabled. The former action 
seems easier.


Don't know, this has always been like this for a while.


Yeah, I see pros and cons of changing it, so not touching it may be wise 
=)


2) the role of rc.d/othermta is quite bizarre, especially now that it 
checks for a non-existing script (/etc/rc.sendmail). I wonder whether 
it should be retired, together with the rc.conf variable 
mta_start_script (which now is set to a non-existing script). It 
feels like a relic from a long gone past.


https://lists.freebsd.org/archives/freebsd-arch/2022-November/000264.html

It is a relic from the past (unused for 20 years)


Glad we agree. I should probably start following arch@ again. =)



3) the comments on some of the sendmail_* variables in rc.conf 
mention a generic "MTA", but perhaps they should say "sendmail", as 
that's not the one-and-only MTA.



I am open to proposals/patches


(Can't promise cut and paste will work correctly)

diff --git a/libexec/rc/rc.conf b/libexec/rc/rc.conf
index 50fed07df1..b5d908e38b 100644
--- a/libexec/rc/rc.conf
+++ b/libexec/rc/rc.conf
@@ -596,21 +596,21 @@ allscreens_kbdflags=""# Set this kbdcontrol mode for 
all virtual screens
 mta_start_script="/etc/rc.sendmail"
# Script to start your chosen MTA, called by /etc/rc.
 # Settings for /etc/rc.d/sendmail:
-sendmail_enable="NO"   # Run the sendmail inbound daemon (YES/NO/NONE).
+sendmail_enable="NO"   # Start a inbound/outbound sendmail daemon 
(YES/NO/NONE).
# If NONE, don't start any sendmail processes.
 sendmail_pidfile="/var/run/sendmail.pid"   # sendmail pid file
 sendmail_procname="/usr/sbin/sendmail" # sendmail process name
-sendmail_flags="-L sm-mta -bd -q30m" # Flags to sendmail (as a server)
+sendmail_flags="-L sm-mta -bd -q30m" # Flags to sendmail (as inbound/outbound 
server)
 sendmail_cert_create="YES" # Create a server certificate if none (YES/NO)
-#sendmail_cert_cn="CN" # CN of the generate certificate
-sendmail_submit_enable="NO"# Start a localhost-only MTA for mail submission
+#sendmail_cert_cn="CN" # CN of the generated certificate
+sendmail_submit_enable="NO"# Start a localhost-only sendmail for mail 
submission
 sendmail_submit_flags="-L sm-mta -bd -q30m -ODaemonPortOptions=Addr=localhost"
-   # Flags for localhost-only MTA
-sendmail_outbound_enable="NO"  # Dequeue stuck mail (YES/NO).
-sendmail_outbound_flags="-L sm-queue -q30m" # Flags to sendmail (outbound only)
-sendmail_msp_queue_enable="NO" # Dequeue stuck clientmqueue mail (YES/NO).
+   # Flags for localhost-only sendmail
+sendmail_outbound_enable="NO"  # Start an outbound-only sendmail (YES/NO).
+sendmail_outbound_flags="-L sm-queue -q30m" # Flags to outbund-only sendmail
+sendmail_msp_queue_enable="NO" # Start a clientmqueue runner sendmail (YES/NO).
 sendmail_msp_queue_flags="-L sm-msp-queue -Ac -q30m"
-   # Flags for sendmail_msp_queue daemon.
+   # Flags for clientmqueue runner sendmail
 sendmail_rebuild_aliases="NO"  # Run newaliases if necessary (YES/NO).

Thanks,
Matteo


signature.asc
Description: PGP signature


Re: git: f53dc31bb3ef - main - src.opts.mk: Disable all of LLVM if C++ support is disabled.

2022-11-16 Thread Matteo Riondato

On 2022-11-15 at 22:23 EST, John Baldwin  wrote:


The branch main has been updated by jhb:

URL: 
https://cgit.FreeBSD.org/src/commit/?id=f53dc31bb3ef387338a7678581e8c7d587da8d2c

commit f53dc31bb3ef387338a7678581e8c7d587da8d2c
Author: John Baldwin 
AuthorDate: 2022-11-16 03:21:20 + Commit: John Baldwin 


CommitDate: 2022-11-16 03:21:20 +

   src.opts.mk: Disable all of LLVM if C++ support is disabled.


This should probably be documented in src.conf(5).


   Reviewed by:imp, emaste
   Differential Revision:  https://reviews.freebsd.org/D36891
---
share/mk/src.opts.mk | 3 +++
1 file changed, 3 insertions(+)

diff --git a/share/mk/src.opts.mk b/share/mk/src.opts.mk
index 4c0913474ef7..ff8c359acc42 100644
--- a/share/mk/src.opts.mk
+++ b/share/mk/src.opts.mk
@@ -380,6 +380,9 @@ MK_KERBEROS_SUPPORT:=   no

.if ${MK_CXX} == "no"
MK_CLANG:=  no
+MK_LLD:=   no
+MK_LLDB:=  no
+MK_LLVM_BINUTILS:= no
MK_GOOGLETEST:= no
MK_OFED:=   no
MK_OPENMP:= no


Thanks,
Matteo


signature.asc
Description: PGP signature


Re: git: 62a149bf6219 - main - Add new rc: machine_id to generate /etc/machine-id

2023-01-09 Thread Matteo Riondato

On 2022-12-24 at 03:39 EST, Tobias C. Berner  wrote:


Moin moin

I think it tries to solve the same problem of giving the machine a unique id.

From the linux man page [1]

  The machine ID does not change based on local or network 
  configuration or when hardware is replaced. Due to this and its 
  greater length, it is a more useful replacement for the 
  gethostid(3) call that POSIX specifies.




Does this make or should it make hostid obsolete?
If yes, then changes are needed to deorbit hostid. If not, why not?

Thanks,
Matteo


signature.asc
Description: PGP signature


Re: git: 3a1f834b5228 - main - pf: Add code to enable filtering for locally delivered packets

2023-06-20 Thread Matteo Riondato

On 2023-06-20 at 10:35 EDT, Doug Rabson  wrote:


The branch main has been updated by dfr:

URL: 
https://cgit.FreeBSD.org/src/commit/?id=3a1f834b5228986a7c14fd60da13cf2700e80996

commit 3a1f834b5228986a7c14fd60da13cf2700e80996
Author: Doug Rabson 
AuthorDate: 2023-06-20 13:01:58 +
Commit: Doug Rabson 
CommitDate: 2023-06-20 14:34:01 +

   pf: Add code to enable filtering for locally delivered packets

   This is disabled by default since it potentially changes the behavior of
   existing filter rule sets. To enable this extra filter for packets being
   delivered locally, use:

   sysctl net.pf.filter_local=1
   service pf restart

   PR: 268717
   Reviewed-by:kp
   MFC-after:  2 weeks
   Differential Revision: https://reviews.freebsd.org/D40373
---
UPDATING | 12 
sys/netpfil/pf/pf_ioctl.c| 20 
tests/sys/netpfil/common/utils.subr  |  3 +--
tests/sys/netpfil/pf/fragmentation_compat.sh |  3 ++-
tests/sys/netpfil/pf/fragmentation_pass.sh   |  3 ++-
tests/sys/netpfil/pf/killstate.sh| 24 
tests/sys/netpfil/pf/map_e.sh|  3 ++-
tests/sys/netpfil/pf/pass_block.sh   |  3 ++-
tests/sys/netpfil/pf/pfsync.sh   |  1 +
tests/sys/netpfil/pf/route_to.sh |  3 ++-
tests/sys/netpfil/pf/set_skip.sh |  2 +-
tests/sys/netpfil/pf/table.sh|  6 --
12 files changed, 65 insertions(+), 18 deletions(-)

diff --git a/UPDATING b/UPDATING
index 1980411c1853..f4e13d97006d 100644
--- a/UPDATING
+++ b/UPDATING
@@ -27,6 +27,18 @@ NOTE TO PEOPLE WHO THINK THAT FreeBSD 14.x IS SLOW:
world, or to merely disable the most expensive debugging functionality
at runtime, run "ln -s 'abort:false,junk:false' /etc/malloc.conf".)

+20230619:
+   To enable pf rdr rules for connections initiated from the host, pf
+   filter rules can be optionally enabled for packets delivered
+   locally. This can change the behavior of rules which match packets
+   delivered to lo0. To enable this feature:
+
+   sysctl net.pf.filter_local=1
+   service pf restart


It seems a bit weird to suggest an action that is not permanent (does 
not survive reboot). See proposed rewording below.



+
+   When enabled, its best to ensure that packets delivered locally are not


s/its/it is/


+   filtered, e.g. by adding a 'skip on lo' rule.


TBH, I find the phrasing a bit confusing: "to enable pf rdr rules for 
connections …, pf filter rules can *optionally* be  enabled for packets 
delivered locally". That "optionally" makes it sound as if it is not 
*required* to enable pf filter rules for packets delivered locally in 
order to enable pf rdr rules for connections etc etc., but, given this 
change, I assume it is.


Perhaps a better phrasing (assuming I understand the feature) would be:

"The new sysctl net.pf.filter_local controls whether PF filter rules are 
enabled for packets originating from localhost and delivered locally.


This feature can be useful for, e.g., enabling rdr rules for connections 
initiated from localhost and redirected to a different port on 
localhost. Setting the sysctl to 1 may change the behavior of rules 
which match packets delivered to lo0, so it may be necessary to add 
enable the "skip on lo" option."


Note that "skip on" is not a rule, even if it is translated to a pair of 
rules: it's part of the options, and requires "set" before it, per 
pf.conf(5). Also, I'm assuming (and mention in the rewording) we are 
talking about rdr rules for port remapping, not rdr rules that redirect 
to other destinations, but please confirm or adjust.


More generally, this new feature should likely also be documented 
somewhere else (pf(4) ? pfctl(8)? pf.conf(5)?). 

But apart from the above, I'm a little puzzled: does it mean that until 
now (and continuing to do so, unless one sets the sysctl to 1), packets 
originating locally and destined locally were not filtered by pf? I.e., 
that filtering rules on lo0 had no effect on incoming traffic from 
localhost?


Thanks,
Matteo


signature.asc
Description: PGP signature


Re: git: 3a1f834b5228 - main - pf: Add code to enable filtering for locally delivered packets

2023-07-18 Thread Matteo Riondato

On 2023-06-20 at 11:57 EDT, Matteo Riondato  wrote:


On 2023-06-20 at 10:35 EDT, Doug Rabson  wrote:


The branch main has been updated by dfr:

URL: 
https://cgit.FreeBSD.org/src/commit/?id=3a1f834b5228986a7c14fd60da13cf2700e80996

commit 3a1f834b5228986a7c14fd60da13cf2700e80996
Author: Doug Rabson 
AuthorDate: 2023-06-20 13:01:58 +
Commit: Doug Rabson 
CommitDate: 2023-06-20 14:34:01 +

  pf: Add code to enable filtering for locally delivered packets

  This is disabled by default since it potentially changes the behavior of 
  existing filter rule sets. To enable this extra filter for packets being 
  delivered locally, use:


  sysctl net.pf.filter_local=1
  service pf restart

  PR: 268717
  Reviewed-by:kp
  MFC-after:  2 weeks
  Differential Revision: https://reviews.freebsd.org/D40373
---
UPDATING | 12 
sys/netpfil/pf/pf_ioctl.c| 20 
tests/sys/netpfil/common/utils.subr  |  3 +--
tests/sys/netpfil/pf/fragmentation_compat.sh |  3 ++-
tests/sys/netpfil/pf/fragmentation_pass.sh   |  3 ++-
tests/sys/netpfil/pf/killstate.sh| 24 


tests/sys/netpfil/pf/map_e.sh|  3 ++-
tests/sys/netpfil/pf/pass_block.sh   |  3 ++-
tests/sys/netpfil/pf/pfsync.sh   |  1 +
tests/sys/netpfil/pf/route_to.sh |  3 ++-
tests/sys/netpfil/pf/set_skip.sh |  2 +-
tests/sys/netpfil/pf/table.sh|  6 --
12 files changed, 65 insertions(+), 18 deletions(-)

diff --git a/UPDATING b/UPDATING
index 1980411c1853..f4e13d97006d 100644
--- a/UPDATING
+++ b/UPDATING
@@ -27,6 +27,18 @@ NOTE TO PEOPLE WHO THINK THAT FreeBSD 14.x IS SLOW:
	world, or to merely disable the most expensive debugging 
	functionality

at runtime, run "ln -s 'abort:false,junk:false' /etc/malloc.conf".)

+20230619:
+   To enable pf rdr rules for connections initiated from the host, pf
+   filter rules can be optionally enabled for packets delivered
+   locally. This can change the behavior of rules which match packets
+   delivered to lo0. To enable this feature:
+
+   sysctl net.pf.filter_local=1
+   service pf restart


It seems a bit weird to suggest an action that is not permanent (does 
not survive reboot). See proposed rewording below.



+
+   When enabled, its best to ensure that packets delivered locally are not


s/its/it is/


+   filtered, e.g. by adding a 'skip on lo' rule.


TBH, I find the phrasing a bit confusing: "to enable pf rdr rules for 
connections …, pf filter rules can *optionally* be  enabled for 
packets delivered locally". That "optionally" makes it sound as if it 
is not *required* to enable pf filter rules for packets delivered 
locally in order to enable pf rdr rules for connections etc etc., but, 
given this change, I assume it is.


Perhaps a better phrasing (assuming I understand the feature) would be:

"The new sysctl net.pf.filter_local controls whether PF filter rules 
are enabled for packets originating from localhost and delivered 
locally.


This feature can be useful for, e.g., enabling rdr rules for 
connections initiated from localhost and redirected to a different 
port on localhost. Setting the sysctl to 1 may change the behavior of 
rules which match packets delivered to lo0, so it may be necessary to 
add enable the "skip on lo" option."


Note that "skip on" is not a rule, even if it is translated to a pair 
of rules: it's part of the options, and requires "set" before it, per 
pf.conf(5). Also, I'm assuming (and mention in the rewording) we are 
talking about rdr rules for port remapping, not rdr rules that 
redirect to other destinations, but please confirm or adjust.


More generally, this new feature should likely also be documented 
somewhere else (pf(4) ? pfctl(8)? pf.conf(5)?).


But apart from the above, I'm a little puzzled: does it mean that 
until now (and continuing to do so, unless one sets the sysctl to 1), 
packets originating locally and destined locally were not filtered by 
pf? I.e., that filtering rules on lo0 had no effect on incoming 
traffic from localhost?


Hi Doug and Kristof,

A ping about what I said below, as I think there is a need to better 
document this sysctl.


Additionally, I'm also a little worried about the name of the sysctl, 
which seems extremely generic, while its use may be much more specific. 
If the name could be made more specific, that should probably be done 
before 14 is branched.


Thanks,
Matteo


signature.asc
Description: PGP signature


Re: git: f8a8bb7bade9 - main - pfctl: Improve duplicate table name warning

2025-07-07 Thread Matteo Riondato


> On Jul 7, 2025, at 11:07 AM, Kristof Provost  wrote:
> 
> The branch main has been updated by kp:
> 
> URL: 
> https://cgit.FreeBSD.org/src/commit/?id=f8a8bb7bade9575c549cbc94500ba706b712c650
> 
> commit f8a8bb7bade9575c549cbc94500ba706b712c650
> Author: Kristof Provost 
> AuthorDate: 2025-07-01 10:02:03 +
> Commit: Kristof Provost 
> CommitDate: 2025-07-07 15:06:48 +
> 
>pfctl: Improve duplicate table name warning
> 
>When creating tables inside anchors, pfctl warned about namespace
>collisions with global tables, but only in certain cases and with
>limited information sometimes leaving users clueless.
> 
>Deferring the check to process_tabledefs() where tables are eventually
>created, both anchor and table name are known which allows for checking
>all existing anchors.
> 
>With this, warn on all duplicates even in dry-runs (`-n') and print
>quoted names so they can be copied to fix configurations right away.
> 
>No functional change in parsing or ruleset production.

Based only on the commit message, this may fix PR #262295.

Thanks,
Matteo