In D48163#1098579, @zlei wrote:

I thinks that is a general question, that is, does a firewall want to see all known invalid packets? For example, the above logic to check IN_LOOPBACK on the wire.

In D47842#1090897, @jhb wrote:

The thing that makes this a bit odd in terms of the memory limit is that userspace isn't the one requesting to wire the memory. The kernel does by calling sysctl_wire_old_buffer in an individual handler. It would be nice to be smarter about this, e.g. don't acquire the lock until sysctl_wire_old_buffer is actually called? OTOH, one might ask why we really need the lock at all? I guess the worry is that userspace can DOS the kernel by making lots of requests with large buffers for nodes that are known to use sysctl_wire_old_buffer?

In D48163#1098248, @glebius wrote:

On second thought this probably needs to be mentioned in Relnotes and not MFC-ed. Cause it might be that somebody was relying on the stack to drop those packets before firewall.

I'm sorry for the delayed follow-up. I'm trying to understand the purpose of the roundup. It appears to have been introduced by commit daf6545e6158f; the implication seems to be that some drivers depend on the property that "the relative alignment of two consecutive bytes in the I/O stream have a difference of 1 even if they are not physically contiguous."

In D47953#1094057, @vegeta_tuxpowered.net wrote:

Improve the situation a bit: force logging if state creation fails.
This isn't totally right as we'll end up logging the packet twice in
this case, but it's better than not logging the drop at all.

This seems more like a workaround than a fix of underlying design issues. Since I'm responsible for the mess of pf_rule_actions and match rules, I might be able to propose an alternative solution. Can you wait a few days?

In D48150#1098165, @kib wrote:

In D48150#1098161, @markj wrote:

Suppose a filesystem has MNT_NOSUID set, and contains a setuid root executable. If I create a nullfs mount of that filesystem and run the executable though nullfs, should the suid bit be honoured? I would expect not, but it is. In particular, MNT_NOSUID is not copied to the upper mount, and the code which checks MNT_NOSUID does not use a VOP, it just checks vp->v_mount->mnt_flags.

This is the current behavior, before or after the change, and in fact it is not that weird. Imagine that instead of nullfs, the lower fs is exported and mounted by nfs. The result is same.

In D48150#1098159, @kib wrote:

In D48150#1098146, @markj wrote:

When lower fs acts according to its mount configuration, it is reported up as VOP errors.

I do not quite understand what you mean here.

I mean that mnt_flags from lower fs do not need to be propagated to upper fs to make the mount operate correctly. Lower fs flags are checked when bypass calls lower VOP. So there is no need to 'fake' mnt_flags for upper fs, and esp. to fake it for only reporting to user in statfs(2).

When lower fs acts according to its mount configuration, it is reported up as VOP errors.

The current verbiage does need improvement.

If you mail me git format-patch output or submit it on github, I'll apply it.

Set PR_VNET earlier, so that prison_deref() won't leak the VNET if an
error occurs during jail initialization.

My comments are just suggestions/quibbling.

In D47992#1097201, @jamie wrote:

In D47992#1097186, @markj wrote:

The new PR_VNET_ROOT flag is unneeded: it's equivalent to PR_VNET, just check
that instead.

Not quite, I'm afraid. This is true once a prison has been created, but errors during the creation process can break that. pr_vnet is set when a new jail is created, but the flags are not actually set until after some parameter checking and other error opportunities. Chances are, this just requires setting the prison's PR_VNET flag at the same time the vnet is created (still saving other flags for later).

Revert unintended style change.

The new PR_VNET_ROOT flag is unneeded: it's equivalent to PR_VNET, just check
that instead.

In D48058#1096826, @mhorne wrote:

I have no objection to the current revision, although we can say a few things about how the code may look/change in the future.

Obviously, the feature-to-ISA-string code will continue to expand/balloon, as we will want to advertise the availability of the many unprivileged ISA extensions to the guest as well.

The problem is that we have no mechanism for reporting extension presence from the kernel to userspace, therefore you are using vm_cap_type here. I am working on something "official" for this, most likely it will be a sysarch exporting a bitmap, with bit definitions compatible to what can be found in Linux. Therefore one call can obtain the full set of supported extensions (sanitized), and we will be able to use this in bhyve to construct the ISA string for the guest.

So I don't think we will need to rely on vm_cap_type in the long run. We also don't need to be precious about the ABI, given the experimental state of RISC-V vmm and hypervisor hardware, and since I intend for this extension reporting stuff to be resolved before the release of 15.0. So this VM_CAP_SSTC can easily be removed later.

Any comments on the patch?

In D48034#1095816, @markj wrote:

as suggested, it'd be more natural to cache the value in a local variable instead, assuming that the compiler doesn't recognize that the value is constant and hoist that computation out the loop (which would make this diff even less useful).

In D48093#1096464, @olce wrote:

Thanks. But while we're here, shouldn't it wait for D47956 as well? Going to update it in a short while to separate the change of switching the generation number to 32 bits to another revision so that the rest can be reviewed and hopefully approved/committed shortly.

Update UPDATING.

As usual, I will quibble a bit about naming - the kyua subcommands are mostly english words, so "rr" is a bit odd. Why not "resolve" or even "resolve-requirements" (or "resolve-reqs")?

In D47879#1095707, @olce wrote:

In D47879#1093639, @rmacklem wrote:

Just fyi, whenever the "struct fid" changes,
(snip)
RELNOTES entry is needed.

Yes. Not sure if it's worth it for these filesystems. Mark didn't do that.

In D46653#1096065, @ngie wrote:

In D46653#1092361, @markj wrote:

In D46653#1091544, @ngie wrote:

I think being added to the FreeBSD org is documented somewhere in the Committer's Guide. CC: @markj

I don't see any docs on that. Am I missing something?

In any case, I submitted https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=283127

Regarding the change itself, I have a hard time believing any serious project is parsing the result summary line and using that as input to something else. Do we know of any actual examples?

I kind of thing option 4 makes the most sense..

For the run of the FreeBSD regression suite that I just completed, option 4 gives:

Test cases: 9032 total, 211 skipped, 33 expected failures, 1 broken, 6 failed

Whereas kyua currently prints:

8814/9032 passed (1 broken, 6 failed, 211 skipped)

The issue is the ratio… random folks who use other frameworks might be confused by it. My first gut reaction was “X testcases failed…?! Oh wait… they were skipped—really??”. pytest for instance doesn’t emit the passed count as a ratio, but instead outputs the actual count in each category: https://slar.se/essential-pytest-1.html .

Skipped tests shouldn’t count against passed or failed tests. The ratio as it stands is confusing. I vote for just outputting the raw values; non-verbose output can just omit categories without any results — seems like a reasonable compromise from the -v output.

In D47868#1092802, @christos wrote:

@markj What do you think?

I'm skeptical that this change makes any measurable difference. And, as suggested, it'd be more natural to cache the value in a local variable instead, assuming that the compiler doesn't recognize that the value is constant and hoist that computation out the loop (which would make this diff even less useful).

Avoid modifying prison_deref_kill().

In D47935#1094163, @glebius wrote:

In D47935#1093253, @markj wrote:

Also, in your stack, why are we using kmem_malloc() to allocate a slab for inpcbs? That should be going through uma_small_alloc(), which doesn't need KVA. Was the kernel compiled with KASAN enabled?

No, KASAN is not in the kernel. The inpcb keg has uk_ppera = 4, hence its uk_allocf is page_alloc() which is basically kmem_malloc_domainset(). Our inpcb is 0x880 bytes, so I guess keg_layout() calculated 4 page slabs as most efficient.