ACK+CMNT: [PATCH 0/1][SRU][FOCAL][GROOVY] tcp: correct read of TFO keys on big endian systems

[Colin Ian King]

On 11/08/2020 18:00, Thadeu Lima de Souza Cascardo wrote:
> On Tue, Aug 11, 2020 at 05:01:39PM +0100, Colin King wrote:
>> From: Colin Ian King <colin.king at canonical.com>
>>
>> BugLink: https://bugs.launchpad.net/bugs/1869134
>>
>> == SRU Justification Focal, Groovy ==
>>
>> Running the tcp_fastopen_backup_key.sh from net in ubuntu_kernel_selftests
>> fails on little endian systems. This is a regression that occurred because
> 
> You mean they fail on big endian systems, right?

Yep. I get my ends mixed up.

> 
> The fact that there is no clear bounds checking in that code makes me a little
> uneasy, but I don't see regressions on this patch on that regard. And by
> inspecting the code, tcp_fastopen_context_len will return the value for a
> member that is initialized to either 1 or 2 right after allocation. And
> TCP_FASTOPEN_KEY_MAX is 2.
> 
> But worse yet is all these shenanigans with types. Is it a u32, a u64 or a
> siphash_key_t? Is it decided yet? But it seems this is just fine too, as all
> buffers are sized TCP_FASTOPEN_KEY_BUF_LENGTH.

The original code before the regression was much like this.

> 
> The test seem to be specific to the sysctl interface. Or is getsockopt tested
> at all?

I believe the TCP getsockopt is being exercise in the selftests, e.g.

selftests/net/reuseport_bpf.c:

	if (setsockopt(fd[i], SOL_TCP, TCP_FASTOPEN, &opt, ....

so I think we have code test coverage for this.

> 
> Acked-by: Thadeu Lima de Souza Cascardo <cascardo at canonical.com>
> 
>> of commit 438ac88009bc ("net: fastopen: robustness and endianness fixes
>> for SipHash"). 
>>
>> == Fix ==
>>
>> Upstream (linux-next) fix:
>>
>>   commit f19008e676366c44e9241af57f331b6c6edf9552
>>   Author: Jason Baron <jbaron at akamai.com>
>>   Date:   Mon Aug 10 13:38:39 2020 -0400
>>
>>       tcp: correct read of TFO keys on big endian systems
>>
>> == Test case ==
>>
>> Run the kernel self tests tcp_fastopen_backup_key.sh.  Currently this fails
>> on the last few test cases.  With the fix the test passes.
>>
>> == Regression Potential == 
>>
>> This fix touches the tcp fast open code paths so it can potentially impact
>> TCP fast opening and perhaps the TCP opening. However, this has been tested
>> on big and little endian systems with the regression tests, so the
>> regression risk is low with this fix.
>>
>> -----
>>
>> Jason Baron (1):
>>   tcp: correct read of TFO keys on big endian systems
>>
>>  include/net/tcp.h          |  2 ++
>>  net/ipv4/sysctl_net_ipv4.c | 16 ++++------------
>>  net/ipv4/tcp.c             | 16 ++++------------
>>  net/ipv4/tcp_fastopen.c    | 23 +++++++++++++++++++++++
>>  4 files changed, 33 insertions(+), 24 deletions(-)
>>
>> -- 
>> 2.27.0
>>
>>
>> -- 
>> kernel-team mailing list
>> kernel-team at lists.ubuntu.com
>> https://lists.ubuntu.com/mailman/listinfo/kernel-team