netdev - Re: [PATCH net] selftests: net: avoid waiting for server in amt.sh forever when it fails.

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAMArcTV6zPVNjay=ka3NVEUq2iEZ_W2dy8V6UJkT9Mf2PRKA0A@mail.gmail.com>
Date: Fri, 10 May 2024 13:57:49 +0900
From: Taehee Yoo <ap420073@...il.com>
To: Simon Horman <horms@...nel.org>
Cc: davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org, 
	pabeni@...hat.com, shuah@...nel.org, netdev@...r.kernel.org, 
	linux-kselftest@...r.kernel.org
Subject: Re: [PATCH net] selftests: net: avoid waiting for server in amt.sh
 forever when it fails.

On Thu, May 9, 2024 at 5:38 PM Simon Horman <horms@...nel.org> wrote:
>

Hi Simon,
Thanks a lot for the review!

> On Wed, May 08, 2024 at 04:06:43AM +0000, Taehee Yoo wrote:
> > In the forwarding testcase, it opens a server and a client with the nc.
> > The server receives the correct message from NC, it prints OK.
> > The server prints FAIL if it receives the wrong message from the client.
> >
> > But If the server can't receive any message, it will not close so
> > the amt.sh waits forever.
> > There are several reasons.
> > 1. crash of smcrouted.
> > 2. Send a message from the client to the server before the server is up.
> >
> > To avoid this problem, the server waits only for 10 seconds.
> > The client sends messages for 10 seconds.
> > If the server is successfully closed, it kills the client.
> >
> > Fixes: c08e8baea78e ("selftests: add amt interface selftest script")
> > Signed-off-by: Taehee Yoo <ap420073@...il.com>
> > ---
> >  tools/testing/selftests/net/amt.sh | 63 +++++++++++++++++++-----------
> >  1 file changed, 40 insertions(+), 23 deletions(-)
> >
> > diff --git a/tools/testing/selftests/net/amt.sh b/tools/testing/selftests/net/amt.sh
> > index 75528788cb95..16641d3dccce 100755
> > --- a/tools/testing/selftests/net/amt.sh
> > +++ b/tools/testing/selftests/net/amt.sh
> > @@ -77,6 +77,7 @@ readonly LISTENER=$(mktemp -u listener-XXXXXXXX)
> >  readonly GATEWAY=$(mktemp -u gateway-XXXXXXXX)
> >  readonly RELAY=$(mktemp -u relay-XXXXXXXX)
> >  readonly SOURCE=$(mktemp -u source-XXXXXXXX)
> > +readonly RESULT=$(mktemp -p /tmp amt-XXXXXXXX)
> >  ERR=4
> >  err=0
> >
> > @@ -85,6 +86,10 @@ exit_cleanup()
> >       for ns in "$@"; do
> >               ip netns delete "${ns}" 2>/dev/null || true
> >       done
> > +     rm $RESULT
> > +     smcpid=$(< $SMCROUTEDIR/amt.pid)
> > +     kill $smcpid
> > +     rm -rf $SMCROUTEDIR
>
> Hi Taehee Yoo,
>
> I think this cleanup may be executed before SMCROUTEDIR exists.
>
> For consistency with other temp files, perhaps
> perpahps it is best to move the creation of SMCROUTEDIR up
> to where RESULT is instantiated above.
>
> And perhaps the pid handling can be made conditional on the
> existence of $SMCROUTEDIR/amt.pid
>
>         if [ -f "$SMCROUTEDIR/amt.pid" ]; then
>                 ...
>         fi
>

Thanks!
I will check a pid file before kills smcrouted.

> >
> >       exit $ERR
> >  }
> > @@ -167,7 +172,9 @@ setup_iptables()
> >
> >  setup_mcast_routing()
> >  {
> > -     ip netns exec "${RELAY}" smcrouted
> > +     SMCROUTEDIR="$(mktemp -d)"
> > +
> > +     ip netns exec "${RELAY}" smcrouted -P $SMCROUTEDIR/amt.pid
> >       ip netns exec "${RELAY}" smcroutectl a relay_src \
> >               172.17.0.2 239.0.0.1 amtr
> >       ip netns exec "${RELAY}" smcroutectl a relay_src \
> > @@ -210,40 +217,52 @@ check_features()
> >
> >  test_ipv4_forward()
> >  {
> > -     RESULT4=$(ip netns exec "${LISTENER}" nc -w 1 -l -u 239.0.0.1 4000)
> > +     echo "" > $RESULT
> > +     bash -c "$(ip netns exec "${LISTENER}" \
> > +             timeout 10s nc -w 1 -l -u 239.0.0.1 4000 > $RESULT)"
>
> Hi,
>
> It's unclear to me what the purpose of the bash -c "$(...)" construction is
> here. Can the same be achieved using simply:
>
>         ip netns exec "${LISTENER}" \
>                 timeout 10s nc -w 1 -l -u 239.0.0.1 4000 > $RESULT
>

The purpose of using bash -s was to avoid exiting main bash program
by timeout expiration due to 'set -e' option.
But Jakub avoided that problem by adding (|| true) in the recent patch.


> Also, not strictly related to this patch, it seems a little odd here, and
> elsewhere, to call bash in a /bin/sh script.
>

Oh Thanks,
Shebang should be bash, not sh.
I will fix it.


> > +     RESULT4=$(< $RESULT)
> >       if [ "$RESULT4" == "172.17.0.2" ]; then
> >               printf "TEST: %-60s  [ OK ]\n" "IPv4 amt multicast forwarding"
> > -             exit 0
> >       else
> >               printf "TEST: %-60s  [FAIL]\n" "IPv4 amt multicast forwarding"
> > -             exit 1
> >       fi
> > +
> >  }
>
> ...
>
> >  send_mcast4()
> >  {
> >       sleep 2
> > -     ip netns exec "${SOURCE}" bash -c \
> > -             'echo 172.17.0.2 | nc -w 1 -u 239.0.0.1 4000' &
> > +     for n in {0..10}; do
> > +             ip netns exec "${SOURCE}" bash -c \
> > +                     'echo 172.17.0.2 | nc -w 1 -u 239.0.0.1 4000'
> > +             sleep 1
> > +     done
> > +
> >  }
> >
> >  send_mcast6()
> >  {
> >       sleep 2
> > -     ip netns exec "${SOURCE}" bash -c \
> > -             'echo 2001:db8:3::2 | nc -w 1 -u ff0e::5:6 6000' &
> > +     for n in {0..10}; do
> > +             ip netns exec "${SOURCE}" bash -c \
> > +                     'echo 2001:db8:3::2 | nc -w 1 -u ff0e::5:6 6000'
> > +             sleep 1
> > +     done
> > +
> >  }
> >
> >  check_features
>
> ...
>
> --
> pw-bot: under-review

Thanks a lot!
Taehee Yoo