netdev - Re: [PATCH net-next 02/13] selftests: ncdevmem: Remove validation

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: <ZuNjz7HvlD4z6dR8@mini-arch>
Date: Thu, 12 Sep 2024 14:57:35 -0700
From: Stanislav Fomichev <stfomichev@...il.com>
To: Mina Almasry <almasrymina@...gle.com>
Cc: Stanislav Fomichev <sdf@...ichev.me>, netdev@...r.kernel.org,
	davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
	pabeni@...hat.com
Subject: Re: [PATCH net-next 02/13] selftests: ncdevmem: Remove validation

On 09/12, Mina Almasry wrote:
> On Thu, Sep 12, 2024 at 10:12 AM Stanislav Fomichev <sdf@...ichev.me> wrote:
> >
> > ncdevmem should (see next patches) print the payload on the stdout.
> > The validation can and should be done by the callers:
> >
> > $ ncdevmem -l ... > file
> > $ sha256sum file
> >
> > Cc: Mina Almasry <almasrymina@...gle.com>
> > Signed-off-by: Stanislav Fomichev <sdf@...ichev.me>
> > ---
> >  tools/testing/selftests/net/ncdevmem.c | 56 +++-----------------------
> >  1 file changed, 6 insertions(+), 50 deletions(-)
> >
> > diff --git a/tools/testing/selftests/net/ncdevmem.c b/tools/testing/selftests/net/ncdevmem.c
> > index 352dba211fb0..3712296d997b 100644
> > --- a/tools/testing/selftests/net/ncdevmem.c
> > +++ b/tools/testing/selftests/net/ncdevmem.c
> > @@ -64,24 +64,13 @@
> >  static char *server_ip = "192.168.1.4";
> >  static char *client_ip = "192.168.1.2";
> >  static char *port = "5201";
> > -static size_t do_validation;
> >  static int start_queue = 8;
> >  static int num_queues = 8;
> >  static char *ifname = "eth1";
> >  static unsigned int ifindex;
> >  static unsigned int dmabuf_id;
> >
> > -void print_bytes(void *ptr, size_t size)
> > -{
> > -       unsigned char *p = ptr;
> > -       int i;
> > -
> > -       for (i = 0; i < size; i++)
> > -               printf("%02hhX ", p[i]);
> > -       printf("\n");
> > -}
> > -
> > -void print_nonzero_bytes(void *ptr, size_t size)
> > +static void print_nonzero_bytes(void *ptr, size_t size)
> >  {
> >         unsigned char *p = ptr;
> >         unsigned int i;
> > @@ -91,30 +80,6 @@ void print_nonzero_bytes(void *ptr, size_t size)
> >         printf("\n");
> >  }
> >
> > -void validate_buffer(void *line, size_t size)
> > -{
> > -       static unsigned char seed = 1;
> > -       unsigned char *ptr = line;
> > -       int errors = 0;
> > -       size_t i;
> > -
> > -       for (i = 0; i < size; i++) {
> > -               if (ptr[i] != seed) {
> > -                       fprintf(stderr,
> > -                               "Failed validation: expected=%u, actual=%u, index=%lu\n",
> > -                               seed, ptr[i], i);
> > -                       errors++;
> 
> FWIW the index at where the validation started to fail often gives
> critical clues about where the bug is, along with this line, which I'm
> glad is not removed:
> 
> printf("received frag_page=%llu, in_page_offset=%llu,
> frag_offset=%llu, frag_size=%u, token=%u, total_received=%lu,
> dmabuf_id=%u\n",
> 
> I think we can ensure that what is doing the validation above ncdevmem
> prints enough context about the error. Although, just to understand
> your thinking a bit, why not have this binary do the validation
> itself?

Right, the debugging stuff will still be there to track the offending
part. And the caller can print out the idx of data where the validation
failed.

The reason I removed it from the tool is to be able to do the validation
with regular nc on the other side. I just do:

- echo "expected payload" | nc ..
- ncdevmem -s .. > expected_payload.txt
- [ "expected payload" != $(cat expected_payload.txt) ] && fail

If I were to use the validation on the ncdevmem side, I'd need to bother
with generating the payload that it expects which seems like
an unnecessary complication? For the (to be posted) txrx test I basically
do the following (sha256sum to validate a bunch of random bytes):

def check_txrx(cfg) -> None:
    cfg.require_v6()
    require_devmem(cfg)

    cmd(f"cat /dev/urandom | tr -dc '[:print:]' | head -c 1M > random_file.txt", host=cfg.remote, shell=True)
    want_sha = cmd(f"sha256sum random_file.txt", host=cfg.remote, shell=True).stdout.strip()

    port = rand_port()
    listen_cmd = f"./ncdevmem -l -f {cfg.ifname} -s {cfg.v6} -p {port} | tee random_file.txt | sha256sum -"

    pwd = cmd(f"pwd").stdout.strip()
    with bkg(listen_cmd, exit_wait=True) as nc:
        wait_port_listen(port)
        cmd(f"cat random_file.txt | {pwd}/ncdevmem -f {cfg.ifname} -s {cfg.v6} -p {port}", host=cfg.remote, shell=True)

    ksft_eq(nc.stdout.strip().split(" ")[0], want_sha.split(" ")[0])