Page MenuHomeFreeBSD
Differential D44165

ktls: enumify tcp_tls_mode magic numbers
AbandonedPublic
Actions

Authored by rscheff on Mar 1 2024, 1:45 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sun, Mar 9, 11:56 AM
Unknown Object (File)
Fri, Mar 7, 10:50 PM
Unknown Object (File)
Jan 10 2025, 2:34 PM
Unknown Object (File)
Jan 3 2025, 11:15 AM
Unknown Object (File)
Dec 8 2024, 6:28 PM
Unknown Object (File)
Oct 5 2024, 6:12 AM
Unknown Object (File)
Oct 4 2024, 2:44 PM
Unknown Object (File)
Oct 3 2024, 12:12 PM
View All 38 Files
Subscribers
cc
glebius
imp
melifaro
olce
tuexen

Details

Reviewers
cc
jhb
emaste
tuexen
Group Reviewers
transport
Summary

In order to allow stricter type checking,
convert KTLS related magic numbers to enum.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 56362
Build 53250: arc lint + arc unit

Event Timeline

rscheff created this revision.Mar 1 2024, 1:45 PM
Herald added a reviewer: transport. · View Herald TranscriptMar 1 2024, 1:45 PM
Herald added subscribers: olce, glebius, melifaro, imp. · View Herald Transcript
rscheff requested review of this revision.Mar 1 2024, 1:45 PM
Harbormaster completed remote builds in B56361: Diff 135258.Mar 1 2024, 1:45 PM
cc added a subscriber: cc.Mar 1 2024, 1:58 PM

The ktls draws my attention. Hope you add reviewers.

sys/sys/ktls.h
153

This extra empty line is not necessary.

223

This extra empty line is not necessary.

rscheff updated this revision to Diff 135259.Mar 1 2024, 2:08 PM

whitespace fix

Harbormaster completed remote builds in B56362: Diff 135259.Mar 1 2024, 2:08 PM
rscheff updated this revision to Diff 135260.Mar 1 2024, 2:12 PM
rscheff marked an inline comment as done.

whitespace fix 2

Harbormaster completed remote builds in B56363: Diff 135260.Mar 1 2024, 2:12 PM
tuexen added a subscriber: tuexen.Mar 1 2024, 4:22 PM
tuexen added inline comments.
sys/sys/ktls.h
151

Why do you move the definition from tcp.h to ktls.h? At least the names sound to be TCP specific.

rscheff marked an inline comment as done.Mar 1 2024, 5:44 PM
rscheff added inline comments.
sys/sys/ktls.h
151

To minimize header churn; many drivers don't actually use tcp.h (and use the universally availabe int) prior to enum. But all drivers with tls offload to use ktls.h.

with the typedef in tcp.h, ktls.h would also have to come past tcp.h.

overall this was more of a test case, if the -Wswitch for clang would properly catch missing default / incomplete enum conditions in switch statements throughout the tree (and it worked, my clang caught the missing default in uipc_ktls.c.

I used the phabricator to get this patch from my local machine to the beefy build vm I allocated the other day.

But then again, I think using enums more broadly, and thus allowing the compiler to type-check and verify style(9) would have a few benefits...

rscheff added reviewers: cc, jhb, emaste, tuexen.Mar 1 2024, 8:58 PM
cc accepted this revision as: cc.Mar 4 2024, 1:05 AM
jhb added a comment.Mar 4 2024, 11:07 PM

Note that the mode is tcp.h because it is a user-facing API (you can use it with setsockopt/getsockopt) and needs to stay in tcp.h

sys/kern/uipc_ktls.c
1941

This should be __assert_unreachable(). It is not a reachable case (and a false positive from the compiler warning).

sys/netinet/tcp_usrreq.c
2663

I'm not a super fan of the cast and assuming that the enum aliases to an int. I would rather have a local variable in this function of the right type that is assigned to optval before the call to sooptcopyout.

rscheff abandoned this revision.Mar 7 2024, 4:03 PM
rscheff marked an inline comment as done.

Discussed transport

Revision Contents
Changeset List

PathSize
sys/
kern/
8 lines
netinet/
6 lines
4 lines
sys/
17 lines

Diff 135259

View Options

sys/kern/uipc_ktls.c

Context not available.
static int static int
bpf_gettime(struct bintime *bt, int tstype, struct mbuf *m) bpf_gettime(struct bintime *bt, int tstype, struct mbuf *m)
{ {
struct timespec ts;
struct m_tag *tag; struct m_tag *tag;
int quality; int quality;
Context not available.
return (quality); return (quality);
if (m != NULL) { if (m != NULL) {
if ((m->m_flags & (M_PKTHDR | M_TSTMP)) == (M_PKTHDR | M_TSTMP)) {
mbuf_tstmp2timespec(m, &ts);
timespec2bintime(&ts, bt);
kibUnsubmitted
Done
Inline Actions

Please use mbuf_tstmp2timespec(), somebody might add more precision to it one day.

kib: Please use mbuf_tstmp2timespec(), somebody might add more precision to it one day.
jkimAuthorUnsubmitted
Done
Inline Actions

Thanks, I didn't see that.

jkim: Thanks, I didn't see that.
return (BPF_TSTAMP_EXTERN);
}
tag = m_tag_locate(m, MTAG_BPF, MTAG_BPF_TIMESTAMP, NULL); tag = m_tag_locate(m, MTAG_BPF, MTAG_BPF_TIMESTAMP, NULL);
if (tag != NULL) { if (tag != NULL) {
*bt = *(struct bintime *)(tag + 1); *bt = *(struct bintime *)(tag + 1);
Context not available.
View Options

sys/netinet/tcp.h

Context not available.
static int static int
bpf_gettime(struct bintime *bt, int tstype, struct mbuf *m) bpf_gettime(struct bintime *bt, int tstype, struct mbuf *m)
{ {
struct timespec ts;
struct m_tag *tag; struct m_tag *tag;
int quality; int quality;
Context not available.
return (quality); return (quality);
if (m != NULL) { if (m != NULL) {
if ((m->m_flags & (M_PKTHDR | M_TSTMP)) == (M_PKTHDR | M_TSTMP)) {
mbuf_tstmp2timespec(m, &ts);
timespec2bintime(&ts, bt);
kibUnsubmitted
Done
Inline Actions

Please use mbuf_tstmp2timespec(), somebody might add more precision to it one day.

kib: Please use mbuf_tstmp2timespec(), somebody might add more precision to it one day.
jkimAuthorUnsubmitted
Done
Inline Actions

Thanks, I didn't see that.

jkim: Thanks, I didn't see that.
return (BPF_TSTAMP_EXTERN);
}
tag = m_tag_locate(m, MTAG_BPF, MTAG_BPF_TIMESTAMP, NULL); tag = m_tag_locate(m, MTAG_BPF, MTAG_BPF_TIMESTAMP, NULL);
if (tag != NULL) { if (tag != NULL) {
*bt = *(struct bintime *)(tag + 1); *bt = *(struct bintime *)(tag + 1);
Context not available.
View Options

sys/netinet/tcp_usrreq.c

Context not available.
static int static int
bpf_gettime(struct bintime *bt, int tstype, struct mbuf *m) bpf_gettime(struct bintime *bt, int tstype, struct mbuf *m)
{ {
struct timespec ts;
struct m_tag *tag; struct m_tag *tag;
int quality; int quality;
Context not available.
return (quality); return (quality);
if (m != NULL) { if (m != NULL) {
if ((m->m_flags & (M_PKTHDR | M_TSTMP)) == (M_PKTHDR | M_TSTMP)) {
mbuf_tstmp2timespec(m, &ts);
timespec2bintime(&ts, bt);
kibUnsubmitted
Done
Inline Actions

Please use mbuf_tstmp2timespec(), somebody might add more precision to it one day.

kib: Please use mbuf_tstmp2timespec(), somebody might add more precision to it one day.
jkimAuthorUnsubmitted
Done
Inline Actions

Thanks, I didn't see that.

jkim: Thanks, I didn't see that.
return (BPF_TSTAMP_EXTERN);
}
tag = m_tag_locate(m, MTAG_BPF, MTAG_BPF_TIMESTAMP, NULL); tag = m_tag_locate(m, MTAG_BPF, MTAG_BPF_TIMESTAMP, NULL);
if (tag != NULL) { if (tag != NULL) {
*bt = *(struct bintime *)(tag + 1); *bt = *(struct bintime *)(tag + 1);
Context not available.
View Options

sys/sys/ktls.h

Context not available.
static int static int
bpf_gettime(struct bintime *bt, int tstype, struct mbuf *m) bpf_gettime(struct bintime *bt, int tstype, struct mbuf *m)
{ {
struct timespec ts;
struct m_tag *tag; struct m_tag *tag;
int quality; int quality;
Context not available.
return (quality); return (quality);
if (m != NULL) { if (m != NULL) {
if ((m->m_flags & (M_PKTHDR | M_TSTMP)) == (M_PKTHDR | M_TSTMP)) {
mbuf_tstmp2timespec(m, &ts);
timespec2bintime(&ts, bt);
kibUnsubmitted
Done
Inline Actions

Please use mbuf_tstmp2timespec(), somebody might add more precision to it one day.

kib: Please use mbuf_tstmp2timespec(), somebody might add more precision to it one day.
jkimAuthorUnsubmitted
Done
Inline Actions

Thanks, I didn't see that.

jkim: Thanks, I didn't see that.
return (BPF_TSTAMP_EXTERN);
}
tag = m_tag_locate(m, MTAG_BPF, MTAG_BPF_TIMESTAMP, NULL); tag = m_tag_locate(m, MTAG_BPF, MTAG_BPF_TIMESTAMP, NULL);
if (tag != NULL) { if (tag != NULL) {
*bt = *(struct bintime *)(tag + 1); *bt = *(struct bintime *)(tag + 1);
Context not available.