Re: NPF config data limit

[Taylor R Campbell <riastradh%NetBSD.org@localhost>]

> Date: Mon, 22 Jun 2026 04:56:31 -0400
> From: Emmanuel Nyarko <emmankoko519%gmail.com@localhost>
> 
> @@ -53,6 +53,9 @@
>  #include <sys/pserialize.h>
>  #include <sys/socketvar.h>
>  #include <sys/uio.h>
> +#include <sys/sysctl.h>
> +#include <sys/syslog.h>
> +#include <sys/systm.h>

Sort includes lexicographically.  These should go between
sys/socketvar.h and sys/uio.h.

> +uint64_t npf_ioc_data_limit = NPF_IOCTL_DATA_LIMIT;

Where is this used?  Is part of the patch missing?

Also: Are there new test cases?  Test cases should:

1. verify loading a medium (>4MB, <16MB) set of rules fails
2. verify loading a large (>>16MB) set of rules fails
3. verify increasing the limit to 16MB enables loading medium
4. verify increasing the limit to 16MB does not enable loading large
5. verify increasing the limit past the physical RAM available fails
6. if npfctl can load new things at securelevel>0 or securelevel>1:
   verify the limit can't be increased at securelevel>1 (or maybe even
   securelevel>0), if npf

> +extern psize_t physmem;

Don't put `extern' declarations in .c files; include the correct .h
file.  Looks like sys/systm.h was already enough for this, so just
delete it.

> +static void sysctl_net_npf_limit_setup(struct sysctllog **);

No need to forward-declare this, because you're defining it before its
use.  But if you do want to forward-declare it, use SYSCTLFN_ARGS in
the prototype just like in the definition, instead of writing out the
type `struct sysctllog **' -- in case we ever change SYSCTLFN_ARGS.

> +static int
> +sysctl_npf_get_limit(SYSCTLFN_ARGS)
> +{
> +	struct sysctlnode node;
> +	uint64_t new_size;
> +	int error;
> +
> +	node = *rnode;
> +	node.sysctl_data = &new_size;
> +	new_size = npf_ioc_data_limit;
> +
> +	error = sysctl_lookup(SYSCTLFN_CALL(&node));
> +	if (error || newp == NULL)
> +		return error;
> +
> +	if (new_size < PAGE_SIZE || new_size > physmem)
> +		return EINVAL;
> +
> +	npf_ioc_data_limit = new_size;

Access to npf_ioc_data_limit needs to be atomic.

I suggest you change the type to volatile size_t, and use
atomic_load/store_relaxed for access to it.

(The more obvious approach of holding a mutex here could cause
trouble, because it is tempting to hold it across the sysctl_lookup
call -- which you must not do because sysctl_lookup may sleep
indefinitely for I/O while userland is paged in and out.)

The same is true for physmem, but that's not a new problem.  So you
should put an XXX comment here to say that physmem might be changed
concurrently by uvm_physseg_plug, and we really need to have some way
to propagate updates to physmem here.  Fortunately, the updates are
(currently) only to increase it, so observing a stale physmem here is
safe.

What units is physmem in?  Is it bytes, pages, or what?  Make sure 

The limit should be a little below physmem: we need some spare space
for other kernel functions.  In fact, physmem may be much larger than
what can actually be copied into the kernel in a temporary buffer.
MIN(physmem, nkmempages)/4 is probably a better approximation, like we
use in nmbclusters_limit (nkmempages is from uvm/uvm_extern.h).

> +SYSCTL_SETUP(sysctl_net_npf_limit_setup, "npf sysctl")

Nit: Call it sysctl_net_npf_setup, since it sets up net.npf.*.
(Currently there's only one sysctl in there but it might grow more
later.)

> +{
> +	int error;
> +	const struct sysctlnode *node;
> +
> +	error = sysctl_createv(clog, 0, NULL, &node,
> +		       CTLFLAG_PERMANENT,
> +		       CTLTYPE_NODE, "npf",
> +		       SYSCTL_DESCR("NPF related settings"),
> +		       NULL, 0, NULL, 0,
> +		       CTL_NET, CTL_CREATE, CTL_EOL);

Nit: four-space additional indent for continuation lines in KNF.

	error = sysctl_createv(clog, 0, NULL, &node,
	    CTLFLAG_PERMANENT,
	    CTLTYPE_NODE, "npf",
	    SYSCTL_DESCR("NPF related settings"),
	    NULL, 0, NULL, 0,
	    CTL_NET, CTL_CREATE, CTL_EOL);

Nit: hyphenate `NPF-related settings', or just say `NPF settings'.

> +	if (error != 0)
> +		goto bad;

Nit: I like to say `if (error) ...' -- conciser and I think it reads
more obviously.  (But it's not specified in KNF; if you prefer to
write `if (error != 0) ...', that's fine too.)

> +	error = sysctl_createv(clog, 0, NULL, NULL,
> +		       CTLFLAG_PERMANENT|CTLFLAG_READWRITE,
> +		       CTLTYPE_QUAD, "datalimit",

Bikeshed alert!  `Data limit' isn't obvious to me.  Maybe
`maxioctlbytes' or something?

> +		       SYSCTL_DESCR("NPF config data limit settings"),

Say "Maximum npf ioctl input size in bytes" or something like that:
say what it limits, and what units it is in.

> +	error = sysctl_createv(clog, 0, NULL, NULL,
> ...
> +			   CTL_NET, node->sysctl_num, CTL_CREATE, CTL_EOL);

Slight simplification: just pass the node, instead of extracting its
number and writing out the fully qualified mib {CTL_NET,
node->sysctl_num, ...}.

	sysctl_createv(clog, 0, &node, NULL,
...
	    CTL_CREATE, CTL_EOL);

(Why you pass &node here instead of node is a mystery to me, but so is
much of the complexity in the sysctl_createv interface!)

> +	log(LOG_ERR, "%s: could not create a sysctl node for net.npf.datalimit\n",
> +	    __func__);

Except for the first sysctl_createv, I suggest you simply ignore the
sysctl_createv return value, like here:

https://nxr.NetBSD.org/xref/src/sys/kern/kern_entropy.c?r=1.74#357

Even for the first one, you don't need to log a message -- it will
already print a message to the console itself if it is failing:

https://nxr.NetBSD.org/xref/src/sys/kern/kern_sysctl.c?r=1.273#2206

So you can just do:

	error = sysctl_createv(clog, 0, NULL, &node,
	    ...,
	    CTL_NET, CTL_CREATE, CTL_EOL);
	if (error)
		return;
	sysctl_createv(clog, 0, &node, NULL,
	    ...,
	    CTL_CREATE, CTL_EOL);
	sysctl_createv(clog, 0, &node, NULL,
	    ...,
	    CTL_CREATE, CTL_EOL);
	sysctl_createv(clog, 0, &node, NULL,
	    ...,
	    CTL_CREATE, CTL_EOL);