In D58328#1365066, @ziaee wrote:

Thanks for the ping. So I dug in a little further. The existing description is actually aligned with a library, mktemp.3 which has the same description. It can make not only directories, but pipes and fifos and... (etc) which are all normalized to files. Further, mktemp was written by Todd C Miller and even has a website https://www.mktemp.org/ and seems to have the same Nd in every single OS because we're all using the same implementation. So, we should not change it. In fact perhaps it should be in contrib/ to make that more obvious but I do not want to grind that axe.

Thanks for the ping. So I dug in a little further. The existing description is actually aligned with a library, mktemp.3 which has the same description. It can make not only directories, but pipes and fifos and... (etc) which are all normalized to files. Further, mktemp was written by Todd C Miller and even has a website https://www.mktemp.org/ and seems to have the same Nd in every single OS because we're all using the same implementation. So, we should not change it. In fact perhaps it should be in contrib/ to make that more obvious but I do not want to grind that axe.

Ping.

Ping.

Ping.

In D59356#1363271, @pho wrote:

In D59356#1363214, @kib wrote:

From the first look, this is fine.
I believe the next step is to help Peter (pho@) to create the reproducer, and get the stress2 testing.
Also, please wait for Kirk' feedback.

That would be nice. I have been seeing the panic a few times over the years, but not after Feb 10 2021.

In D59356#1363214, @kib wrote:

From the first look, this is fine.
I believe the next step is to help Peter (pho@) to create the reproducer, and get the stress2 testing.
Also, please wait for Kirk' feedback.

From the first look, this is fine.
I believe the next step is to help Peter (pho@) to create the reproducer, and get the stress2 testing.
Also, please wait for Kirk' feedback.

In D59356#1362297, @sobomax wrote:

In D59356#1362256, @kib wrote:

The new 'find' logic essentially reorders the processing of the newblk dependencies, and I am not sure that this is fine in this case (it is generally not).

IMO the correct solution for the race in flush_pagedep_deps() as described in the summary is to memoize the vnode returned by get_parent_vp(), and then restart the global loop. When you see the MKDIR_BODY dependency, and the inode number matches the memoized result from get_parent_vp(), you can process it without relocking sd_fslock, which closes the race. If the vp is NULL or does not match the inode number, you vput() it, call get_parent_vp(), and restart the loop.

Thanks, I agree that scanning past the first matching newblk is not appropriate because it changes dependency-processing order.

I will rework flush_pagedep_deps() to retain the vnode returned by get_parent_vp(), reacquire sd_fslock, and restart dependency selection. The MKDIR_BODY operation will only proceed when the newly selected dependency's inode matches the retained vnode, avoiding use of the dap that was selected before get_parent_vp() dropped the lock.

One detail I want to confirm: flush_newblk_dep() also drops and reacquires sd_fslock internally around bwrite() and ffs_update(), then repeats newblk_lookup(). If the D_ALLOCDIRECT completes during one of those operations, the next lookup could expose the retained D_ALLOCINDIR in the same way. Should flush_newblk_dep() return to the outer dependency-selection loop after such an operation, or revalidate the associated MKDIR_BODY dependency after reacquiring sd_fslock?

Rework to preserve dependencies as suggested by @kib

In D59356#1362256, @kib wrote:

The new 'find' logic essentially reorders the processing of the newblk dependencies, and I am not sure that this is fine in this case (it is generally not).

IMO the correct solution for the race in flush_pagedep_deps() as described in the summary is to memoize the vnode returned by get_parent_vp(), and then restart the global loop. When you see the MKDIR_BODY dependency, and the inode number matches the memoized result from get_parent_vp(), you can process it without relocking sd_fslock, which closes the race. If the vp is NULL or does not match the inode number, you vput() it, call get_parent_vp(), and restart the loop.

The new 'find' logic essentially reorders the processing of the newblk dependencies, and I am not sure that this is fine in this case (it is generally not).

Some of the facts supporting this diagnosis:

In D59277#1360135, @ngie wrote:

I think the prior approach suggested by @dim to make as minimal of a change as possible makes a lot of sense: there's no real single-maintainer in inetd and the daemon has remained in maintenance mode for many years.

Approved by: ngie (co-mentor)

revert refactoring

I think the prior approach suggested by @dim to make as minimal of a change as possible makes a lot of sense: there's no real single-maintainer in inetd and the daemon has remained in maintenance mode for many years.

refactor and dedup code

Thank you again for your review.

I built the world without ipv6 support . We don't test that in CI.

How did you produce the warning? I don't see it when building inetd. In any case, I think it would be better to only declare and use nbuf2 and netid2 if INET6 is defined. This way you can avoid the warning, and no unused data is allocated or used.

@alfred was the one who touched the line last, but he's been inactive for many years now.
@des or @kevans: this change seems straightforward, but do you have any objections to this change? I ask because Klara has taken an active role at cleaning up portions of the base system lately.

I'm not sure who to route this to for review. I'll go poke around!

I like it, lets give it a few days to see if anyone else has any suggestions to improve it further.

Ping.

Ping.

Ping.

First of all, THANK YOU VERY MUCH for your review.

In D58457#1342400, @ziaee wrote:

This line was forgotten to be updated when information about st_birthtime was first added into the manual page.

Nice, so we want to find that commit and put it at the bottom of the commit message like Fixes: <shorthash> ("Commit title").

In D58476#1342416, @ziaee wrote:

So we've been trying to standardize this language, here's the draft: https://reviews.freebsd.org/D57690

Use standardized formulation.

So we've been trying to standardize this language, here's the draft: https://reviews.freebsd.org/D57690

Hey Artem, thanks for the patch.