[WIP] ping: set random ident also for ICMP datagram socket #490
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Hi! |
Before 87dbb3a both raw socket and ICMP datagram socket used random ident (PID of ping at the time). 87dbb3a stop setting random ident for ICMP datagram socket, instead the default value max value 65535 (via htons(-1)) was used. This commit removed the need of root privileges for running ping and maybe that was needed at the time (2015-05-29: kernel v4.0 or maybe 3.16.x which already had ICMP datagram socket support and was still used) ICMP datagram socket really required root for setting ident. But that's not true any more now. Tested on kernel 6.4.x, 5.10.x. Kernel v4.14.x is still LTS, but these are used on distros which will not update iputils. Fixes: 87dbb3a ("This patch allows running ping and ping6 without root privileges on kernels that support it. Almost identical to Lorenzo Colitti's original patch except:") Signed-off-by: Petr Vorel <pvorel@suse.cz>
|
@rumvadim Thanks for a hint (could you quote kernel code?), I removed that code and update docs, but I need to verify that with I also set a random ident also for ICMP datagram socket, thus there should be always |
pevik
force-pushed
the
ping/fix-print-default-ident
branch
from
3d103e0
to
f2e9f55
Compare
pevik
added a commit
to pevik/iputils
that referenced
this pull request
Sep 21, 2023
Document the reason why ident -1 is not printed: When the ident is -1, the ident for the group of requests is chosen by the kernel and most likely it's not 65535 (htons(-1)). Link: iputils#490 (comment) Suggested-by: Vadim Rumyantsev <vadelve@yandex.ru> Signed-off-by: Petr Vorel <pvorel@suse.cz>
pevik
force-pushed
the
ping/fix-print-default-ident
branch
from
f2e9f55
to
59da996
Compare
When the ident is -1 the ident for the group of requests is chosen by the kernel and most likely it's not 65535. That's why it was not printed in 43e38f2. But since previous commit random ident was set even for ICMP datagram socket and we don't allow to set ident < 0. Thus it should be safe to always print ident. Link: iputils#490 (comment) Link: iputils#489 (comment) Reported-by: Miloslav Hůla Signed-off-by: Petr Vorel <pvorel@suse.cz>
pevik
force-pushed
the
ping/fix-print-default-ident
branch
from
59da996
to
286707b
Compare
|
@nmav Could you please also have look? |
pevik
changed the title
|
The kernel code is here: https://elixir.bootlin.com/linux/latest/source/net/ipv4/ping.c#L78 (ping_get_port) |
pevik
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Before 87dbb3a both raw socket and ICMP datagram socket used
random ident (PID of ping at the time). 87dbb3a stop setting
random ident for ICMP datagram socket, instead the default value max
value 65535 (via htons(-1)) was used.
This commit removed the need of root privileges for running ping and
maybe that was needed at the time (2015-05-29: kernel v4.0 or maybe
3.16.x which already had ICMP datagram socket support and was still
used) ICMP datagram socket really required root for setting ident. But
that's not true any more now.
Tested on kernel 6.4.x, 5.10.x. Kernel v4.14.x is still LTS, but these
are used on distros which will not update iputils.
Fixes: 87dbb3a
When the ident is -1 the ident for the group of requests is chosen by
the kernel and most likely it's not 65535. That's why it was not printed
in 43e38f2. But since previous commit random ident was set even for ICMP
datagram socket and we don't allow to set ident < 0. Thus it should be
safe to always print ident.
#489 (comment)