| 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
| ||
|
Message-ID: <51DEB8B4.4030201@oracle.com>
Date: Thu, 11 Jul 2013 21:52:52 +0800
From: annie li <annie.li@...cle.com>
To: Matt Wilson <msw@...zon.com>
CC: netdev@...r.kernel.org, Wei Liu <wei.liu2@...rix.com>,
Ian Campbell <Ian.Campbell@...rix.com>,
Xi Xiong <xixiong@...zon.com>, xen-devel@...ts.xenproject.org
Subject: Re: [Xen-devel] [PATCH RFC] xen-netback: calculate the number of
slots required for large MTU vifs
On 2013-7-11 14:01, annie li wrote:
>
> On 2013-7-11 13:14, Matt Wilson wrote:
>> On Wed, Jul 10, 2013 at 08:37:03PM +0100, Wei Liu wrote:
>>> On Wed, Jul 10, 2013 at 09:13:33AM +0100, Wei Liu wrote:
>>>> On Tue, Jul 09, 2013 at 10:40:59PM +0000, Matt Wilson wrote:
>>>>> From: Xi Xiong <xixiong@...zon.com>
>>>>>
>>>>> [ note: I've just cherry picked this onto net-next, and only compile
>>>>> tested. This a RFC only. -msw ]
>>>>>
>>>> Should probably rebase it on net.git because it is a bug fix. Let's
>>>> worry about that later...
>> *nod*
>>
>>>>> Currently the number of RX slots required to transmit a SKB to
>>>>> xen-netfront can be miscalculated when an interface uses a MTU larger
>>>>> than PAGE_SIZE. If the slot calculation is wrong, xen-netback can
>>>>> pause the queue indefinitely or reuse slots. The former manifests
>>>>> as a
>>>>> loss of connectivity to the guest (which can be restored by lowering
>>>>> the MTU set on the interface). The latter manifests with "Bad grant
>>>>> reference" messages from Xen such as:
>>>>>
>>>>> (XEN) grant_table.c:1797:d0 Bad grant reference 264241157
>>>>>
>>>>> and kernel messages within the guest such as:
>>>>>
>>>>> [ 180.419567] net eth0: Invalid extra type: 112
>>>>> [ 180.868620] net eth0: rx->offset: 0, size: 4294967295
>>>>> [ 180.868629] net eth0: rx->offset: 0, size: 4294967295
>>>>>
>>>>> BUG_ON() assertions can also be hit if RX slots are exhausted while
>>>>> handling a SKB.
>>>>>
>>>>> This patch changes xen_netbk_rx_action() to count the number of RX
>>>>> slots actually consumed by netbk_gop_skb() instead of using
>>>>> nr_frags + 1.
>>>>> This prevents under-counting the number of RX slots consumed when a
>>>>> SKB has a large linear buffer.
>>>>>
>>>>> Additionally, we now store the estimated number of RX slots required
>>>>> to handle a SKB in the cb overlay. This value is used to determine if
>>>>> the next SKB in the queue can be processed.
>>>>>
>>>>> Finally, the logic in start_new_rx_buffer() can cause RX slots to be
>>>>> wasted when setting up copy grant table operations for SKBs with
>>>>> large
>>>>> linear buffers. For example, a SKB with skb_headlen() equal to 8157
>>>>> bytes that starts 64 bytes 64 bytes from the start of the page will
>>>> Duplicated "64 bytes". And this change looks like an improvement not a
>>>> bug fix. Probably submit a separate patch for this?
>> Argh, I knew it was in there somewhere (since you pointed it out in
>> Dubin :-).
>>
>> Maybe it could be a separate patch. I think the description is also a
>> bit confusing. I'll work on rewording it.
>>
>>>>> consume three RX slots instead of two. This patch changes the "head"
>>>>> parameter to netbk_gop_frag_copy() to act as a flag. When set,
>>>>> start_new_rx_buffer() will always place as much data as possible into
>>>>> each RX slot.
>>>>>
>>>>> Signed-off-by: Xi Xiong <xixiong@...zon.com>
>>>>> Reviewed-by: Matt Wilson <msw@...zon.com>
>>>>> [ msw: minor code cleanups, rewrote commit message, adjusted code
>>>>> to count RX slots instead of meta structures ]
>>>>> Signed-off-by: Matt Wilson <msw@...zon.com>
>>>>> Cc: Annie Li <annie.li@...cle.com>
>>>>> Cc: Wei Liu <wei.liu2@...rix.com>
>>>>> Cc: Ian Campbell <Ian.Campbell@...rix.com>
>>>>> Cc: netdev@...r.kernel.org
>>>>> Cc: xen-devel@...ts.xenproject.org
>>>>> ---
>>>>> drivers/net/xen-netback/netback.c | 51
>>>>> ++++++++++++++++++++++--------------
>>>>> 1 files changed, 31 insertions(+), 20 deletions(-)
>>>>>
>>>>> diff --git a/drivers/net/xen-netback/netback.c
>>>>> b/drivers/net/xen-netback/netback.c
>>>>> index 64828de..82dd207 100644
>>>>> --- a/drivers/net/xen-netback/netback.c
>>>>> +++ b/drivers/net/xen-netback/netback.c
>>>>> @@ -110,6 +110,11 @@ union page_ext {
>>>>> void *mapping;
>>>>> };
>>>>> +struct skb_cb_overlay {
>>>>> + int meta_slots_used;
>>>>> + int peek_slots_count;
>>>>> +};
>>>>> +
>>>>> struct xen_netbk {
>>>>> wait_queue_head_t wq;
>>>>> struct task_struct *task;
>>>>> @@ -370,6 +375,7 @@ unsigned int xen_netbk_count_skb_slots(struct
>>>>> xenvif *vif, struct sk_buff *skb)
>>>>> {
>>>>> unsigned int count;
>>>>> int i, copy_off;
>>>>> + struct skb_cb_overlay *sco;
>>>>> count = DIV_ROUND_UP(skb_headlen(skb), PAGE_SIZE);
>>>>> @@ -411,6 +417,9 @@ unsigned int
>>>>> xen_netbk_count_skb_slots(struct xenvif *vif, struct sk_buff *skb)
>>>>> offset = 0;
>>>>> }
>>>>> }
>>>>> +
>>>>> + sco = (struct skb_cb_overlay *) skb->cb;
>>>>> + sco->peek_slots_count = count;
>>>>> return count;
>>>>> }
>>>>> @@ -443,13 +452,12 @@ static struct netbk_rx_meta
>>>>> *get_next_rx_buffer(struct xenvif *vif,
>>>>> }
>>>>> /*
>>>>> - * Set up the grant operations for this fragment. If it's a flipping
>>>>> - * interface, we also set up the unmap request from here.
>>>>> + * Set up the grant operations for this fragment.
>>>>> */
>>>>> static void netbk_gop_frag_copy(struct xenvif *vif, struct
>>>>> sk_buff *skb,
>>>>> struct netrx_pending_operations *npo,
>>>>> struct page *page, unsigned long size,
>>>>> - unsigned long offset, int *head)
>>>>> + unsigned long offset, int head, int *first)
>>>>> {
>>>>> struct gnttab_copy *copy_gop;
>>>>> struct netbk_rx_meta *meta;
>>>>> @@ -479,12 +487,12 @@ static void netbk_gop_frag_copy(struct
>>>>> xenvif *vif, struct sk_buff *skb,
>>>>> if (bytes > size)
>>>>> bytes = size;
>>>>> - if (start_new_rx_buffer(npo->copy_off, bytes, *head)) {
>>>>> + if (start_new_rx_buffer(npo->copy_off, bytes, head)) {
>
> head is always 1 here when processing the header data, so it is
> impossible to start a new buffer for header data with larger header
> size, and get_next_rx_buffer is never called. I assume this would not
> work well for skb with larger header data size. Have you run network
> test with this patch?
Sorry, I forgot the offset == MAX_BUFFER_OFFSET case and misunderstand
your patch, please ignore my last comments. Your patch keeps the
original DIV_ROUND_UP and changes the mechanism in netbk_gop_frag_copy
to make slots same with xen_netbk_count_skb_slots. All Roads Lead to
Rome!:-)
Thanks
Annie
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists