Re: [LTP] [PATCH] ssh-stress: disable resource penalties

Re: [LTP] [PATCH] ssh-stress: disable resource penalties

[Petr Vorel]

Hi Vasileios,

first, our ML requires subscription (unlike kernel's lore). I subscribed you then.

> Our tests create a number of ssh sessions in the
> background which are immediately killed. Some of
> them haven't finished the authentication stage yet
> and they close the connection incurring penalties from
> the ssh daemon.

> debug1: srclimit_penalise: active penalty for ipv4 10.0.0.1/32
> already exists, 16 seconds remaining

> Then when we try to reconnect to the daemon we are bounced
> because of the active penalty which leads to a failed test.

> ssh-stress 1 TINFO: Killing all ssh sessions
> kex_exchange_identification: read: Connection reset by peer
> Connection reset by fd00:1:1:1::2 port 58373
> ssh-stress 1 TFAIL: SSH not reachable

> From the sshd logs we can see

> debug1: srclimit_penalise: active penalty for ipv4 10.0.0.1/32
> already exists, 16 seconds remaining

> This feature was added to OpenSSH 9.8 in the 2024 release in the
> 81c1099d2 commit. Lets disable penalties for the versions that
> support them.

> Signed-off-by: Vasileios Almpanis <vasileios.almpanis@virtuozzo.com>
> ---
>  testcases/network/stress/ssh/ssh-stress.sh | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)

> diff --git a/testcases/network/stress/ssh/ssh-stress.sh b/testcases/network/stress/ssh/ssh-stress.sh
> index c27c27a28..cb6659ed5 100755
> --- a/testcases/network/stress/ssh/ssh-stress.sh
> +++ b/testcases/network/stress/ssh/ssh-stress.sh
> @@ -39,8 +39,12 @@ cleanup()

>  setup()
>  {
> -	local port rc
> +	local port rc version major minor

> +	version=$(sshd -V 2>&1 | sed -nE 's/^.*OpenSSH_([0-9]+)\.([0-9]+).*$/\1 \2/p' | head -n1)
> +	set -- $version
> +	major=$1
> +	minor=$2

Interesting, I never used set like this.  FYI we suppose POSIX shell
compatibility, i.e. it should work on dash and busybox sh. At least
checkbashisms does not complain therefore it looks to be valid. Testing just
this part locally on both dash and busybox sh shows it's working.

>  	port=$(tst_rhost_run -c "tst_get_unused_port ipv${TST_IPVER} stream")

> @@ -60,6 +64,13 @@ HostKey $TST_TMPDIR/ssh_host_ecdsa_key
>  HostKey $TST_TMPDIR/ssh_host_ed25519_key
>  EOF

> +	if ([ -n "$major" ] && [ -n "$minor" ]); then
	if [ -n "$major" ] && [ -n "$minor" ]; then
nit: IMHO this could be without curly brackets, right? ( ). Any reason to add
them?

> +		if ([ "$major" -gt 9 ] || ([ "$major" -eq 9 ] && [ "$minor" -ge 8 ])); then
I never tried ( ) to force evaluation. I hope it works on POSIX only shell
(dash, busybox sh). BTW IMHO it should work as (which is POSIX compatible):

		if [ "$major" -gt 9 ] || [ "$major" -eq 9 -a "$minor" -ge 8 ]; then

The rest LGTM.

Kind regards,
Petr

> +			cat << EOF >> sshd_config
> +PerSourcePenalties no
> +EOF
> +		fi
> +	fi
>  	ssh-keygen -q -N "" -t rsa -b 4096 -f $TST_TMPDIR/ssh_host_rsa_key
>  	ssh-keygen -q -N "" -t ecdsa -f $TST_TMPDIR/ssh_host_ecdsa_key
>  	ssh-keygen -q -N "" -t ed25519 -f $TST_TMPDIR/ssh_host_ed25519_key

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH] ssh-stress: disable resource penalties

[Petr Vorel]

Hi Vasileios,

first, our ML requires subscription (unlike kernel's lore). I subscribed you
then. I'm replying to your second mail which got to ML.

FYI these tests are probably not run by many people (most of the people run
tests from runtest/syscalls or other C based tests).

> Our tests create a number of ssh sessions in the
> background which are immediately killed. Some of
> them haven't finished the authentication stage yet
> and they close the connection incurring penalties from
> the ssh daemon.

> debug1: srclimit_penalise: active penalty for ipv4 10.0.0.1/32
> already exists, 16 seconds remaining

> Then when we try to reconnect to the daemon we are bounced
> because of the active penalty which leads to a failed test.

> ssh-stress 1 TINFO: Killing all ssh sessions
> kex_exchange_identification: read: Connection reset by peer
> Connection reset by fd00:1:1:1::2 port 58373
> ssh-stress 1 TFAIL: SSH not reachable

> From the sshd logs we can see

> debug1: srclimit_penalise: active penalty for ipv4 10.0.0.1/32
> already exists, 16 seconds remaining

> This feature was added to OpenSSH 9.8 in the 2024 release in the
> 81c1099d2 commit. Lets disable penalties for the versions that
> support them.

> Signed-off-by: Vasileios Almpanis <vasileios.almpanis@virtuozzo.com>
> ---
>  testcases/network/stress/ssh/ssh-stress.sh | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)

> diff --git a/testcases/network/stress/ssh/ssh-stress.sh b/testcases/network/stress/ssh/ssh-stress.sh
> index c27c27a28..cb6659ed5 100755
> --- a/testcases/network/stress/ssh/ssh-stress.sh
> +++ b/testcases/network/stress/ssh/ssh-stress.sh
> @@ -39,8 +39,12 @@ cleanup()

>  setup()
>  {
> -	local port rc
> +	local port rc version major minor

> +	version=$(sshd -V 2>&1 | sed -nE 's/^.*OpenSSH_([0-9]+)\.([0-9]+).*$/\1 \2/p' | head -n1)
> +	set -- $version
> +	major=$1
> +	minor=$2

Interesting, I never used set like this.  FYI we suppose POSIX shell
compatibility, i.e. it should work on dash and busybox sh. At least
checkbashisms does not complain therefore it looks to be valid. Testing just
this part locally on both dash and busybox sh shows it's working.

>  	port=$(tst_rhost_run -c "tst_get_unused_port ipv${TST_IPVER} stream")

> @@ -60,6 +64,13 @@ HostKey $TST_TMPDIR/ssh_host_ecdsa_key
>  HostKey $TST_TMPDIR/ssh_host_ed25519_key
>  EOF

> +	if ([ -n "$major" ] && [ -n "$minor" ]); then
	if [ -n "$major" ] && [ -n "$minor" ]; then
nit: IMHO this could be without curly brackets, right? ( ). Any reason to add
them?

> +		if ([ "$major" -gt 9 ] || ([ "$major" -eq 9 ] && [ "$minor" -ge 8 ])); then
I never tried ( ) to force evaluation. I hope it works on POSIX only shell
(dash, busybox sh). BTW IMHO it should work as (which is POSIX compatible):

		if [ "$major" -gt 9 ] || [ "$major" -eq 9 -a "$minor" -ge 8 ]; then

Kind regards,
Petr

> +			cat << EOF >> sshd_config
> +PerSourcePenalties no
> +EOF
> +		fi
> +	fi
>  	ssh-keygen -q -N "" -t rsa -b 4096 -f $TST_TMPDIR/ssh_host_rsa_key
>  	ssh-keygen -q -N "" -t ecdsa -f $TST_TMPDIR/ssh_host_ecdsa_key
>  	ssh-keygen -q -N "" -t ed25519 -f $TST_TMPDIR/ssh_host_ed25519_key

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

[LTP] [PATCH v2 1/1] ssh-stress: disable resource penalties

[Vasileios Almpanis via ltp]

Our tests create a number of ssh sessions in the
background which are immediately killed. Some of
them haven't finished the authentication stage yet
and they close the connection incurring penalties from
the ssh daemon.

debug1: srclimit_penalise: active penalty for ipv4 10.0.0.1/32
already exists, 16 seconds remaining

Then when we try to reconnect to the daemon we are bounced
because of the active penalty which leads to a failed test.

ssh-stress 1 TINFO: Killing all ssh sessions
kex_exchange_identification: read: Connection reset by peer
Connection reset by fd00:1:1:1::2 port 58373
ssh-stress 1 TFAIL: SSH not reachable

From the sshd logs we can see

debug1: srclimit_penalise: active penalty for ipv4 10.0.0.1/32
already exists, 16 seconds remaining

This feature was added to OpenSSH 9.8 in the 2024 release in the
81c1099d2 commit. Lets disable penalties for the versions that
support them.

Signed-off-by: Vasileios Almpanis <vasileios.almpanis@virtuozzo.com>
---
Changes in v2:
- Removed unnecessary parenthesis around if statements.
---
 testcases/network/stress/ssh/ssh-stress.sh | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/testcases/network/stress/ssh/ssh-stress.sh b/testcases/network/stress/ssh/ssh-stress.sh
index c27c27a28..d5db24835 100755
--- a/testcases/network/stress/ssh/ssh-stress.sh
+++ b/testcases/network/stress/ssh/ssh-stress.sh
@@ -39,8 +39,12 @@ cleanup()
 
 setup()
 {
-	local port rc
+	local port rc version major minor
 
+	version=$(sshd -V 2>&1 | sed -nE 's/^.*OpenSSH_([0-9]+)\.([0-9]+).*$/\1 \2/p' | head -n1)
+	set -- $version
+	major=$1
+	minor=$2
 
 	port=$(tst_rhost_run -c "tst_get_unused_port ipv${TST_IPVER} stream")
 
@@ -60,6 +64,13 @@ HostKey $TST_TMPDIR/ssh_host_ecdsa_key
 HostKey $TST_TMPDIR/ssh_host_ed25519_key
 EOF
 
+	if [ -n "$major" ] && [ -n "$minor" ]; then
+		if [ "$major" -gt 9 ] || [ "$major" -eq 9 ] && [ "$minor" -ge 8 ]; then
+			cat << EOF >> sshd_config
+PerSourcePenalties no
+EOF
+		fi
+	fi
 	ssh-keygen -q -N "" -t rsa -b 4096 -f $TST_TMPDIR/ssh_host_rsa_key
 	ssh-keygen -q -N "" -t ecdsa -f $TST_TMPDIR/ssh_host_ecdsa_key
 	ssh-keygen -q -N "" -t ed25519 -f $TST_TMPDIR/ssh_host_ed25519_key
-- 
2.43.0


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v2 1/1] ssh-stress: disable resource penalties

[Petr Vorel]

Hi Vasileios,

> Our tests create a number of ssh sessions in the
> background which are immediately killed. Some of
> them haven't finished the authentication stage yet
> and they close the connection incurring penalties from
> the ssh daemon.

> debug1: srclimit_penalise: active penalty for ipv4 10.0.0.1/32
> already exists, 16 seconds remaining

> Then when we try to reconnect to the daemon we are bounced
> because of the active penalty which leads to a failed test.

> ssh-stress 1 TINFO: Killing all ssh sessions
> kex_exchange_identification: read: Connection reset by peer
> Connection reset by fd00:1:1:1::2 port 58373
> ssh-stress 1 TFAIL: SSH not reachable

> From the sshd logs we can see

> debug1: srclimit_penalise: active penalty for ipv4 10.0.0.1/32
> already exists, 16 seconds remaining

> This feature was added to OpenSSH 9.8 in the 2024 release in the
> 81c1099d2 commit. Lets disable penalties for the versions that
> support them.

+1 for a nice description!

> Signed-off-by: Vasileios Almpanis <vasileios.almpanis@virtuozzo.com>
> ---
> Changes in v2:
> - Removed unnecessary parenthesis around if statements.
> ---
>  testcases/network/stress/ssh/ssh-stress.sh | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)

> diff --git a/testcases/network/stress/ssh/ssh-stress.sh b/testcases/network/stress/ssh/ssh-stress.sh
> index c27c27a28..d5db24835 100755
> --- a/testcases/network/stress/ssh/ssh-stress.sh
> +++ b/testcases/network/stress/ssh/ssh-stress.sh
> @@ -39,8 +39,12 @@ cleanup()

>  setup()
>  {
> -	local port rc
> +	local port rc version major minor

> +	version=$(sshd -V 2>&1 | sed -nE 's/^.*OpenSSH_([0-9]+)\.([0-9]+).*$/\1 \2/p' | head -n1)
> +	set -- $version
> +	major=$1
> +	minor=$2

>  	port=$(tst_rhost_run -c "tst_get_unused_port ipv${TST_IPVER} stream")

> @@ -60,6 +64,13 @@ HostKey $TST_TMPDIR/ssh_host_ecdsa_key
>  HostKey $TST_TMPDIR/ssh_host_ed25519_key
>  EOF

> +	if [ -n "$major" ] && [ -n "$minor" ]; then
This could be simplified by assigning both to zero.

> +		if [ "$major" -gt 9 ] || [ "$major" -eq 9 ] && [ "$minor" -ge 8 ]; then

This is wrong. It should be as I suggested at v1:

if [ "$major" -gt 9 ] || [ "$major" -eq 9 -a "$minor" -ge 8 ]; then

otherwise it does not work for > 9. Using () in v1 was also correct, just
unnecessary complicated.

If you agree I can merge this:

setup()
{
	local port rc version
	local major=0 minor=0

	version=$(sshd -V 2>&1 | sed -nE 's/^.*OpenSSH_([0-9]+)\.([0-9]+).*$/\1 \2/p' | head -n1)
	set -- $version
	major=$1
	minor=$2
	...
	if [ "$major" -gt 9 ] || [ "$major" -eq 9 -a "$minor" -ge 8 ]; then
		cat << EOF >> sshd_config
PerSourcePenalties no
EOF
	fi

(See full diff from your v2 below.)
---
Why? && and || have equal precedence (unlike C like languages where && has
higher preference):

$ echo 1 || echo 2 && echo 3
1
3
=> In C like evaluation it would be just:
1

If you wonder why, see section "Lists" in man bash(1) [1]:

	A list is a sequence of one or more pipelines separated by one of
	the operators ;, &, &&, or ||, and optionally terminated by one of
	;, &, or <newline>.

	Of these list operators, && and || have equal precedence, followed
	by ; and &, which have equal precedence.

Just to double check this basic functionality works the same across POSIX shell
see section "Short-Circuit List Operators" in man dash(1) [2]:

	“&&” and “||” are AND-OR list operators.  “&&” executes the first
	command, and then executes the second command if and only if the
	exit status of the first command is zero.  “||” is similar, but
	executes the second command if and only if the exit status of the
	first command is nonzero.  “&&” and “||” both have the same
	priority.

Therefore 10.1 version will not be selected in your new code:

$ major=10 minor=1; if [ "$major" -gt 9 ] || [ "$major" -eq 9 ] && [ "$minor" -ge 8 ]; then echo "found"; else echo "not found"; fi
not found

But using just 2x [ ] (the second with "-a") will behave like 
$ major=10 minor=1; if [ "$major" -gt 9 ] || [ "$major" -eq 9 -a "$minor" -ge 8 ]; then echo "foo"; fi
foo

$ major=10 minor=1; if [ "$major" -gt 9 ] || [ "$major" -eq 9 -a "$minor" -ge 8 ]; then echo "found"; else echo "not found"; fi
found

Using () in v1 was correct:
$ major=10 minor=1; if [ "$major" -gt 9 ] || ([ "$major" -eq 9 ] && [ "$minor" -ge 8 ]); then echo "found"; else echo "not found"; fi
found

it just looked me unnecessary complicated, because ( ... ) executes evaluation
in a subshell (see "Grouping Commands Together" in dash(1) or "Compound
Commands" in bash(1)).

[1] https://man7.org/linux/man-pages/man1/bash.1.html
[2] https://man7.org/linux/man-pages/man1/dash.1.html

diff --git testcases/network/stress/ssh/ssh-stress.sh testcases/network/stress/ssh/ssh-stress.sh
index d5db24835d..14a4af8213 100755
--- testcases/network/stress/ssh/ssh-stress.sh
+++ testcases/network/stress/ssh/ssh-stress.sh
@@ -39,7 +39,8 @@ cleanup()
 
 setup()
 {
-	local port rc version major minor
+	local port rc version
+	local major=0 minor=0
 
 	version=$(sshd -V 2>&1 | sed -nE 's/^.*OpenSSH_([0-9]+)\.([0-9]+).*$/\1 \2/p' | head -n1)
 	set -- $version
@@ -64,12 +65,10 @@ HostKey $TST_TMPDIR/ssh_host_ecdsa_key
 HostKey $TST_TMPDIR/ssh_host_ed25519_key
 EOF
 
-	if [ -n "$major" ] && [ -n "$minor" ]; then
-		if [ "$major" -gt 9 ] || [ "$major" -eq 9 ] && [ "$minor" -ge 8 ]; then
-			cat << EOF >> sshd_config
+	if [ "$major" -gt 9 ] || [ "$major" -eq 9 -a "$minor" -ge 8 ]; then
+		cat << EOF >> sshd_config
 PerSourcePenalties no
 EOF
-		fi
 	fi
 	ssh-keygen -q -N "" -t rsa -b 4096 -f $TST_TMPDIR/ssh_host_rsa_key
 	ssh-keygen -q -N "" -t ecdsa -f $TST_TMPDIR/ssh_host_ecdsa_key

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v2 1/1] ssh-stress: disable resource penalties

[Petr Vorel]

> Hi Petr,

> Agreed, thanks for the detailed explanation.
> You’re absolutely right about && / || precedence in POSIX shells, and the
> 10.x case is indeed broken in my v2.
> The proposed version looks correct and cleaner to me. Please go ahead and
> merge it.

Thanks for your ack, merged with fixes!

Kind regards,
Petr

> Kind regard,
> Vasileios Almpanis

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp