Message-ID: <61651ca5cbf46_2215c2088b@john-XPS-13-9370.notmuch>
Date:   Mon, 11 Oct 2021 22:27:01 -0700
From:   John Fastabend <john.fastabend@...il.com>
To:     "liujian (CE)" <liujian56@...wei.com>,
        John Fastabend <john.fastabend@...il.com>,
        "daniel@...earbox.net" <daniel@...earbox.net>,
        "jakub@...udflare.com" <jakub@...udflare.com>,
        "lmb@...udflare.com" <lmb@...udflare.com>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "kuba@...nel.org" <kuba@...nel.org>,
        "ast@...nel.org" <ast@...nel.org>,
        "andrii@...nel.org" <andrii@...nel.org>,
        "kafai@...com" <kafai@...com>,
        "songliubraving@...com" <songliubraving@...com>,
        "yhs@...com" <yhs@...com>,
        "kpsingh@...nel.org" <kpsingh@...nel.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "bpf@...r.kernel.org" <bpf@...r.kernel.org>,
        "xiyou.wangcong@...il.com" <xiyou.wangcong@...il.com>
Subject: RE: [PATCH v4] skmsg: lose offset info in sk_psock_skb_ingress

[...]

> > > Thanks. Please add Fixes tags so we can track these I've added it here.
> > >
> > > This has been broken from the initial patches and after a quick glance
> > > I suspect this will need manual backports if we need it. Also all the
> > > I use and all the selftests set parser to a nop by returning skb->len.
> > >
> > > Can you also create a test so we can ensure we don't break this again?
> > Okay, I will do this after the holiday.
> 
> 
> Hi John, 
> I checked selftests, there are have one test case named " test_txmsg_ingress_parser".
> But with this patch and ktls, the test failed, this because ktls parser(tls_read_size) return value is 285 not 256.
> the case like this: 
> tls_sk1 --> redir_sk --> tls_sk2
> tls_sk1 sent out 512 bytes data, after tls related processing redir_sk recved 570 btyes data,
> and redirect 512 (skb_use_parser) bytes data to tls_sk2; but tls_sk2 needs 285 * 2 bytes data, receive timeout occurred.
> I fix this as below:

Ah good catch.

> --- a/tools/testing/selftests/bpf/test_sockmap.c
> +++ b/tools/testing/selftests/bpf/test_sockmap.c
> @@ -1680,6 +1680,8 @@ static void test_txmsg_ingress_parser(int cgrp, struct sockmap_options *opt)
>  {
>         txmsg_pass = 1;
>         skb_use_parser = 512;
> +       if (ktls == 1)
> +               skb_use_parser = 570;
>         opt->iov_length = 256;
>         opt->iov_count = 1;
>         opt->rate = 2;
> 
> 
> And i add one new test as below, is it ok?


Yes looks good to me.

> 
> --- a/tools/testing/selftests/bpf/test_sockmap.c
> +++ b/tools/testing/selftests/bpf/test_sockmap.c
> @@ -139,6 +139,7 @@ struct sockmap_options {
>         bool sendpage;
>         bool data_test;
>         bool drop_expected;
> +       bool check_recved_len;
>         int iov_count;
>         int iov_length;
>         int rate;
> @@ -556,8 +557,12 @@ static int msg_loop(int fd, int iov_count, int iov_length, int cnt,
>         int err, i, flags = MSG_NOSIGNAL;
>         bool drop = opt->drop_expected;
>         bool data = opt->data_test;
> +       int iov_alloc_length = iov_length;
>  
> -       err = msg_alloc_iov(&msg, iov_count, iov_length, data, tx);
> +       if (!tx && opt->check_recved_len)
> +               iov_alloc_length *= 2;
> +
> +       err = msg_alloc_iov(&msg, iov_count, iov_alloc_length, data, tx);
>         if (err)
>                 goto out_errno;
>         if (peek_flag) {
> @@ -665,6 +670,13 @@ static int msg_loop(int fd, int iov_count, int iov_length, int cnt,
>  
>                         s->bytes_recvd += recv;
>  
> +                       if (opt->check_recved_len && s->bytes_recvd > total_bytes) {
> +                               errno = EMSGSIZE;
> +                               fprintf(stderr, "recv failed(), bytes_recvd:%zd, total_bytes:%f\n",
> +                                               s->bytes_recvd, total_bytes);
> +                               goto out_errno;
> +                       }
> +
>                         if (data) {
>                                 int chunk_sz = opt->sendpage ?
>                                                 iov_length * cnt :
> @@ -744,7 +756,8 @@ static int sendmsg_test(struct sockmap_options *opt)
>  
>         rxpid = fork();
>         if (rxpid == 0) {
> -               iov_buf -= (txmsg_pop - txmsg_start_pop + 1);
> +               if (txmsg_pop || txmsg_start_pop)
> +                       iov_buf -= (txmsg_pop - txmsg_start_pop + 1);
>                 if (opt->drop_expected || txmsg_ktls_skb_drop)
>                         _exit(0);
>  
> @@ -1688,6 +1701,19 @@ static void test_txmsg_ingress_parser(int cgrp, struct sockmap_options *opt)
>         test_exec(cgrp, opt);
>  }
>  
> +static void test_txmsg_ingress_parser2(int cgrp, struct sockmap_options *opt)
> +{
> +       if (ktls == 1)
> +               return;
> +       skb_use_parser = 10;
> +       opt->iov_length = 20;
> +       opt->iov_count = 1;
> +       opt->rate = 1;
> +       opt->check_recved_len = true;
> +       test_exec(cgrp, opt);
> +       opt->check_recved_len = false;
> +}
> +
>  char *map_names[] = {
>         "sock_map",
>         "sock_map_txmsg",
> @@ -1786,7 +1812,8 @@ struct _test test[] = {
>         {"txmsg test pull-data", test_txmsg_pull},
>         {"txmsg test pop-data", test_txmsg_pop},
>         {"txmsg test push/pop data", test_txmsg_push_pop},
> -       {"txmsg text ingress parser", test_txmsg_ingress_parser},
> +       {"txmsg test ingress parser", test_txmsg_ingress_parser},
> +       {"txmsg test ingress parser2", test_txmsg_ingress_parser2},
>  };
> 

Great, please post as a series.

Powered by blists - more mailing lists