netdev - Re: [PATCH] tcp: detect use sendpage for slab-based objects

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] [day] [month] [year] [list]

Message-ID: <85b6a52f-f997-6fc6-bc5a-99aaeaa27cc5@virtuozzo.com>
Date:   Tue, 5 Mar 2019 17:24:37 +0300
From:   Vasily Averin <vvs@...tuozzo.com>
To:     Eric Dumazet <eric.dumazet@...il.com>,
        Eric Dumazet <edumazet@...gle.com>
Cc:     netdev <netdev@...r.kernel.org>, Al Viro <viro@...iv.linux.org.uk>
Subject: Re: [PATCH] tcp: detect use sendpage for slab-based objects

On 3/4/19 6:51 PM, Eric Dumazet wrote:
> On 03/04/2019 04:58 AM, Vasily Averin wrote:
>> Eric, what do you think about following patch?
>> I validate its backported version on RHEL7 based OpenVZ kernel before sending to mainline. 
>>
>> diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
>> index cf3c5095c10e..7be7b6abe8b5 100644
>> --- a/net/ipv4/tcp.c
>> +++ b/net/ipv4/tcp.c
>> @@ -943,6 +943,11 @@ ssize_t do_tcp_sendpages(struct sock *sk, struct page *page, int offset,
>>  	ssize_t copied;
>>  	long timeo = sock_sndtimeo(sk, flags & MSG_DONTWAIT);
>>  
>> +	if (PageSlab(page)) {
>> +		VM_WARN_ONCE(true, "sendpage should not handle Slab objects,"
>> +				   " please fix callers\n");
>> +		return sock_no_sendpage_locked(sk, page, offset, size, flags);
>> +	}
>>  	/* Wait for a connection to finish. One exception is TCP Fast Open
>>  	 * (passive side) where data is allowed to be sent before a connection
>>  	 * is fully established.
>>
> 
> There are at least four problems with this approach :
> 
> 1) VM_WARN_ONCE() might be a NOP, and if not, it is simply some lines in syslog,
> among thousands.
> 
> 2) Falling back will give no incentive for callers to fix their code.

We can return error instead of fallback,
but yes, it means an extra (almost unneeded) check in TCP code. 
 
> 3) slowing down TCP, just because of some weird kernel-users.
>    I agree to add sanity check for everything user space can think of (aka syzbot),
>    but kernel users need to be fixed, without adding code in TCP.

Do you advise to add PageSlab check into all .sendpage / .sendpacge_locked / 
tcp_sendpage / do_tcp_sednpages callers instead?

> 4) sendpage() API is providing one page at a time.
>    We therefore call very expensive lock_sock() and release_sock() for every page.
>    sendfile() is sub optimal (compared to sendmsg(MSG_ZEROCOPY))
>    There is an effort to provide batches of pages per round.
>    Your patch would cancel this effort, or make it very complicated.