From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jason Wang Subject: Re: [PATCH net V3 1/2] ptr_ring: try vmalloc() when kmalloc() fails Date: Fri, 9 Feb 2018 12:04:53 +0800 Message-ID: <2f1e33ff-1ea9-9c5e-1d4c-63f6a4b0cce0@redhat.com> References: <1518062365-8596-1-git-send-email-jasowang@redhat.com> <20180208063509-mutt-send-email-mst@kernel.org> <2e3978d8-8bf1-2f0c-b446-a5dd65c7ac94@redhat.com> <20180208211636-mutt-send-email-mst@kernel.org> <20180209055157-mutt-send-email-mst@kernel.org> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Cc: netdev@vger.kernel.org To: "Michael S. Tsirkin" Return-path: Received: from mx3-rdu2.redhat.com ([66.187.233.73]:34110 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1752154AbeBIEE5 (ORCPT ); Thu, 8 Feb 2018 23:04:57 -0500 Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.rdu2.redhat.com [10.11.54.6]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 54309404084F for ; Fri, 9 Feb 2018 04:04:57 +0000 (UTC) In-Reply-To: <20180209055157-mutt-send-email-mst@kernel.org> Content-Language: en-US Sender: netdev-owner@vger.kernel.org List-ID: On 2018年02月09日 11:56, Michael S. Tsirkin wrote: > On Fri, Feb 09, 2018 at 11:49:12AM +0800, Jason Wang wrote: >> >> On 2018年02月09日 03:17, Michael S. Tsirkin wrote: >>> On Thu, Feb 08, 2018 at 02:58:40PM +0800, Jason Wang wrote: >>>> On 2018年02月08日 12:45, Michael S. Tsirkin wrote: >>>>> On Thu, Feb 08, 2018 at 11:59:24AM +0800, Jason Wang wrote: >>>>>> This patch switch to use kvmalloc_array() for using a vmalloc() >>>>>> fallback to help in case kmalloc() fails. >>>>>> >>>>>> Reported-by:syzbot+e4d4f9ddd4295539735d@syzkaller.appspotmail.com >>>>>> Fixes: 2e0ab8ca83c12 ("ptr_ring: array based FIFO for pointers") >>>>> I guess the actual patch is the one that switches tun to ptr_ring. >>>> I think not, since the issue was large allocation. >>>> >>>>> In fact, I think the actual bugfix is patch 2/2. This specific one >>>>> just makes kmalloc less likely to fail but that's >>>>> not what syzbot reported. >>>> Agree. >>>> >>>>> Then I would add this patch on top to make kmalloc less likely to fail. >>>> Ok. >>>> >>>>>> Signed-off-by: Jason Wang >>>>>> --- >>>>>> include/linux/ptr_ring.h | 10 +++++----- >>>>>> 1 file changed, 5 insertions(+), 5 deletions(-) >>>>>> >>>>>> diff --git a/include/linux/ptr_ring.h b/include/linux/ptr_ring.h >>>>>> index 1883d61..2af71a7 100644 >>>>>> --- a/include/linux/ptr_ring.h >>>>>> +++ b/include/linux/ptr_ring.h >>>>>> @@ -466,7 +466,7 @@ static inline int ptr_ring_consume_batched_bh(struct ptr_ring *r, >>>>>> static inline void **__ptr_ring_init_queue_alloc(unsigned int size, gfp_t gfp) >>>>>> { >>>>>> - return kcalloc(size, sizeof(void *), gfp); >>>>>> + return kvmalloc_array(size, sizeof(void *), gfp | __GFP_ZERO); >>>>>> } >>>>>> static inline void __ptr_ring_set_size(struct ptr_ring *r, int size) >>>>> This implies a bunch of limitations on the flags. From kvmalloc_node >>>>> docs: >>>>> >>>>> * Reclaim modifiers - __GFP_NORETRY and __GFP_NOFAIL are not supported. >>>>> * __GFP_RETRY_MAYFAIL is supported, and it should be used only if kmalloc is >>>>> * preferable to the vmalloc fallback, due to visible performance drawbacks. >>>>> >>>>> Fine with all the current users, but if we go this way, please add >>>>> documentation so future users don't misuse this API. >>>> I suspect this is somehow a overkill since this means we need sync with >>>> mm/vmalloc changes in the future to keep it synced. >>>> >>>>> Alternatively, test flags and call kvmalloc or kcalloc? >>>> Similar to the above issue, I would rather leave it as is. >>>> >>>> Thanks >>> How do we prevent someone from inevitably trying to use this with >>> GFP_ATOMIC? >>> >> Well, we somehow can't prevent this even if there's a documentation, that's >> why there's a BUG() in vmalloc code I think. And kvmalloc also requires >> GFP_KERNEL otherewise another WARN(). >> >> So looks like the WARN()/BUG() should be sufficient? > Well vmalloc only triggers when you pass in a huge size. > Let's settle for There's a:     BUG_ON(in_interrupt()); in __get_vm_area_node(). > > /* Not all gfp_t flags (besides GFP_KERNEL) are allowed. See > * documentation for vmalloc for which of them are legal. > */ Fine with me. >> Thanks >> >> Another thing is kvm > ? Sorry typo. Thanks