Hi all,
Today's linux-next merge of the net-next tree got a conflict in:
tools/testing/selftests/net/tls.c
between commit:
50c6b58a814d ("tls: fix currently broken MSG_PEEK behavior")
from the net tree and commit:
c2ad647c6442 ("selftests/tls: Add test for recv(PEEK) spanning across multiple records")
from the net-next tree.
I fixed it up (see below) and can carry the fix as necessary. This
is now fixed as far as linux-next is concerned, but any non trivial
conflicts should be mentioned to your upstream maintainer when your tree
is submitted for merging. You may also want to consider cooperating
with the maintainer of the conflicting tree to minimise any particularly
complex conflicts.
--
Cheers,
Stephen Rothwell
diff --cc tools/testing/selftests/net/tls.c
index 8fdfeafaf8c0,96fc6fe70293..000000000000
--- a/tools/testing/selftests/net/tls.c
+++ b/tools/testing/selftests/net/tls.c
@@@ -502,55 -502,28 +502,77 @@@ TEST_F(tls, recv_peek_multiple
EXPECT_EQ(memcmp(test_str, buf, send_len), 0);
}
+TEST_F(tls, recv_peek_multiple_records)
+{
+ char const *test_str = "test_read_peek_mult_recs";
+ char const *test_str_first = "test_read_peek";
+ char const *test_str_second = "_mult_recs";
+ int len;
+ char buf[64];
+
+ len = strlen(test_str_first);
+ EXPECT_EQ(send(self->fd, test_str_first, len, 0), len);
+
+ len = strlen(test_str_second) + 1;
+ EXPECT_EQ(send(self->fd, test_str_second, len, 0), len);
+
+ len = sizeof(buf);
+ memset(buf, 0, len);
+ EXPECT_NE(recv(self->cfd, buf, len, MSG_PEEK), -1);
+
+ /* MSG_PEEK can only peek into the current record. */
+ len = strlen(test_str_first) + 1;
+ EXPECT_EQ(memcmp(test_str_first, buf, len), 0);
+
+ len = sizeof(buf);
+ memset(buf, 0, len);
+ EXPECT_NE(recv(self->cfd, buf, len, 0), -1);
+
+ /* Non-MSG_PEEK will advance strparser (and therefore record)
+ * however.
+ */
+ len = strlen(test_str) + 1;
+ EXPECT_EQ(memcmp(test_str, buf, len), 0);
+
+ /* MSG_MORE will hold current record open, so later MSG_PEEK
+ * will see everything.
+ */
+ len = strlen(test_str_first);
+ EXPECT_EQ(send(self->fd, test_str_first, len, MSG_MORE), len);
+
+ len = strlen(test_str_second) + 1;
+ EXPECT_EQ(send(self->fd, test_str_second, len, 0), len);
+
+ len = sizeof(buf);
+ memset(buf, 0, len);
+ EXPECT_NE(recv(self->cfd, buf, len, MSG_PEEK), -1);
+
+ len = strlen(test_str) + 1;
+ EXPECT_EQ(memcmp(test_str, buf, len), 0);
+}
+
+ TEST_F(tls, recv_peek_large_buf_mult_recs)
+ {
+ char const *test_str = "test_read_peek_mult_recs";
+ char const *test_str_first = "test_read_peek";
+ char const *test_str_second = "_mult_recs";
+ int len;
+ char buf[64];
+
+ len = strlen(test_str_first);
+ EXPECT_EQ(send(self->fd, test_str_first, len, 0), len);
+
+ len = strlen(test_str_second) + 1;
+ EXPECT_EQ(send(self->fd, test_str_second, len, 0), len);
+
+ len = sizeof(buf);
+ memset(buf, 0, len);
+ EXPECT_NE(recv(self->cfd, buf, len, MSG_PEEK), -1);
+
+ len = strlen(test_str) + 1;
+ EXPECT_EQ(memcmp(test_str, buf, len), 0);
+ }
+
TEST_F(tls, pollin)
{
char const *test_str = "test_poll";
On 09/18/2018 02:11 AM, Stephen Rothwell wrote:
> Hi all,
>
> Today's linux-next merge of the net-next tree got a conflict in:
>
> tools/testing/selftests/net/tls.c
>
> between commit:
>
> 50c6b58a814d ("tls: fix currently broken MSG_PEEK behavior")
>
> from the net tree and commit:
>
> c2ad647c6442 ("selftests/tls: Add test for recv(PEEK) spanning across multiple records")
>
> from the net-next tree.
>
> I fixed it up (see below) and can carry the fix as necessary. This
> is now fixed as far as linux-next is concerned, but any non trivial
> conflicts should be mentioned to your upstream maintainer when your tree
> is submitted for merging. You may also want to consider cooperating
> with the maintainer of the conflicting tree to minimise any particularly
> complex conflicts.
The test from 50c6b58a814d supersedes the one from c2ad647c6442 so the
recv_peek_large_buf_mult_recs could be removed; latter was also not working
correctly due to this bug.
Thanks,
Daniel
> -----Original Message-----
> From: Daniel Borkmann <[email protected]>
> Sent: Tuesday, September 18, 2018 2:14 PM
> To: Stephen Rothwell <[email protected]>; David Miller
> <[email protected]>; Networking <[email protected]>
> Cc: Linux-Next Mailing List <[email protected]>; Linux Kernel
> Mailing List <[email protected]>; Vakul Garg
> <[email protected]>
> Subject: Re: linux-next: manual merge of the net-next tree with the net tree
>
> On 09/18/2018 02:11 AM, Stephen Rothwell wrote:
> > Hi all,
> >
> > Today's linux-next merge of the net-next tree got a conflict in:
> >
> > tools/testing/selftests/net/tls.c
> >
> > between commit:
> >
> > 50c6b58a814d ("tls: fix currently broken MSG_PEEK behavior")
> >
> > from the net tree and commit:
> >
> > c2ad647c6442 ("selftests/tls: Add test for recv(PEEK) spanning
> > across multiple records")
> >
> > from the net-next tree.
> >
> > I fixed it up (see below) and can carry the fix as necessary. This is
> > now fixed as far as linux-next is concerned, but any non trivial
> > conflicts should be mentioned to your upstream maintainer when your
> > tree is submitted for merging. You may also want to consider
> > cooperating with the maintainer of the conflicting tree to minimise
> > any particularly complex conflicts.
>
> The test from 50c6b58a814d supersedes the one from c2ad647c6442 so the
> recv_peek_large_buf_mult_recs could be removed; latter was also not
> working correctly due to this bug.
Why remove recv_peek_large_buf_mult_recs if its correct?
Why not the newly added one which achieves the same thing?
Regards, Vakul
>
> Thanks,
> Daniel
On 09/18/2018 11:10 AM, Vakul Garg wrote:
>> -----Original Message-----
>> From: Daniel Borkmann <[email protected]>
>> Sent: Tuesday, September 18, 2018 2:14 PM
>> To: Stephen Rothwell <[email protected]>; David Miller
>> <[email protected]>; Networking <[email protected]>
>> Cc: Linux-Next Mailing List <[email protected]>; Linux Kernel
>> Mailing List <[email protected]>; Vakul Garg
>> <[email protected]>
>> Subject: Re: linux-next: manual merge of the net-next tree with the net tree
>>
>> On 09/18/2018 02:11 AM, Stephen Rothwell wrote:
>>> Hi all,
>>>
>>> Today's linux-next merge of the net-next tree got a conflict in:
>>>
>>> tools/testing/selftests/net/tls.c
>>>
>>> between commit:
>>>
>>> 50c6b58a814d ("tls: fix currently broken MSG_PEEK behavior")
>>>
>>> from the net tree and commit:
>>>
>>> c2ad647c6442 ("selftests/tls: Add test for recv(PEEK) spanning
>>> across multiple records")
>>>
>>> from the net-next tree.
>>>
>>> I fixed it up (see below) and can carry the fix as necessary. This is
>>> now fixed as far as linux-next is concerned, but any non trivial
>>> conflicts should be mentioned to your upstream maintainer when your
>>> tree is submitted for merging. You may also want to consider
>>> cooperating with the maintainer of the conflicting tree to minimise
>>> any particularly complex conflicts.
>>
>> The test from 50c6b58a814d supersedes the one from c2ad647c6442 so the
>> recv_peek_large_buf_mult_recs could be removed; latter was also not
>> working correctly due to this bug.
>
> Why remove recv_peek_large_buf_mult_recs if its correct?
> Why not the newly added one which achieves the same thing?
Hmm, not quite, on net-next kernel, the recv_peek_large_buf_mult_recs fails
every time I invoke the tls test suite:
# ./tls
[==========] Running 28 tests from 2 test cases.
[ RUN ] tls.sendfile
[ OK ] tls.sendfile
[ RUN ] tls.send_then_sendfile
[ OK ] tls.send_then_sendfile
[ RUN ] tls.recv_max
[ OK ] tls.recv_max
[ RUN ] tls.recv_small
[ OK ] tls.recv_small
[ RUN ] tls.msg_more
[ OK ] tls.msg_more
[ RUN ] tls.sendmsg_single
[ OK ] tls.sendmsg_single
[ RUN ] tls.sendmsg_large
[ OK ] tls.sendmsg_large
[ RUN ] tls.sendmsg_multiple
[ OK ] tls.sendmsg_multiple
[ RUN ] tls.sendmsg_multiple_stress
[ OK ] tls.sendmsg_multiple_stress
[ RUN ] tls.splice_from_pipe
[ OK ] tls.splice_from_pipe
[ RUN ] tls.splice_from_pipe2
[ OK ] tls.splice_from_pipe2
[ RUN ] tls.send_and_splice
[ OK ] tls.send_and_splice
[ RUN ] tls.splice_to_pipe
[ OK ] tls.splice_to_pipe
[ RUN ] tls.recvmsg_single
[ OK ] tls.recvmsg_single
[ RUN ] tls.recvmsg_single_max
[ OK ] tls.recvmsg_single_max
[ RUN ] tls.recvmsg_multiple
[ OK ] tls.recvmsg_multiple
[ RUN ] tls.single_send_multiple_recv
[ OK ] tls.single_send_multiple_recv
[ RUN ] tls.multiple_send_single_recv
[ OK ] tls.multiple_send_single_recv
[ RUN ] tls.recv_partial
[ OK ] tls.recv_partial
[ RUN ] tls.recv_nonblock
[ OK ] tls.recv_nonblock
[ RUN ] tls.recv_peek
[ OK ] tls.recv_peek
[ RUN ] tls.recv_peek_multiple
[ OK ] tls.recv_peek_multiple
[ RUN ] tls.recv_peek_large_buf_mult_recs
tls.c:524:tls.recv_peek_large_buf_mult_recs:Expected memcmp(test_str, buf, len) (18446744073709551595) == 0 (0)
tls.recv_peek_large_buf_mult_recs: Test failed at step #8
[ FAIL ] tls.recv_peek_large_buf_mult_recs
[ RUN ] tls.pollin
[ OK ] tls.pollin
[ RUN ] tls.poll_wait
[ OK ] tls.poll_wait
[ RUN ] tls.blocking
[ OK ] tls.blocking
[ RUN ] tls.nonblocking
[ OK ] tls.nonblocking
[ RUN ] tls.control_msg
[ OK ] tls.control_msg
[==========] 27 / 28 tests passed.
[ FAILED ]
Here's what the recvfrom() with MSG_PEEK sees:
[pid 2602] socket(AF_INET, SOCK_STREAM, IPPROTO_IP) = 3
[pid 2602] socket(AF_INET, SOCK_STREAM, IPPROTO_IP) = 4
[pid 2602] bind(4, {sa_family=AF_INET, sin_port=htons(0), sin_addr=inet_addr("0.0.0.0")}, 16) = 0
[pid 2602] listen(4, 10) = 0
[pid 2602] getsockname(4, {sa_family=AF_INET, sin_port=htons(41483), sin_addr=inet_addr("0.0.0.0")}, [16]) = 0
[pid 2602] connect(3, {sa_family=AF_INET, sin_port=htons(41483), sin_addr=inet_addr("0.0.0.0")}, 16) = 0
[pid 2602] setsockopt(3, SOL_TCP, 0x1f /* TCP_??? */, [7564404], 4) = 0
[pid 2602] setsockopt(3, 0x11a /* SOL_?? */, 1, "\3\0033\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"..., 40) = 0
[pid 2602] accept(4, {sa_family=AF_INET, sin_port=htons(46290), sin_addr=inet_addr("127.0.0.1")}, [16]) = 5
[pid 2602] setsockopt(5, SOL_TCP, 0x1f /* TCP_??? */, [7564404], 4) = 0
[pid 2602] setsockopt(5, 0x11a /* SOL_?? */, 2, "\3\0033\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"..., 40) = 0
[pid 2602] close(4) = 0
[pid 2602] sendto(3, "test_read_peek", 14, 0, NULL, 0) = 14
[pid 2602] sendto(3, "_mult_recs\0", 11, 0, NULL, 0) = 11
[pid 2602] recvfrom(5, "test_read_peektest_read_peektest"..., 64, MSG_PEEK, NULL, NULL) = 64
[pid 2602] write(2, "tls.c:526:tls.recv_peek_large_bu"..., 112tls.c:526:tls.recv_peek_large_buf_mult_recs:Expected memcmp(test_str, buf, len) (18446744073709551595) == 0 (0)
) = 112
[pid 2602] close(3) = 0
[pid 2602] close(5) = 0
[pid 2602] exit_group(8) = ?
Reason for the "test_read_peektest_read_peektest[...]" is because MSG_PEEK
cannot call tls_sw_advance_skb(), since the skb is sitting there that needs
to be consumed for non-MSG_PEEK case, and only then we can advance it.
Could you elaborate on where you ever had this test succeeding? With nxp
accelerator?
Thanks,
Daniel
> -----Original Message-----
> From: Daniel Borkmann <[email protected]>
> Sent: Tuesday, September 18, 2018 2:57 PM
> To: Vakul Garg <[email protected]>; Stephen Rothwell
> <[email protected]>; David Miller <[email protected]>;
> Networking <[email protected]>
> Cc: Linux-Next Mailing List <[email protected]>; Linux Kernel
> Mailing List <[email protected]>
> Subject: Re: linux-next: manual merge of the net-next tree with the net tree
>
> On 09/18/2018 11:10 AM, Vakul Garg wrote:
> >> -----Original Message-----
> >> From: Daniel Borkmann <[email protected]>
> >> Sent: Tuesday, September 18, 2018 2:14 PM
> >> To: Stephen Rothwell <[email protected]>; David Miller
> >> <[email protected]>; Networking <[email protected]>
> >> Cc: Linux-Next Mailing List <[email protected]>; Linux
> >> Kernel Mailing List <[email protected]>; Vakul Garg
> >> <[email protected]>
> >> Subject: Re: linux-next: manual merge of the net-next tree with the
> >> net tree
> >>
> >> On 09/18/2018 02:11 AM, Stephen Rothwell wrote:
> >>> Hi all,
> >>>
> >>> Today's linux-next merge of the net-next tree got a conflict in:
> >>>
> >>> tools/testing/selftests/net/tls.c
> >>>
> >>> between commit:
> >>>
> >>> 50c6b58a814d ("tls: fix currently broken MSG_PEEK behavior")
> >>>
> >>> from the net tree and commit:
> >>>
> >>> c2ad647c6442 ("selftests/tls: Add test for recv(PEEK) spanning
> >>> across multiple records")
> >>>
> >>> from the net-next tree.
> >>>
> >>> I fixed it up (see below) and can carry the fix as necessary. This
> >>> is now fixed as far as linux-next is concerned, but any non trivial
> >>> conflicts should be mentioned to your upstream maintainer when your
> >>> tree is submitted for merging. You may also want to consider
> >>> cooperating with the maintainer of the conflicting tree to minimise
> >>> any particularly complex conflicts.
> >>
> >> The test from 50c6b58a814d supersedes the one from c2ad647c6442 so
> >> the recv_peek_large_buf_mult_recs could be removed; latter was also
> >> not working correctly due to this bug.
> >
> > Why remove recv_peek_large_buf_mult_recs if its correct?
> > Why not the newly added one which achieves the same thing?
>
> Hmm, not quite, on net-next kernel, the recv_peek_large_buf_mult_recs fails
> every time I invoke the tls test suite:
>
> # ./tls
> [==========] Running 28 tests from 2 test cases.
> [ RUN ] tls.sendfile
> [ OK ] tls.sendfile
> [ RUN ] tls.send_then_sendfile
> [ OK ] tls.send_then_sendfile
> [ RUN ] tls.recv_max
> [ OK ] tls.recv_max
> [ RUN ] tls.recv_small
> [ OK ] tls.recv_small
> [ RUN ] tls.msg_more
> [ OK ] tls.msg_more
> [ RUN ] tls.sendmsg_single
> [ OK ] tls.sendmsg_single
> [ RUN ] tls.sendmsg_large
> [ OK ] tls.sendmsg_large
> [ RUN ] tls.sendmsg_multiple
> [ OK ] tls.sendmsg_multiple
> [ RUN ] tls.sendmsg_multiple_stress
> [ OK ] tls.sendmsg_multiple_stress
> [ RUN ] tls.splice_from_pipe
> [ OK ] tls.splice_from_pipe
> [ RUN ] tls.splice_from_pipe2
> [ OK ] tls.splice_from_pipe2
> [ RUN ] tls.send_and_splice
> [ OK ] tls.send_and_splice
> [ RUN ] tls.splice_to_pipe
> [ OK ] tls.splice_to_pipe
> [ RUN ] tls.recvmsg_single
> [ OK ] tls.recvmsg_single
> [ RUN ] tls.recvmsg_single_max
> [ OK ] tls.recvmsg_single_max
> [ RUN ] tls.recvmsg_multiple
> [ OK ] tls.recvmsg_multiple
> [ RUN ] tls.single_send_multiple_recv
> [ OK ] tls.single_send_multiple_recv
> [ RUN ] tls.multiple_send_single_recv
> [ OK ] tls.multiple_send_single_recv
> [ RUN ] tls.recv_partial
> [ OK ] tls.recv_partial
> [ RUN ] tls.recv_nonblock
> [ OK ] tls.recv_nonblock
> [ RUN ] tls.recv_peek
> [ OK ] tls.recv_peek
> [ RUN ] tls.recv_peek_multiple
> [ OK ] tls.recv_peek_multiple
> [ RUN ] tls.recv_peek_large_buf_mult_recs
> tls.c:524:tls.recv_peek_large_buf_mult_recs:Expected memcmp(test_str,
> buf, len) (18446744073709551595) == 0 (0)
> tls.recv_peek_large_buf_mult_recs: Test failed at step #8
> [ FAIL ] tls.recv_peek_large_buf_mult_recs
> [ RUN ] tls.pollin
> [ OK ] tls.pollin
> [ RUN ] tls.poll_wait
> [ OK ] tls.poll_wait
> [ RUN ] tls.blocking
> [ OK ] tls.blocking
> [ RUN ] tls.nonblocking
> [ OK ] tls.nonblocking
> [ RUN ] tls.control_msg
> [ OK ] tls.control_msg
> [==========] 27 / 28 tests passed.
> [ FAILED ]
>
> Here's what the recvfrom() with MSG_PEEK sees:
>
> [pid 2602] socket(AF_INET, SOCK_STREAM, IPPROTO_IP) = 3 [pid 2602]
> socket(AF_INET, SOCK_STREAM, IPPROTO_IP) = 4 [pid 2602] bind(4,
> {sa_family=AF_INET, sin_port=htons(0), sin_addr=inet_addr("0.0.0.0")}, 16) =
> 0
> [pid 2602] listen(4, 10) = 0
> [pid 2602] getsockname(4, {sa_family=AF_INET, sin_port=htons(41483),
> sin_addr=inet_addr("0.0.0.0")}, [16]) = 0 [pid 2602] connect(3,
> {sa_family=AF_INET, sin_port=htons(41483), sin_addr=inet_addr("0.0.0.0")},
> 16) = 0 [pid 2602] setsockopt(3, SOL_TCP, 0x1f /* TCP_??? */, [7564404], 4)
> = 0 [pid 2602] setsockopt(3, 0x11a /* SOL_?? */, 1,
> "\3\0033\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"...,
> 40) = 0 [pid 2602] accept(4, {sa_family=AF_INET, sin_port=htons(46290),
> sin_addr=inet_addr("127.0.0.1")}, [16]) = 5 [pid 2602] setsockopt(5,
> SOL_TCP, 0x1f /* TCP_??? */, [7564404], 4) = 0 [pid 2602] setsockopt(5,
> 0x11a /* SOL_?? */, 2,
> "\3\0033\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"...,
> 40) = 0
> [pid 2602] close(4) = 0
> [pid 2602] sendto(3, "test_read_peek", 14, 0, NULL, 0) = 14 [pid 2602]
> sendto(3, "_mult_recs\0", 11, 0, NULL, 0) = 11 [pid 2602] recvfrom(5,
> "test_read_peektest_read_peektest"..., 64, MSG_PEEK, NULL, NULL) = 64
> [pid 2602] write(2, "tls.c:526:tls.recv_peek_large_bu"...,
> 112tls.c:526:tls.recv_peek_large_buf_mult_recs:Expected memcmp(test_str,
> buf, len) (18446744073709551595) == 0 (0)
> ) = 112
> [pid 2602] close(3) = 0
> [pid 2602] close(5) = 0
> [pid 2602] exit_group(8) = ?
>
> Reason for the "test_read_peektest_read_peektest[...]" is because
> MSG_PEEK cannot call tls_sw_advance_skb(), since the skb is sitting there
> that needs to be consumed for non-MSG_PEEK case, and only then we can
> advance it.
>
I general, my plan was to modify the tls_sw_recvmsg() to trigger as many
decryption as possible as required by requested user space PEEK size.
This would have required creating a pending list of decrypted records in tls_tx context.
> Could you elaborate on where you ever had this test succeeding? With nxp
> accelerator?
I never had this test succeeding. I pointed the problem to Dave Watson sometime
back (found during code reading).
To make sure that this bug does not slip out, I simply submitted a test case to keep
reminding ourselves that we need to fix it sometime.
>
> Thanks,
> Daniel
On 09/18/2018 11:32 AM, Vakul Garg wrote:
>> -----Original Message-----
>> From: Daniel Borkmann <[email protected]>
>> Sent: Tuesday, September 18, 2018 2:57 PM
>> To: Vakul Garg <[email protected]>; Stephen Rothwell
>> <[email protected]>; David Miller <[email protected]>;
>> Networking <[email protected]>
>> Cc: Linux-Next Mailing List <[email protected]>; Linux Kernel
>> Mailing List <[email protected]>
>> Subject: Re: linux-next: manual merge of the net-next tree with the net tree
>>
>> On 09/18/2018 11:10 AM, Vakul Garg wrote:
>>>> -----Original Message-----
>>>> From: Daniel Borkmann <[email protected]>
>>>> Sent: Tuesday, September 18, 2018 2:14 PM
>>>> To: Stephen Rothwell <[email protected]>; David Miller
>>>> <[email protected]>; Networking <[email protected]>
>>>> Cc: Linux-Next Mailing List <[email protected]>; Linux
>>>> Kernel Mailing List <[email protected]>; Vakul Garg
>>>> <[email protected]>
>>>> Subject: Re: linux-next: manual merge of the net-next tree with the
>>>> net tree
>>>>
>>>> On 09/18/2018 02:11 AM, Stephen Rothwell wrote:
>>>>> Hi all,
>>>>>
>>>>> Today's linux-next merge of the net-next tree got a conflict in:
>>>>>
>>>>> tools/testing/selftests/net/tls.c
>>>>>
>>>>> between commit:
>>>>>
>>>>> 50c6b58a814d ("tls: fix currently broken MSG_PEEK behavior")
>>>>>
>>>>> from the net tree and commit:
>>>>>
>>>>> c2ad647c6442 ("selftests/tls: Add test for recv(PEEK) spanning
>>>>> across multiple records")
>>>>>
>>>>> from the net-next tree.
>>>>>
>>>>> I fixed it up (see below) and can carry the fix as necessary. This
>>>>> is now fixed as far as linux-next is concerned, but any non trivial
>>>>> conflicts should be mentioned to your upstream maintainer when your
>>>>> tree is submitted for merging. You may also want to consider
>>>>> cooperating with the maintainer of the conflicting tree to minimise
>>>>> any particularly complex conflicts.
>>>>
>>>> The test from 50c6b58a814d supersedes the one from c2ad647c6442 so
>>>> the recv_peek_large_buf_mult_recs could be removed; latter was also
>>>> not working correctly due to this bug.
>>>
>>> Why remove recv_peek_large_buf_mult_recs if its correct?
>>> Why not the newly added one which achieves the same thing?
>>
>> Hmm, not quite, on net-next kernel, the recv_peek_large_buf_mult_recs fails
>> every time I invoke the tls test suite:
>>
>> # ./tls
>> [==========] Running 28 tests from 2 test cases.
>> [ RUN ] tls.sendfile
>> [ OK ] tls.sendfile
>> [ RUN ] tls.send_then_sendfile
>> [ OK ] tls.send_then_sendfile
>> [ RUN ] tls.recv_max
>> [ OK ] tls.recv_max
>> [ RUN ] tls.recv_small
>> [ OK ] tls.recv_small
>> [ RUN ] tls.msg_more
>> [ OK ] tls.msg_more
>> [ RUN ] tls.sendmsg_single
>> [ OK ] tls.sendmsg_single
>> [ RUN ] tls.sendmsg_large
>> [ OK ] tls.sendmsg_large
>> [ RUN ] tls.sendmsg_multiple
>> [ OK ] tls.sendmsg_multiple
>> [ RUN ] tls.sendmsg_multiple_stress
>> [ OK ] tls.sendmsg_multiple_stress
>> [ RUN ] tls.splice_from_pipe
>> [ OK ] tls.splice_from_pipe
>> [ RUN ] tls.splice_from_pipe2
>> [ OK ] tls.splice_from_pipe2
>> [ RUN ] tls.send_and_splice
>> [ OK ] tls.send_and_splice
>> [ RUN ] tls.splice_to_pipe
>> [ OK ] tls.splice_to_pipe
>> [ RUN ] tls.recvmsg_single
>> [ OK ] tls.recvmsg_single
>> [ RUN ] tls.recvmsg_single_max
>> [ OK ] tls.recvmsg_single_max
>> [ RUN ] tls.recvmsg_multiple
>> [ OK ] tls.recvmsg_multiple
>> [ RUN ] tls.single_send_multiple_recv
>> [ OK ] tls.single_send_multiple_recv
>> [ RUN ] tls.multiple_send_single_recv
>> [ OK ] tls.multiple_send_single_recv
>> [ RUN ] tls.recv_partial
>> [ OK ] tls.recv_partial
>> [ RUN ] tls.recv_nonblock
>> [ OK ] tls.recv_nonblock
>> [ RUN ] tls.recv_peek
>> [ OK ] tls.recv_peek
>> [ RUN ] tls.recv_peek_multiple
>> [ OK ] tls.recv_peek_multiple
>> [ RUN ] tls.recv_peek_large_buf_mult_recs
>> tls.c:524:tls.recv_peek_large_buf_mult_recs:Expected memcmp(test_str,
>> buf, len) (18446744073709551595) == 0 (0)
>> tls.recv_peek_large_buf_mult_recs: Test failed at step #8
>> [ FAIL ] tls.recv_peek_large_buf_mult_recs
>> [ RUN ] tls.pollin
>> [ OK ] tls.pollin
>> [ RUN ] tls.poll_wait
>> [ OK ] tls.poll_wait
>> [ RUN ] tls.blocking
>> [ OK ] tls.blocking
>> [ RUN ] tls.nonblocking
>> [ OK ] tls.nonblocking
>> [ RUN ] tls.control_msg
>> [ OK ] tls.control_msg
>> [==========] 27 / 28 tests passed.
>> [ FAILED ]
>>
>> Here's what the recvfrom() with MSG_PEEK sees:
>>
>> [pid 2602] socket(AF_INET, SOCK_STREAM, IPPROTO_IP) = 3 [pid 2602]
>> socket(AF_INET, SOCK_STREAM, IPPROTO_IP) = 4 [pid 2602] bind(4,
>> {sa_family=AF_INET, sin_port=htons(0), sin_addr=inet_addr("0.0.0.0")}, 16) =
>> 0
>> [pid 2602] listen(4, 10) = 0
>> [pid 2602] getsockname(4, {sa_family=AF_INET, sin_port=htons(41483),
>> sin_addr=inet_addr("0.0.0.0")}, [16]) = 0 [pid 2602] connect(3,
>> {sa_family=AF_INET, sin_port=htons(41483), sin_addr=inet_addr("0.0.0.0")},
>> 16) = 0 [pid 2602] setsockopt(3, SOL_TCP, 0x1f /* TCP_??? */, [7564404], 4)
>> = 0 [pid 2602] setsockopt(3, 0x11a /* SOL_?? */, 1,
>> "\3\0033\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"...,
>> 40) = 0 [pid 2602] accept(4, {sa_family=AF_INET, sin_port=htons(46290),
>> sin_addr=inet_addr("127.0.0.1")}, [16]) = 5 [pid 2602] setsockopt(5,
>> SOL_TCP, 0x1f /* TCP_??? */, [7564404], 4) = 0 [pid 2602] setsockopt(5,
>> 0x11a /* SOL_?? */, 2,
>> "\3\0033\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"...,
>> 40) = 0
>> [pid 2602] close(4) = 0
>> [pid 2602] sendto(3, "test_read_peek", 14, 0, NULL, 0) = 14 [pid 2602]
>> sendto(3, "_mult_recs\0", 11, 0, NULL, 0) = 11 [pid 2602] recvfrom(5,
>> "test_read_peektest_read_peektest"..., 64, MSG_PEEK, NULL, NULL) = 64
>> [pid 2602] write(2, "tls.c:526:tls.recv_peek_large_bu"...,
>> 112tls.c:526:tls.recv_peek_large_buf_mult_recs:Expected memcmp(test_str,
>> buf, len) (18446744073709551595) == 0 (0)
>> ) = 112
>> [pid 2602] close(3) = 0
>> [pid 2602] close(5) = 0
>> [pid 2602] exit_group(8) = ?
>>
>> Reason for the "test_read_peektest_read_peektest[...]" is because
>> MSG_PEEK cannot call tls_sw_advance_skb(), since the skb is sitting there
>> that needs to be consumed for non-MSG_PEEK case, and only then we can
>> advance it.
>
> I general, my plan was to modify the tls_sw_recvmsg() to trigger as many
> decryption as possible as required by requested user space PEEK size.
> This would have required creating a pending list of decrypted records in tls_tx context.
Right, had been thinking the same though for a fix in -net it would have been
way too intrusive, hence the 50c6b58a814d ("tls: fix currently broken MSG_PEEK
behavior") to avoid looping the same record which is clearly a bug. Wondering
if DaveW's original rationale was to avoid accumulating too many records in the
kernel since we would need to unpause strparser and keep processing the deeper
we peek.
>> Could you elaborate on where you ever had this test succeeding? With nxp
>> accelerator?
>
> I never had this test succeeding. I pointed the problem to Dave Watson sometime
> back (found during code reading).
>
> To make sure that this bug does not slip out, I simply submitted a test case to keep
> reminding ourselves that we need to fix it sometime.
Ok, I think usually tests assert current kernel behavior to make sure any changes
coming in don't accidentally break expectations from applications as opposed to
future tests that still need fixing, but I guess I'm fine either way how to resolve
the conflict; leaving it up to DaveM. Thanks for clarifying!
Cheers,
Daniel
On 09/18/2018 11:53 AM, Daniel Borkmann wrote:
> On 09/18/2018 11:32 AM, Vakul Garg wrote:
>>> -----Original Message-----
>>> From: Daniel Borkmann <[email protected]>
>>> Sent: Tuesday, September 18, 2018 2:57 PM
>>> To: Vakul Garg <[email protected]>; Stephen Rothwell
>>> <[email protected]>; David Miller <[email protected]>;
>>> Networking <[email protected]>
>>> Cc: Linux-Next Mailing List <[email protected]>; Linux Kernel
>>> Mailing List <[email protected]>
>>> Subject: Re: linux-next: manual merge of the net-next tree with the net tree
>>>
>>> On 09/18/2018 11:10 AM, Vakul Garg wrote:
>>>>> -----Original Message-----
>>>>> From: Daniel Borkmann <[email protected]>
>>>>> Sent: Tuesday, September 18, 2018 2:14 PM
>>>>> To: Stephen Rothwell <[email protected]>; David Miller
>>>>> <[email protected]>; Networking <[email protected]>
>>>>> Cc: Linux-Next Mailing List <[email protected]>; Linux
>>>>> Kernel Mailing List <[email protected]>; Vakul Garg
>>>>> <[email protected]>
>>>>> Subject: Re: linux-next: manual merge of the net-next tree with the
>>>>> net tree
>>>>>
>>>>> On 09/18/2018 02:11 AM, Stephen Rothwell wrote:
>>>>>> Hi all,
>>>>>>
>>>>>> Today's linux-next merge of the net-next tree got a conflict in:
>>>>>>
>>>>>> tools/testing/selftests/net/tls.c
>>>>>>
>>>>>> between commit:
>>>>>>
>>>>>> 50c6b58a814d ("tls: fix currently broken MSG_PEEK behavior")
>>>>>>
>>>>>> from the net tree and commit:
>>>>>>
>>>>>> c2ad647c6442 ("selftests/tls: Add test for recv(PEEK) spanning
>>>>>> across multiple records")
>>>>>>
>>>>>> from the net-next tree.
>>>>>>
>>>>>> I fixed it up (see below) and can carry the fix as necessary. This
>>>>>> is now fixed as far as linux-next is concerned, but any non trivial
>>>>>> conflicts should be mentioned to your upstream maintainer when your
>>>>>> tree is submitted for merging. You may also want to consider
>>>>>> cooperating with the maintainer of the conflicting tree to minimise
>>>>>> any particularly complex conflicts.
>>>>>
>>>>> The test from 50c6b58a814d supersedes the one from c2ad647c6442 so
>>>>> the recv_peek_large_buf_mult_recs could be removed; latter was also
>>>>> not working correctly due to this bug.
>>>>
>>>> Why remove recv_peek_large_buf_mult_recs if its correct?
>>>> Why not the newly added one which achieves the same thing?
>>>
>>> Hmm, not quite, on net-next kernel, the recv_peek_large_buf_mult_recs fails
>>> every time I invoke the tls test suite:
>>>
>>> # ./tls
>>> [==========] Running 28 tests from 2 test cases.
>>> [ RUN ] tls.sendfile
>>> [ OK ] tls.sendfile
>>> [ RUN ] tls.send_then_sendfile
>>> [ OK ] tls.send_then_sendfile
>>> [ RUN ] tls.recv_max
>>> [ OK ] tls.recv_max
>>> [ RUN ] tls.recv_small
>>> [ OK ] tls.recv_small
>>> [ RUN ] tls.msg_more
>>> [ OK ] tls.msg_more
>>> [ RUN ] tls.sendmsg_single
>>> [ OK ] tls.sendmsg_single
>>> [ RUN ] tls.sendmsg_large
>>> [ OK ] tls.sendmsg_large
>>> [ RUN ] tls.sendmsg_multiple
>>> [ OK ] tls.sendmsg_multiple
>>> [ RUN ] tls.sendmsg_multiple_stress
>>> [ OK ] tls.sendmsg_multiple_stress
>>> [ RUN ] tls.splice_from_pipe
>>> [ OK ] tls.splice_from_pipe
>>> [ RUN ] tls.splice_from_pipe2
>>> [ OK ] tls.splice_from_pipe2
>>> [ RUN ] tls.send_and_splice
>>> [ OK ] tls.send_and_splice
>>> [ RUN ] tls.splice_to_pipe
>>> [ OK ] tls.splice_to_pipe
>>> [ RUN ] tls.recvmsg_single
>>> [ OK ] tls.recvmsg_single
>>> [ RUN ] tls.recvmsg_single_max
>>> [ OK ] tls.recvmsg_single_max
>>> [ RUN ] tls.recvmsg_multiple
>>> [ OK ] tls.recvmsg_multiple
>>> [ RUN ] tls.single_send_multiple_recv
>>> [ OK ] tls.single_send_multiple_recv
>>> [ RUN ] tls.multiple_send_single_recv
>>> [ OK ] tls.multiple_send_single_recv
>>> [ RUN ] tls.recv_partial
>>> [ OK ] tls.recv_partial
>>> [ RUN ] tls.recv_nonblock
>>> [ OK ] tls.recv_nonblock
>>> [ RUN ] tls.recv_peek
>>> [ OK ] tls.recv_peek
>>> [ RUN ] tls.recv_peek_multiple
>>> [ OK ] tls.recv_peek_multiple
>>> [ RUN ] tls.recv_peek_large_buf_mult_recs
>>> tls.c:524:tls.recv_peek_large_buf_mult_recs:Expected memcmp(test_str,
>>> buf, len) (18446744073709551595) == 0 (0)
>>> tls.recv_peek_large_buf_mult_recs: Test failed at step #8
>>> [ FAIL ] tls.recv_peek_large_buf_mult_recs
>>> [ RUN ] tls.pollin
>>> [ OK ] tls.pollin
>>> [ RUN ] tls.poll_wait
>>> [ OK ] tls.poll_wait
>>> [ RUN ] tls.blocking
>>> [ OK ] tls.blocking
>>> [ RUN ] tls.nonblocking
>>> [ OK ] tls.nonblocking
>>> [ RUN ] tls.control_msg
>>> [ OK ] tls.control_msg
>>> [==========] 27 / 28 tests passed.
>>> [ FAILED ]
>>>
>>> Here's what the recvfrom() with MSG_PEEK sees:
>>>
>>> [pid 2602] socket(AF_INET, SOCK_STREAM, IPPROTO_IP) = 3 [pid 2602]
>>> socket(AF_INET, SOCK_STREAM, IPPROTO_IP) = 4 [pid 2602] bind(4,
>>> {sa_family=AF_INET, sin_port=htons(0), sin_addr=inet_addr("0.0.0.0")}, 16) =
>>> 0
>>> [pid 2602] listen(4, 10) = 0
>>> [pid 2602] getsockname(4, {sa_family=AF_INET, sin_port=htons(41483),
>>> sin_addr=inet_addr("0.0.0.0")}, [16]) = 0 [pid 2602] connect(3,
>>> {sa_family=AF_INET, sin_port=htons(41483), sin_addr=inet_addr("0.0.0.0")},
>>> 16) = 0 [pid 2602] setsockopt(3, SOL_TCP, 0x1f /* TCP_??? */, [7564404], 4)
>>> = 0 [pid 2602] setsockopt(3, 0x11a /* SOL_?? */, 1,
>>> "\3\0033\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"...,
>>> 40) = 0 [pid 2602] accept(4, {sa_family=AF_INET, sin_port=htons(46290),
>>> sin_addr=inet_addr("127.0.0.1")}, [16]) = 5 [pid 2602] setsockopt(5,
>>> SOL_TCP, 0x1f /* TCP_??? */, [7564404], 4) = 0 [pid 2602] setsockopt(5,
>>> 0x11a /* SOL_?? */, 2,
>>> "\3\0033\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"...,
>>> 40) = 0
>>> [pid 2602] close(4) = 0
>>> [pid 2602] sendto(3, "test_read_peek", 14, 0, NULL, 0) = 14 [pid 2602]
>>> sendto(3, "_mult_recs\0", 11, 0, NULL, 0) = 11 [pid 2602] recvfrom(5,
>>> "test_read_peektest_read_peektest"..., 64, MSG_PEEK, NULL, NULL) = 64
>>> [pid 2602] write(2, "tls.c:526:tls.recv_peek_large_bu"...,
>>> 112tls.c:526:tls.recv_peek_large_buf_mult_recs:Expected memcmp(test_str,
>>> buf, len) (18446744073709551595) == 0 (0)
>>> ) = 112
>>> [pid 2602] close(3) = 0
>>> [pid 2602] close(5) = 0
>>> [pid 2602] exit_group(8) = ?
>>>
>>> Reason for the "test_read_peektest_read_peektest[...]" is because
>>> MSG_PEEK cannot call tls_sw_advance_skb(), since the skb is sitting there
>>> that needs to be consumed for non-MSG_PEEK case, and only then we can
>>> advance it.
>>
>> I general, my plan was to modify the tls_sw_recvmsg() to trigger as many
>> decryption as possible as required by requested user space PEEK size.
>> This would have required creating a pending list of decrypted records in tls_tx context.
>
> Right, had been thinking the same though for a fix in -net it would have been
> way too intrusive, hence the 50c6b58a814d ("tls: fix currently broken MSG_PEEK
> behavior") to avoid looping the same record which is clearly a bug. Wondering
> if DaveW's original rationale was to avoid accumulating too many records in the
> kernel since we would need to unpause strparser and keep processing the deeper
> we peek.
>
>>> Could you elaborate on where you ever had this test succeeding? With nxp
>>> accelerator?
>>
>> I never had this test succeeding. I pointed the problem to Dave Watson sometime
>> back (found during code reading).
>>
>> To make sure that this bug does not slip out, I simply submitted a test case to keep
>> reminding ourselves that we need to fix it sometime.
>
> Ok, I think usually tests assert current kernel behavior to make sure any changes
> coming in don't accidentally break expectations from applications as opposed to
> future tests that still need fixing, but I guess I'm fine either way how to resolve
> the conflict; leaving it up to DaveM. Thanks for clarifying!
By the way, full commit message from c2ad647c6442 said:
selftests/tls: Add test for recv(PEEK) spanning across multiple records
Added test case to receive multiple records with a single recvmsg()
operation with a MSG_PEEK set.
From reading it, the expectation would normally be that the test case would
succeed for the author, I think in future such things definitely need to be
better clarified in the commit log to avoid confusion for others.
Thanks,
Daniel
> -----Original Message-----
> From: Daniel Borkmann <[email protected]>
> Sent: Tuesday, September 18, 2018 3:46 PM
> To: Vakul Garg <[email protected]>; Stephen Rothwell
> <[email protected]>; David Miller <[email protected]>;
> Networking <[email protected]>
> Cc: Linux-Next Mailing List <[email protected]>; Linux Kernel
> Mailing List <[email protected]>; [email protected];
> [email protected]
> Subject: Re: linux-next: manual merge of the net-next tree with the net tree
>
> On 09/18/2018 11:53 AM, Daniel Borkmann wrote:
> > On 09/18/2018 11:32 AM, Vakul Garg wrote:
> >>> -----Original Message-----
> >>> From: Daniel Borkmann <[email protected]>
> >>> Sent: Tuesday, September 18, 2018 2:57 PM
> >>> To: Vakul Garg <[email protected]>; Stephen Rothwell
> >>> <[email protected]>; David Miller <[email protected]>;
> >>> Networking <[email protected]>
> >>> Cc: Linux-Next Mailing List <[email protected]>; Linux
> >>> Kernel Mailing List <[email protected]>
> >>> Subject: Re: linux-next: manual merge of the net-next tree with the
> >>> net tree
> >>>
> >>> On 09/18/2018 11:10 AM, Vakul Garg wrote:
> >>>>> -----Original Message-----
> >>>>> From: Daniel Borkmann <[email protected]>
> >>>>> Sent: Tuesday, September 18, 2018 2:14 PM
> >>>>> To: Stephen Rothwell <[email protected]>; David Miller
> >>>>> <[email protected]>; Networking <[email protected]>
> >>>>> Cc: Linux-Next Mailing List <[email protected]>; Linux
> >>>>> Kernel Mailing List <[email protected]>; Vakul Garg
> >>>>> <[email protected]>
> >>>>> Subject: Re: linux-next: manual merge of the net-next tree with
> >>>>> the net tree
> >>>>>
> >>>>> On 09/18/2018 02:11 AM, Stephen Rothwell wrote:
> >>>>>> Hi all,
> >>>>>>
> >>>>>> Today's linux-next merge of the net-next tree got a conflict in:
> >>>>>>
> >>>>>> tools/testing/selftests/net/tls.c
> >>>>>>
> >>>>>> between commit:
> >>>>>>
> >>>>>> 50c6b58a814d ("tls: fix currently broken MSG_PEEK behavior")
> >>>>>>
> >>>>>> from the net tree and commit:
> >>>>>>
> >>>>>> c2ad647c6442 ("selftests/tls: Add test for recv(PEEK) spanning
> >>>>>> across multiple records")
> >>>>>>
> >>>>>> from the net-next tree.
> >>>>>>
> >>>>>> I fixed it up (see below) and can carry the fix as necessary.
> >>>>>> This is now fixed as far as linux-next is concerned, but any non
> >>>>>> trivial conflicts should be mentioned to your upstream maintainer
> >>>>>> when your tree is submitted for merging. You may also want to
> >>>>>> consider cooperating with the maintainer of the conflicting tree
> >>>>>> to minimise any particularly complex conflicts.
> >>>>>
> >>>>> The test from 50c6b58a814d supersedes the one from c2ad647c6442
> so
> >>>>> the recv_peek_large_buf_mult_recs could be removed; latter was
> >>>>> also not working correctly due to this bug.
> >>>>
> >>>> Why remove recv_peek_large_buf_mult_recs if its correct?
> >>>> Why not the newly added one which achieves the same thing?
> >>>
> >>> Hmm, not quite, on net-next kernel, the
> >>> recv_peek_large_buf_mult_recs fails every time I invoke the tls test
> suite:
> >>>
> >>> # ./tls
> >>> [==========] Running 28 tests from 2 test cases.
> >>> [ RUN ] tls.sendfile
> >>> [ OK ] tls.sendfile
> >>> [ RUN ] tls.send_then_sendfile
> >>> [ OK ] tls.send_then_sendfile
> >>> [ RUN ] tls.recv_max
> >>> [ OK ] tls.recv_max
> >>> [ RUN ] tls.recv_small
> >>> [ OK ] tls.recv_small
> >>> [ RUN ] tls.msg_more
> >>> [ OK ] tls.msg_more
> >>> [ RUN ] tls.sendmsg_single
> >>> [ OK ] tls.sendmsg_single
> >>> [ RUN ] tls.sendmsg_large
> >>> [ OK ] tls.sendmsg_large
> >>> [ RUN ] tls.sendmsg_multiple
> >>> [ OK ] tls.sendmsg_multiple
> >>> [ RUN ] tls.sendmsg_multiple_stress
> >>> [ OK ] tls.sendmsg_multiple_stress
> >>> [ RUN ] tls.splice_from_pipe
> >>> [ OK ] tls.splice_from_pipe
> >>> [ RUN ] tls.splice_from_pipe2
> >>> [ OK ] tls.splice_from_pipe2
> >>> [ RUN ] tls.send_and_splice
> >>> [ OK ] tls.send_and_splice
> >>> [ RUN ] tls.splice_to_pipe
> >>> [ OK ] tls.splice_to_pipe
> >>> [ RUN ] tls.recvmsg_single
> >>> [ OK ] tls.recvmsg_single
> >>> [ RUN ] tls.recvmsg_single_max
> >>> [ OK ] tls.recvmsg_single_max
> >>> [ RUN ] tls.recvmsg_multiple
> >>> [ OK ] tls.recvmsg_multiple
> >>> [ RUN ] tls.single_send_multiple_recv
> >>> [ OK ] tls.single_send_multiple_recv
> >>> [ RUN ] tls.multiple_send_single_recv
> >>> [ OK ] tls.multiple_send_single_recv
> >>> [ RUN ] tls.recv_partial
> >>> [ OK ] tls.recv_partial
> >>> [ RUN ] tls.recv_nonblock
> >>> [ OK ] tls.recv_nonblock
> >>> [ RUN ] tls.recv_peek
> >>> [ OK ] tls.recv_peek
> >>> [ RUN ] tls.recv_peek_multiple
> >>> [ OK ] tls.recv_peek_multiple
> >>> [ RUN ] tls.recv_peek_large_buf_mult_recs
> >>> tls.c:524:tls.recv_peek_large_buf_mult_recs:Expected
> >>> memcmp(test_str, buf, len) (18446744073709551595) == 0 (0)
> >>> tls.recv_peek_large_buf_mult_recs: Test failed at step #8
> >>> [ FAIL ] tls.recv_peek_large_buf_mult_recs
> >>> [ RUN ] tls.pollin
> >>> [ OK ] tls.pollin
> >>> [ RUN ] tls.poll_wait
> >>> [ OK ] tls.poll_wait
> >>> [ RUN ] tls.blocking
> >>> [ OK ] tls.blocking
> >>> [ RUN ] tls.nonblocking
> >>> [ OK ] tls.nonblocking
> >>> [ RUN ] tls.control_msg
> >>> [ OK ] tls.control_msg
> >>> [==========] 27 / 28 tests passed.
> >>> [ FAILED ]
> >>>
> >>> Here's what the recvfrom() with MSG_PEEK sees:
> >>>
> >>> [pid 2602] socket(AF_INET, SOCK_STREAM, IPPROTO_IP) = 3 [pid 2602]
> >>> socket(AF_INET, SOCK_STREAM, IPPROTO_IP) = 4 [pid 2602] bind(4,
> >>> {sa_family=AF_INET, sin_port=htons(0),
> >>> sin_addr=inet_addr("0.0.0.0")}, 16) =
> >>> 0
> >>> [pid 2602] listen(4, 10) = 0
> >>> [pid 2602] getsockname(4, {sa_family=AF_INET,
> >>> sin_port=htons(41483), sin_addr=inet_addr("0.0.0.0")}, [16]) = 0
> >>> [pid 2602] connect(3, {sa_family=AF_INET, sin_port=htons(41483),
> >>> sin_addr=inet_addr("0.0.0.0")},
> >>> 16) = 0 [pid 2602] setsockopt(3, SOL_TCP, 0x1f /* TCP_??? */,
> >>> [7564404], 4) = 0 [pid 2602] setsockopt(3, 0x11a /* SOL_?? */, 1,
> >>>
> "\3\0033\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0".
> >>> ..,
> >>> 40) = 0 [pid 2602] accept(4, {sa_family=AF_INET,
> >>> sin_port=htons(46290), sin_addr=inet_addr("127.0.0.1")}, [16]) = 5
> >>> [pid 2602] setsockopt(5, SOL_TCP, 0x1f /* TCP_??? */, [7564404], 4)
> >>> = 0 [pid 2602] setsockopt(5, 0x11a /* SOL_?? */, 2,
> >>>
> "\3\0033\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0".
> >>> ..,
> >>> 40) = 0
> >>> [pid 2602] close(4) = 0
> >>> [pid 2602] sendto(3, "test_read_peek", 14, 0, NULL, 0) = 14 [pid
> >>> 2602] sendto(3, "_mult_recs\0", 11, 0, NULL, 0) = 11 [pid 2602]
> >>> recvfrom(5, "test_read_peektest_read_peektest"..., 64, MSG_PEEK,
> >>> NULL, NULL) = 64 [pid 2602] write(2,
> >>> "tls.c:526:tls.recv_peek_large_bu"...,
> >>> 112tls.c:526:tls.recv_peek_large_buf_mult_recs:Expected
> >>> memcmp(test_str, buf, len) (18446744073709551595) == 0 (0)
> >>> ) = 112
> >>> [pid 2602] close(3) = 0
> >>> [pid 2602] close(5) = 0
> >>> [pid 2602] exit_group(8) = ?
> >>>
> >>> Reason for the "test_read_peektest_read_peektest[...]" is because
> >>> MSG_PEEK cannot call tls_sw_advance_skb(), since the skb is sitting
> >>> there that needs to be consumed for non-MSG_PEEK case, and only then
> >>> we can advance it.
> >>
> >> I general, my plan was to modify the tls_sw_recvmsg() to trigger as
> >> many decryption as possible as required by requested user space PEEK
> size.
> >> This would have required creating a pending list of decrypted records in
> tls_tx context.
> >
> > Right, had been thinking the same though for a fix in -net it would
> > have been way too intrusive, hence the 50c6b58a814d ("tls: fix
> > currently broken MSG_PEEK
> > behavior") to avoid looping the same record which is clearly a bug.
> > Wondering if DaveW's original rationale was to avoid accumulating too
> > many records in the kernel since we would need to unpause strparser
> > and keep processing the deeper we peek.
> >
> >>> Could you elaborate on where you ever had this test succeeding? With
> >>> nxp accelerator?
> >>
> >> I never had this test succeeding. I pointed the problem to Dave
> >> Watson sometime back (found during code reading).
> >>
> >> To make sure that this bug does not slip out, I simply submitted a
> >> test case to keep reminding ourselves that we need to fix it sometime.
> >
> > Ok, I think usually tests assert current kernel behavior to make sure
> > any changes coming in don't accidentally break expectations from
> > applications as opposed to future tests that still need fixing, but I
> > guess I'm fine either way how to resolve the conflict; leaving it up to
> DaveM. Thanks for clarifying!
>
> By the way, full commit message from c2ad647c6442 said:
>
> selftests/tls: Add test for recv(PEEK) spanning across multiple records
>
> Added test case to receive multiple records with a single recvmsg()
> operation with a MSG_PEEK set.
>
> From reading it, the expectation would normally be that the test case would
> succeed for the author, I think in future such things definitely need to be
> better clarified in the commit log to avoid confusion for others.
>
Got it.
Thanks for the guidance.
> Thanks,
> Daniel
Hi all,
On Tue, 18 Sep 2018 10:17:03 +0000 Vakul Garg <[email protected]> wrote:
>
> Got it.
> Thanks for the guidance.
So, what should I remove? ;-)
--
Cheers,
Stephen Rothwell
On 09/18/2018 01:48 PM, Stephen Rothwell wrote:
> Hi all,
>
> On Tue, 18 Sep 2018 10:17:03 +0000 Vakul Garg <[email protected]> wrote:
>>
>> Got it.
>> Thanks for the guidance.
>
> So, what should I remove? ;-)
My (very own personal) preference in general would be that we test / assert
the kernel behavior that exists /today/, meaning once we implement support
for multi-record peek we add the corresponding test case along with that
code. Fwiw, this practice would be consistent with the rest of the kernel
selftests development model we have under net (& bpf).
Thanks,
Daniel
From: Daniel Borkmann <[email protected]>
Date: Tue, 18 Sep 2018 11:53:17 +0200
> Ok, I think usually tests assert current kernel behavior to make sure any changes
> coming in don't accidentally break expectations from applications as opposed to
> future tests that still need fixing, but I guess I'm fine either way how to resolve
> the conflict; leaving it up to DaveM. Thanks for clarifying!
I'm doing the merge right now and will leave both tests in.