From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-1.0 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SPF_PASS,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 31EF9C43381 for ; Mon, 25 Feb 2019 09:15:48 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id F3E6F20842 for ; Mon, 25 Feb 2019 09:15:47 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726446AbfBYJPq (ORCPT ); Mon, 25 Feb 2019 04:15:46 -0500 Received: from relay.sw.ru ([185.231.240.75]:60454 "EHLO relay.sw.ru" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726144AbfBYJPq (ORCPT ); Mon, 25 Feb 2019 04:15:46 -0500 Received: from [172.16.24.21] by relay.sw.ru with esmtp (Exim 4.91) (envelope-from ) id 1gyCMR-0001Kb-Da; Mon, 25 Feb 2019 12:15:43 +0300 Subject: Re: [PATCH] tcp: detect use sendpage for slab-based objects To: Eric Dumazet Cc: netdev References: <80f35733-e3cf-c7da-1822-87054903dc67@virtuozzo.com> From: Vasily Averin Message-ID: <56220566-0811-eabe-53f2-9fa625a58bbd@virtuozzo.com> Date: Mon, 25 Feb 2019 12:15:41 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.4.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org On 2/22/19 7:39 PM, Eric Dumazet wrote: > On Fri, Feb 22, 2019 at 6:02 AM Vasily Averin wrote: >> Eric, could you please elaborate once again why tcp_sendpage() should not handle slab objects? > > Simply because SLAB has its own way to manage objects from a page, and > does not care > about the underlying page having its refcount elevated. > > ptr = kmalloc(xx) > ... < here you can attempt cheating and add one to the underlying page> > kfree(ptr); // SLAB does not care of page count, it will effectively > put ptr in the free list. > > ptr2 = kmalloc(xx); // > > ptr2 can be the same than ptr (object was kfreed() earlier) > > This means that some other stuff will happily reuse the piece of > memory that you wanted to use for zero-copy. > > This is a serious bug IMO, since this would allow for data corruption. Thank you for explanation, however I still have some doubts. Yes, it's strange to use sendpage if we want to send some small 8-bytes-long slab based object, it's better to use sendmsg instead. Yes, using of sendpage for slab-based objects can require special attention to guarantee that slab object will not be freed until end of IO. However IMO this should be guaranteed if caller uses sendmsg instead of sendpage. Btw, as far as I understand in my example XFS did it correctly, submitted slab objects was kept in use and seems they should be freed after end of IO, via end_io callback. At least I did not found any bugs in sendpage callers. And most important, it seems for me switch from sendpage to sendmsg doe not resolve the problem completely: tcp_sendmsg_locked() under some conditions can merge neighbours slab-based tcp fragments, so local tcp_recvmsg() can trigger BUG_ON in this case too. Am I missed something probably? >> There is known restriction: sendpage should not be called for pages with counter=0, >> because internal put_page() releases the page. All sendpage callers I know have such check. >> >> However why they should add one check for PageSlab? >> >> Let me explain the problem once again: >> I do not see any bugs neither in tcp nor in any sendpage callers, >> there is false alert on receiving side that crashes correctly worked host. > > This is not a false alert, but a very fundamental issue. > > We can not mix kmalloc() and page fragments, this simply is buggy.