From mboxrd@z Thu Jan 1 00:00:00 1970 From: David Vrabel Subject: Re: [Xen-devel] [PATCHv1 2/2] xen-netback: unref frags when handling a from-guest skb with a frag list Date: Wed, 4 Mar 2015 09:56:59 +0000 Message-ID: <54F6D6EB.5020104@citrix.com> References: <1425399971-27630-1-git-send-email-david.vrabel@citrix.com> <1425399971-27630-3-git-send-email-david.vrabel@citrix.com> <1425462482.25940.93.camel@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="windows-1252" Content-Transfer-Encoding: 7bit Cc: , Wei Liu , To: Ian Campbell , David Vrabel Return-path: Received: from smtp.citrix.com ([66.165.176.89]:8781 "EHLO SMTP.CITRIX.COM" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932603AbbCDJ5B (ORCPT ); Wed, 4 Mar 2015 04:57:01 -0500 In-Reply-To: <1425462482.25940.93.camel@citrix.com> Sender: netdev-owner@vger.kernel.org List-ID: On 04/03/15 09:48, Ian Campbell wrote: > On Tue, 2015-03-03 at 16:26 +0000, David Vrabel wrote: >> Every time a VIF is destroyed up-to 256 pages may be leaked if packets >> with more than MAX_SKB_FRAGS frags where transmitted from the guest. >> Even worse, if another user of ballooned pages allocated one of these >> ballooned pages it would not handle the the unexpectedly non-zero page >> count (e.g., gntdev would deadlock when unmapping a grant because the >> page count would never reach 1). >> >> When handling a from-guest skb with a frag list, unref the frags >> before releasing them so they are freed correctly when the VIF is >> destroyed. > > Am I right that the majority of the first 2 hunks (and various bits of > the 3rd) are just switching the outer loop to nr_frags instead of i, to > free up i for use in the new code below? And also to switch j to the now > available i in the inner loop. Yes. If you prefer I can split this into one patch that adds the skb_frag_unref() calls and one that reorders/refactors. >> Also swap over to the new (local) frags /after/ calling the skb >> destructor. This isn't strictly necessary but it's less confusing. > > My only concern would be that this now means there is a period where the > frags list is invalid. However I think the calling context is such that > nobody else can have a reference to an skb which has the same shinfo as > the one in our hand. Is that right? That is correct. This skb is allocated by netback and has not yet been passed up to the network stack. David