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.

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

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

Open
payload opened this issue Jan 14, 2025 · 1 comment · May be fixed by #375
Open

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

payload opened this issue Jan 14, 2025 · 1 comment · May be fixed by #375
Assignees
Labels

Comments

@payload
Copy link

payload commented Jan 14, 2025

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
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 linked a pull request Jan 16, 2025 that will close this issue
@auerswal auerswal linked a pull request Jan 16, 2025 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants