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

Message-ID: <CANn89iJL+6d1o6XHrqjUH=7_AwstV1knrsHtwR_d6WOYCRLVwg@mail.gmail.com>
Date:   Tue, 5 Mar 2019 07:11:46 -0800
From:   Eric Dumazet <edumazet@...gle.com>
To:     Vasily Averin <vvs@...tuozzo.com>
Cc:     Eric Dumazet <eric.dumazet@...il.com>,
        netdev <netdev@...r.kernel.org>,
        Al Viro <viro@...iv.linux.org.uk>
Subject: Re: [PATCH] tcp: detect use sendpage for slab-based objects

Resent in plain text mode for the lists.

On Tue, Mar 5, 2019 at 7:08 AM Eric Dumazet <edumazet@...gle.com> wrote:
>
>
>
> On Tue, Mar 5, 2019 at 6:24 AM Vasily Averin <vvs@...tuozzo.com> wrote:
>>
>> 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?
>>
>
>
> My original suggestion was to use VM_WARN_ONCE() so that the debug checks would
> be compiled out by the compiler, unless you compile a debug kernel.
>
> Something like :
>
> diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> index ad07dd71063da09843ccfbd3e00d3f41567e1205..889563a66dde2a41c198d92a80183cf5382f632d 100644
> --- a/net/ipv4/tcp.c
> +++ b/net/ipv4/tcp.c
> @@ -943,6 +943,9 @@ 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 (VM_WARN_ONCE(PageSlab(page)), "page must not be a Slab one")
> +               return -EINVAL;
> +
>         /* 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.
>
>
> For some reason you added a fallback :/
>