Resent in plain text mode for the lists.
On Tue, Mar 5, 2019 at 7:08 AM Eric Dumazet <eduma...@google.com> wrote:
>
>
>
> On Tue, Mar 5, 2019 at 6:24 AM Vasily Averin <v...@virtuozzo.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 :/
>
