Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

When falling back to SO_TIMESTAMP reply timestamp is probably not working #374

Closed
payload opened this issue Jan 14, 2025 · 1 comment · Fixed by #375
Closed

When falling back to SO_TIMESTAMP reply timestamp is probably not working #374

payload opened this issue Jan 14, 2025 · 1 comment · Fixed by #375
Assignees
@auerswal
Labels
bug

Comments

@payload
Copy link

payload commented Jan 14, 2025 •
edited
Loading

I noticed in this commit
e866063#commitcomment-151310711
that when fping can't setsockopt SO_TIMESTAMPNS it falls back to SO_TIMESTAMP. But the reading of the cmsg data is not considering this fallback. In that case no timestamp would be read. I think the fallback is not completely implemented. In that case the reply time is still coming from this line

fping/src/fping.c

Line 2454 in cb83286

recv_time = current_time_ns;

Relevant documentation: https://docs.kernel.org/networking/timestamping.html
@auerswal
Copy link
Collaborator

Thanks for the report!

There seems to be code missing to handle non-nanosecond timestamps. Only nanosecond resolution timestamp messages are handled:

fping/src/fping.c

Lines 2104 to 2117 in cb83286

#if HAVE_SO_TIMESTAMPNS
/* ancilliary data */
{
struct timespec reply_timestamp_ts;
for (cmsg = CMSG_FIRSTHDR(&recv_msghdr);
cmsg != NULL;
cmsg = CMSG_NXTHDR(&recv_msghdr, cmsg)) {
if (cmsg->cmsg_level == SOL_SOCKET && cmsg->cmsg_type == SCM_TIMESTAMPNS) {
memcpy(&reply_timestamp_ts, CMSG_DATA(cmsg), sizeof(reply_timestamp_ts));
*reply_timestamp = timespec_ns(&reply_timestamp_ts);
}
}
}
#endif

According to the socket(7) man page, the fall back to SO_TIMESTAMP should result in SCM_TIMESTAMP control messages where the timestamp is in struct timeval format. This differs from SO_TIMESTAMPNS which uses SCM_TIMESTAMPNS control messages and struct timespec.

@auerswal auerswal added the bug label Jan 15, 2025
@auerswal auerswal self-assigned this Jan 16, 2025
auerswal added a commit to auerswal/fping that referenced this issue Jan 16, 2025
@auerswal
complete SO_TIMESTAMPNS to SO_TIMESTAMP fallback
fdd5dee 
Commit e866063 added a
fallback from setting the socket option SO_TIMESTAMPNS to
setting the socket option SO_TIMESTAMP if the nanosecond
timestamp option could not be set.  But it did not add
code to also look for the control message related to
SO_TIMESTAMP.  Thus microsecond timestamps were requested,
but not read.

This commit adds the missing code to read microsecond
timestamp control messages.

The problem was reported in GitHub issue schweikert#374 by @payload.
@auerswal auerswal mentioned this issue Jan 16, 2025
Merged
@auerswal auerswal linked a pull request Jan 16, 2025 that will close this issue
complete SO_TIMESTAMPNS to SO_TIMESTAMP fallback #375
Merged
auerswal added a commit that referenced this issue Jan 26, 2025
@auerswal
complete SO_TIMESTAMPNS to SO_TIMESTAMP fallback
511aa37 
Commit e866063 added a
fallback from setting the socket option SO_TIMESTAMPNS to
setting the socket option SO_TIMESTAMP if the nanosecond
timestamp option could not be set.  But it did not add
code to also look for the control message related to
SO_TIMESTAMP.  Thus microsecond timestamps were requested,
but not read.

This commit adds the missing code to read microsecond
timestamp control messages.

The problem was reported in GitHub issue #374 by @payload.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@auerswal auerswal

Labels
bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

complete SO_TIMESTAMPNS to SO_TIMESTAMP fallback auerswal/fping
2 participants
@payload @auerswal