Presumably the caller should have a credential matching that of the caller? Or does something else prevent a user from dumping all of the session keys?

Extra newline.

Will you add pad bytes for this?

How does xig get initialized?

I missed the cr_canseeinpcb() call, but it still seems too relaxed.

Looks like there is just a global sysctl and you can either dump all of the keys or none of them. Presumably a priv_check() of some sort should be called in the with-keys variant? (I don't think there is one today)

Looks like this should move up before the first loop to answer Mark's question? (Except you still have to initialize xig_count here?)

How is userspace expected to detect cnt vs cnt1 mismatches? Is it based on looking at the end of the buffer for a xig structure? In that case, can the count in the first record just be 0? (And then you can indeed just initialize xig before the first loop)

Then, if cr_canseeinpcb() is to weak, what other checks do you suggest? I did not come with any other suitable restrictions there. canseeinpcb is roughly equivalent to candebug, which means that the keys could be extracted with userspace debugger anyway.

What do you mean? I do not reserve fixed space for keys, so I cannot express it in C. I can add a flexible array with data at the end of the structure, but its utility is not great.

Intent is that the generation count is copied out in the prepended xig, and actual count is copied out in the appended xig. I messed this up eventually.

This is the natural approach IMO: we take the gen counts snapshots before looping, and filter the list based on the read gen count values. Then we know the new value for the element count at the end, and copy it out appending the xig. Also we can provide the suggested new gen count values to user so that userspace can detect a change if so willing.

cr_canseeinpcb() is more permissive than candebug(): it lets an unprivileged user see any other user's socket connection metadata. I think something equivalent to p_candebug() would be ok.

I misunderstood the comment.

I added a place where all policy can be put, for now the only part to add I see is the ids subset check.

Extra newline.

In general I think we shouldn't provide the IV unless the canexportkeys predicate is true. It's not as sensitive as the session keys, but it should still be kept hidden.

We should zero the buffer as well, e.g., with zfree().

Missing comments here.

The buffers for keys cannot be fixed length, TLS_MAX_PARAM_SIZE is way to large to be practical even for modest amount of ktls sessions total. MAX is 1K, which means that wiring would need hundreds of KB if not MB.

Well, this is the same constant used by tls_session_param several lines below.

I added a dedicated defined symbol and bumped it to 32 (from 16).

I really don't like adding an atomic to the critical path (and yes, connection establishment is a critical path for us).

What is the need for a global generation count?

The gen count is to make it possible for the sysctl to distinguish ktls sessions for before/after the gen count snapshot was taken.

The count is updated by atomic because there is no any other protection against parallel modifications for ktls_glob_gen.

Really, the ktls session creation typically programs a slow hardware. It takes several mutexes on the path, and pushes the initialization data into the dump sq (on mlx5). Do you actually see some effect from this atomic, or it is a generic dislike?

I noticed it in our internal code review when we brought in the last few weeks of FreeBSD's main branch.

The difference between this and the other slow stuff is that some of those mutexes are uncontended, and some are per-port (so load is at least sharded 4 ways). This is likely to be a contended atomic across 96 or more cores with more than 100k increments a second in times of heavy load.

I don't know much about the timecounter subsystem, but would it work to use getnanouptime() to record the creation of the session, and disambiguate them using address?

Please see D51000 for the proposed way to get rid of the atomic.
I suppose you do not care about the sysctl or about the parallel execution of the sysctl.