From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S1751352AbaKFVtr (ORCPT <rfc822;w@1wt.eu>);
	Thu, 6 Nov 2014 16:49:47 -0500
Received: from mail-ob0-f177.google.com ([209.85.214.177]:64933 "EHLO
	mail-ob0-f177.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with ESMTP id S1751243AbaKFVtp (ORCPT
	<rfc822;linux-kernel@vger.kernel.org>);
	Thu, 6 Nov 2014 16:49:45 -0500
Date: Thu, 6 Nov 2014 15:49:40 -0600
From: Seth Forshee <seth.forshee@canonical.com>
To: netdev@vger.kernel.org
Cc: "David S. Miller" <davem@davemloft.net>,
        Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>,
        Boris Ostrovsky <boris.ostrovsky@oracle.com>,
        David Vrabel <david.vrabel@citrix.com>,
        Stefan Bader <stefan.bader@canonical.com>,
        Jay Vosburgh <jay.vosburgh@canonical.com>,
        linux-kernel@vger.kernel.org, xen-devel@lists.xenproject.org,
        seth.forshee@canonical.com
Subject: BUG in xennet_make_frags with paged skb data
Message-ID: <20141106214940.GD44162@ubuntu-hedt>
Mail-Followup-To: netdev@vger.kernel.org,
	"David S. Miller" <davem@davemloft.net>,
	Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>,
	Boris Ostrovsky <boris.ostrovsky@oracle.com>,
	David Vrabel <david.vrabel@citrix.com>,
	Stefan Bader <stefan.bader@canonical.com>,
	Jay Vosburgh <jay.vosburgh@canonical.com>,
	linux-kernel@vger.kernel.org, xen-devel@lists.xenproject.org
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
User-Agent: Mutt/1.5.21 (2010-09-15)
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

We've had several reports of hitting the following BUG_ON in
xennet_make_frags with 3.2 and 3.13 kernels (I'm currently awaiting
results of testing with 3.17):

        /* Grant backend access to each skb fragment page. */
        for (i = 0; i < frags; i++) {
                skb_frag_t *frag = skb_shinfo(skb)->frags + i;
                struct page *page = skb_frag_page(frag);

                len = skb_frag_size(frag);
                offset = frag->page_offset;

                /* Data must not cross a page boundary. */
                BUG_ON(len + offset > PAGE_SIZE<<compound_order(page));

When this happens the page in question is a "middle" page in a compound
page (i.e. it's a tail page but not the last tail page), and the data is
fully contained within the compound page. The data does however cross
the hardware page boundary, and since compound_order evaluates to 0 for
tail pages the check fails.

In going over this I've been unable to determine whether the BUG_ON in
xennet_make_frags is incorrect or the paged skb data is wrong. I can't
find that it's documented anywhere, and the networking code itself is a
bit ambiguous when it comes to compound pages. On the one hand
__skb_fill_page_desc specifically handles adding tail pages as paged
data, but on the other hand skb_copy_bits kmaps frag->page.p which could
fail with data that extends into another page.

Can anyone explain what the rules are here? My best guess based on
skb_copy_bits is that paged data should never cross the hardware page
boundary, but I'm not really sure how all of this works out when dealing
with compound pages.

Thanks,
Seth

From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S1751401AbaKGJZr (ORCPT <rfc822;w@1wt.eu>);
	Fri, 7 Nov 2014 04:25:47 -0500
Received: from mail-wi0-f170.google.com ([209.85.212.170]:45016 "EHLO
	mail-wi0-f170.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with ESMTP id S1751121AbaKGJZn (ORCPT
	<rfc822;linux-kernel@vger.kernel.org>);
	Fri, 7 Nov 2014 04:25:43 -0500
Message-ID: <545C9013.1090406@linaro.org>
Date: Fri, 07 Nov 2014 09:25:39 +0000
From: Zoltan Kiss <zoltan.kiss@linaro.org>
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.2.0
MIME-Version: 1.0
To: netdev@vger.kernel.org, "David S. Miller" <davem@davemloft.net>,
        Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>,
        Boris Ostrovsky <boris.ostrovsky@oracle.com>,
        David Vrabel <david.vrabel@citrix.com>,
        Stefan Bader <stefan.bader@canonical.com>,
        Jay Vosburgh <jay.vosburgh@canonical.com>,
        linux-kernel@vger.kernel.org, xen-devel@lists.xenproject.org
Subject: Re: [Xen-devel] BUG in xennet_make_frags with paged skb data
References: <20141106214940.GD44162@ubuntu-hedt>
In-Reply-To: <20141106214940.GD44162@ubuntu-hedt>
Content-Type: text/plain; charset=windows-1252; format=flowed
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Hi,

AFAIK in this scenario your skb frag is wrong. The page pointer should 
point to the original compound page (not a member of it), and offset 
should be set accordingly.
For example, if your compound page is 16K (4 page), then the page 
pointer should point to the first page, and if the data starts at the 
3rd page, then offset should be >8K

Zoli

On 06/11/14 21:49, Seth Forshee wrote:
> We've had several reports of hitting the following BUG_ON in
> xennet_make_frags with 3.2 and 3.13 kernels (I'm currently awaiting
> results of testing with 3.17):
>
>          /* Grant backend access to each skb fragment page. */
>          for (i = 0; i < frags; i++) {
>                  skb_frag_t *frag = skb_shinfo(skb)->frags + i;
>                  struct page *page = skb_frag_page(frag);
>
>                  len = skb_frag_size(frag);
>                  offset = frag->page_offset;
>
>                  /* Data must not cross a page boundary. */
>                  BUG_ON(len + offset > PAGE_SIZE<<compound_order(page));
>
> When this happens the page in question is a "middle" page in a compound
> page (i.e. it's a tail page but not the last tail page), and the data is
> fully contained within the compound page. The data does however cross
> the hardware page boundary, and since compound_order evaluates to 0 for
> tail pages the check fails.
>
> In going over this I've been unable to determine whether the BUG_ON in
> xennet_make_frags is incorrect or the paged skb data is wrong. I can't
> find that it's documented anywhere, and the networking code itself is a
> bit ambiguous when it comes to compound pages. On the one hand
> __skb_fill_page_desc specifically handles adding tail pages as paged
> data, but on the other hand skb_copy_bits kmaps frag->page.p which could
> fail with data that extends into another page.
>
> Can anyone explain what the rules are here? My best guess based on
> skb_copy_bits is that paged data should never cross the hardware page
> boundary, but I'm not really sure how all of this works out when dealing
> with compound pages.
>
> Thanks,
> Seth
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
>

From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S1751856AbaKGKoV (ORCPT <rfc822;w@1wt.eu>);
	Fri, 7 Nov 2014 05:44:21 -0500
Received: from smtp02.citrix.com ([66.165.176.63]:47515 "EHLO
	SMTP02.CITRIX.COM" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with ESMTP id S1751425AbaKGKoS (ORCPT
	<rfc822;linux-kernel@vger.kernel.org>);
	Fri, 7 Nov 2014 05:44:18 -0500
X-IronPort-AV: E=Sophos;i="5.07,331,1413244800"; 
   d="scan'208";a="190513031"
Message-ID: <545CA27F.4070400@citrix.com>
Date: Fri, 7 Nov 2014 10:44:15 +0000
From: David Vrabel <david.vrabel@citrix.com>
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Icedove/24.5.0
MIME-Version: 1.0
To: <netdev@vger.kernel.org>, "David S. Miller" <davem@davemloft.net>,
        Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>,
        Boris Ostrovsky <boris.ostrovsky@oracle.com>,
        Stefan Bader <stefan.bader@canonical.com>,
        Jay Vosburgh <jay.vosburgh@canonical.com>,
        <linux-kernel@vger.kernel.org>, <xen-devel@lists.xenproject.org>
Subject: Re: BUG in xennet_make_frags with paged skb data
References: <20141106214940.GD44162@ubuntu-hedt>
In-Reply-To: <20141106214940.GD44162@ubuntu-hedt>
Content-Type: text/plain; charset="ISO-8859-1"
Content-Transfer-Encoding: 7bit
X-DLP: MIA1
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On 06/11/14 21:49, Seth Forshee wrote:
> We've had several reports of hitting the following BUG_ON in
> xennet_make_frags with 3.2 and 3.13 kernels (I'm currently awaiting
> results of testing with 3.17):
> 
>         /* Grant backend access to each skb fragment page. */
>         for (i = 0; i < frags; i++) {
>                 skb_frag_t *frag = skb_shinfo(skb)->frags + i;
>                 struct page *page = skb_frag_page(frag);
> 
>                 len = skb_frag_size(frag);
>                 offset = frag->page_offset;
> 
>                 /* Data must not cross a page boundary. */
>                 BUG_ON(len + offset > PAGE_SIZE<<compound_order(page));
> 
> When this happens the page in question is a "middle" page in a compound
> page (i.e. it's a tail page but not the last tail page), and the data is
> fully contained within the compound page. The data does however cross
> the hardware page boundary, and since compound_order evaluates to 0 for
> tail pages the check fails.
> 
> In going over this I've been unable to determine whether the BUG_ON in
> xennet_make_frags is incorrect or the paged skb data is wrong. I can't
> find that it's documented anywhere, and the networking code itself is a
> bit ambiguous when it comes to compound pages. On the one hand
> __skb_fill_page_desc specifically handles adding tail pages as paged
> data, but on the other hand skb_copy_bits kmaps frag->page.p which could
> fail with data that extends into another page.

netfront will safely handle this case so you can remove this BUG_ON()
(and the one later on).  But it would be better to find out were these
funny-looking skbs are coming from and (if necessary) fixing the bug there.

David

From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S1751916AbaKGLI0 (ORCPT <rfc822;w@1wt.eu>);
	Fri, 7 Nov 2014 06:08:26 -0500
Received: from youngberry.canonical.com ([91.189.89.112]:33864 "EHLO
	youngberry.canonical.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with ESMTP id S1751047AbaKGLIY (ORCPT
	<rfc822;linux-kernel@vger.kernel.org>);
	Fri, 7 Nov 2014 06:08:24 -0500
Message-ID: <545CA823.2080307@canonical.com>
Date: Fri, 07 Nov 2014 12:08:19 +0100
From: Stefan Bader <stefan.bader@canonical.com>
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.2.0
MIME-Version: 1.0
To: David Vrabel <david.vrabel@citrix.com>, netdev@vger.kernel.org,
        "David S. Miller" <davem@davemloft.net>,
        Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>,
        Boris Ostrovsky <boris.ostrovsky@oracle.com>,
        Jay Vosburgh <jay.vosburgh@canonical.com>,
        linux-kernel@vger.kernel.org, xen-devel@lists.xenproject.org
Subject: Re: BUG in xennet_make_frags with paged skb data
References: <20141106214940.GD44162@ubuntu-hedt> <545CA27F.4070400@citrix.com>
In-Reply-To: <545CA27F.4070400@citrix.com>
Content-Type: multipart/signed; micalg=pgp-sha512;
 protocol="application/pgp-signature";
 boundary="bCvbWEJMulW7EirtrJbTqmj5j1SVXs2Fc"
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

This is an OpenPGP/MIME signed message (RFC 4880 and 3156)
--bCvbWEJMulW7EirtrJbTqmj5j1SVXs2Fc
Content-Type: text/plain; charset=windows-1252
Content-Transfer-Encoding: quoted-printable

On 07.11.2014 11:44, David Vrabel wrote:
> On 06/11/14 21:49, Seth Forshee wrote:
>> We've had several reports of hitting the following BUG_ON in
>> xennet_make_frags with 3.2 and 3.13 kernels (I'm currently awaiting
>> results of testing with 3.17):
>>
>>         /* Grant backend access to each skb fragment page. */
>>         for (i =3D 0; i < frags; i++) {
>>                 skb_frag_t *frag =3D skb_shinfo(skb)->frags + i;
>>                 struct page *page =3D skb_frag_page(frag);
>>
>>                 len =3D skb_frag_size(frag);
>>                 offset =3D frag->page_offset;
>>
>>                 /* Data must not cross a page boundary. */
>>                 BUG_ON(len + offset > PAGE_SIZE<<compound_order(page))=
;
>>
>> When this happens the page in question is a "middle" page in a compoun=
d
>> page (i.e. it's a tail page but not the last tail page), and the data =
is
>> fully contained within the compound page. The data does however cross
>> the hardware page boundary, and since compound_order evaluates to 0 fo=
r
>> tail pages the check fails.
>>
>> In going over this I've been unable to determine whether the BUG_ON in=

>> xennet_make_frags is incorrect or the paged skb data is wrong. I can't=

>> find that it's documented anywhere, and the networking code itself is =
a
>> bit ambiguous when it comes to compound pages. On the one hand
>> __skb_fill_page_desc specifically handles adding tail pages as paged
>> data, but on the other hand skb_copy_bits kmaps frag->page.p which cou=
ld
>> fail with data that extends into another page.
>=20
> netfront will safely handle this case so you can remove this BUG_ON()
> (and the one later on).  But it would be better to find out were these
> funny-looking skbs are coming from and (if necessary) fixing the bug th=
ere.

Right, in fact it does ignore pages from the start of a compound page in =
case
the offset is big enough. So there is no difference between that case and=
 the
one where the page pointer from the frag is adjusted the way it was obser=
ved.

It really boils down to the question what the real rules for the frags ar=
e. If
it is "only" that data may not cross the boundary of a hw or compound pag=
e this
leaves room for interpretation. The odd skb does not violate that but a c=
heck
for conformance would become a lot more complex. And netfront is not the =
only
place that expects the frag page to be pointing to the compound head (the=
re is
igb for example, though it does only a warn_on upon failing).

On the other hand the __skb_fill_page_desc is written in a way that seems=
 to
assume the frag page pointer may be pointing to a tail page. So before "b=
laming"
one piece of code or the other we need an authoritative answer to what we=
 are
supposed to expect.

-Stefan
>=20
> David
>=20



--bCvbWEJMulW7EirtrJbTqmj5j1SVXs2Fc
Content-Type: application/pgp-signature; name="signature.asc"
Content-Description: OpenPGP digital signature
Content-Disposition: attachment; filename="signature.asc"

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1

iQIcBAEBCgAGBQJUXKgjAAoJEOhnXe7L7s6jv00P/1h8nZmWti/L+ueiOit/Uv/u
2sBrwRi96T+9pi9dEFuDZruPCOqWEBcRjQnkdmL0Id0PwjjLwNd+pKfQuOgpGDmp
9GA1yO+yxFikHEgILlmPcvdEwAvMnRhm5BuBtthYuuJ+eWcKQfWJXswVB+769Vy+
hTYp01pqWay7XP5LMntzz7ZtShC3exOXs5nX1R/BmkxDWlm0WxlI/ALCmNCfuvpx
RkA3wfVHqC/azyzOiuyI+82MAx/0dah2kJddKbvwUr7oC3JYrCt+asOg19nB7RML
5505YPdp79SRB5vZa/bMS8QWh8x+xA5iVcTdbJxGGKix9Ms+OHk1KYC+qqKwm/yS
VdjQ5MJlp5woGDGtl04ExyOw1NXaqaMidExGktq7rl528wtn05tvn3PT6W7LdpwM
fook6Qs56Z6F+EVi1ZTY03d4aqp5NKhb5bFaKBsDJCtz+NpOSYxwa0BQZICYnKXy
ZXj8gN0s7dd9FfxGq+AVd5JKaXT18MxpIPW+pIw83ZL2QblH02L4imf9Lo5BkV7Z
0luMQMN3rsXNqINrbOa/JTrEyJwn0V2D4UwOrdJrsqN6Q1mx3Sqaa2QhVNYbBAtb
CdY8SyicB3sD8UaVlHqTB1jb2F6klst5ZTftxyDC9GT/088Zjb5AEqyeCUpItuI+
590GautyGVnF60sEHMK4
=VJCZ
-----END PGP SIGNATURE-----

--bCvbWEJMulW7EirtrJbTqmj5j1SVXs2Fc--

From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S1751904AbaKGLWG (ORCPT <rfc822;w@1wt.eu>);
	Fri, 7 Nov 2014 06:22:06 -0500
Received: from mail-ie0-f182.google.com ([209.85.223.182]:38086 "EHLO
	mail-ie0-f182.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with ESMTP id S1751325AbaKGLWE (ORCPT
	<rfc822;linux-kernel@vger.kernel.org>);
	Fri, 7 Nov 2014 06:22:04 -0500
Message-ID: <1415359320.13896.105.camel@edumazet-glaptop2.roam.corp.google.com>
Subject: Re: [Xen-devel] BUG in xennet_make_frags with paged skb data
From: Eric Dumazet <eric.dumazet@gmail.com>
To: Zoltan Kiss <zoltan.kiss@linaro.org>
Cc: netdev@vger.kernel.org, "David S. Miller" <davem@davemloft.net>,
        Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>,
        Boris Ostrovsky <boris.ostrovsky@oracle.com>,
        David Vrabel <david.vrabel@citrix.com>,
        Stefan Bader <stefan.bader@canonical.com>,
        Jay Vosburgh <jay.vosburgh@canonical.com>,
        linux-kernel@vger.kernel.org, xen-devel@lists.xenproject.org
Date: Fri, 07 Nov 2014 03:22:00 -0800
In-Reply-To: <545C9013.1090406@linaro.org>
References: <20141106214940.GD44162@ubuntu-hedt>
	 <545C9013.1090406@linaro.org>
Content-Type: text/plain; charset="UTF-8"
X-Mailer: Evolution 3.10.4-0ubuntu2 
Mime-Version: 1.0
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Fri, 2014-11-07 at 09:25 +0000, Zoltan Kiss wrote:

Please do not top post.

> Hi,
> 
> AFAIK in this scenario your skb frag is wrong. The page pointer should 
> point to the original compound page (not a member of it), and offset 
> should be set accordingly.
> For example, if your compound page is 16K (4 page), then the page 
> pointer should point to the first page, and if the data starts at the 
> 3rd page, then offset should be >8K

This is not accurate.

This BUG_ON() is wrong.

It should instead be :

BUG_ON(len + offset > PAGE_SIZE<<compound_order(compound_head(page)));

splice() code can generate such cases.



From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S1752290AbaKGMQK (ORCPT <rfc822;w@1wt.eu>);
	Fri, 7 Nov 2014 07:16:10 -0500
Received: from youngberry.canonical.com ([91.189.89.112]:34160 "EHLO
	youngberry.canonical.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with ESMTP id S1752133AbaKGMQE (ORCPT
	<rfc822;linux-kernel@vger.kernel.org>);
	Fri, 7 Nov 2014 07:16:04 -0500
Message-ID: <545CB7FF.8080003@canonical.com>
Date: Fri, 07 Nov 2014 13:15:59 +0100
From: Stefan Bader <stefan.bader@canonical.com>
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.2.0
MIME-Version: 1.0
To: Eric Dumazet <eric.dumazet@gmail.com>,
        Zoltan Kiss <zoltan.kiss@linaro.org>
CC: netdev@vger.kernel.org, "David S. Miller" <davem@davemloft.net>,
        Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>,
        Boris Ostrovsky <boris.ostrovsky@oracle.com>,
        David Vrabel <david.vrabel@citrix.com>,
        Jay Vosburgh <jay.vosburgh@canonical.com>,
        linux-kernel@vger.kernel.org, xen-devel@lists.xenproject.org
Subject: Re: [Xen-devel] BUG in xennet_make_frags with paged skb data
References: <20141106214940.GD44162@ubuntu-hedt>	 <545C9013.1090406@linaro.org> <1415359320.13896.105.camel@edumazet-glaptop2.roam.corp.google.com>
In-Reply-To: <1415359320.13896.105.camel@edumazet-glaptop2.roam.corp.google.com>
Content-Type: multipart/signed; micalg=pgp-sha512;
 protocol="application/pgp-signature";
 boundary="opLDNHQ8k3gaC4DnE7Cxm0op4Sxl5uidl"
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

This is an OpenPGP/MIME signed message (RFC 4880 and 3156)
--opLDNHQ8k3gaC4DnE7Cxm0op4Sxl5uidl
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable

On 07.11.2014 12:22, Eric Dumazet wrote:
> On Fri, 2014-11-07 at 09:25 +0000, Zoltan Kiss wrote:
>=20
> Please do not top post.
>=20
>> Hi,
>>
>> AFAIK in this scenario your skb frag is wrong. The page pointer should=
=20
>> point to the original compound page (not a member of it), and offset=20
>> should be set accordingly.
>> For example, if your compound page is 16K (4 page), then the page=20
>> pointer should point to the first page, and if the data starts at the =

>> 3rd page, then offset should be >8K
>=20
> This is not accurate.
>=20
> This BUG_ON() is wrong.
>=20
> It should instead be :
>=20
> BUG_ON(len + offset > PAGE_SIZE<<compound_order(compound_head(page)));

would that not have to be

BUG_ON((page-compound_head(page)*PAGE_SIZE)+offset+len >
PAGE_SIZE<<compound_order(compound_head(page)));

since offset is adjusted to start from the tail page in that case.
>=20
> splice() code can generate such cases.
>=20
>=20



--opLDNHQ8k3gaC4DnE7Cxm0op4Sxl5uidl
Content-Type: application/pgp-signature; name="signature.asc"
Content-Description: OpenPGP digital signature
Content-Disposition: attachment; filename="signature.asc"

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1

iQIcBAEBCgAGBQJUXLf/AAoJEOhnXe7L7s6jwO4QANXShxZX4Btb/XdJqqqOkOyZ
5yU4K4RucnZCx3x6XZp2QCRtNLThkUeIjQJg5n8SbGrAUYIPceuECAN4HeMIPKDC
GcV/g/o4fy/rklSNaHx+ovZo4fr6vvK6ddN8kdh9S2wydD3z6oLpBIvpFAG5zj2a
YfLa/VZJl/YBGcl/NgW02z+XsNyAn0chwVVk5sQiHY/+dQpbSW9t2sF9fKmevAlN
mmY2K+nTFSE5+urKuNZWBkbBdz+hJayyociJlD6E3OWWa6211LV+ZtUhdzJAX+6C
Galm0Tx18tSg3+RDIZkodUgup6D4rpCxR385CmXSllFT3ZO+ZY2qa6bQD0qGPBR+
KFQ3W4N66auQ/onV0RwxCMCC6oj0yIldCYjq+gQPPeJG2VPiSLAyw8vdr5tQalzD
auqXimkvj/9CsRyihQLbkq7qrhv5zeqb7THLxJQlG2SkKuwwZMGTkV5P9F4rYPxO
1ZKGoXe+5lPJgO18haH2fKNztD/dsR4OkTk6QHausxzRmKIe8d2AIuwJsAwG4X+V
mts94bySYCUY9lJUd6sh8qVtTRShg3QV2GtzTyZ0sU0srSaZEJkpWjHRJ4vqKexF
ovRi8EU5Mt4svRVi3nasM3knvG8ZWsdsxQ57HHIR6fQiPiDuke+9VfCZYl4WePNF
rWCPML3R458kxlDrhr0A
=Jb9a
-----END PGP SIGNATURE-----

--opLDNHQ8k3gaC4DnE7Cxm0op4Sxl5uidl--

From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S1752262AbaKGMVJ (ORCPT <rfc822;w@1wt.eu>);
	Fri, 7 Nov 2014 07:21:09 -0500
Received: from mail-wi0-f170.google.com ([209.85.212.170]:35609 "EHLO
	mail-wi0-f170.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with ESMTP id S1752074AbaKGMVF (ORCPT
	<rfc822;linux-kernel@vger.kernel.org>);
	Fri, 7 Nov 2014 07:21:05 -0500
Message-ID: <545CB92E.9050106@linaro.org>
Date: Fri, 07 Nov 2014 12:21:02 +0000
From: Zoltan Kiss <zoltan.kiss@linaro.org>
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.2.0
MIME-Version: 1.0
To: Stefan Bader <stefan.bader@canonical.com>,
        Eric Dumazet <eric.dumazet@gmail.com>
CC: netdev@vger.kernel.org, "David S. Miller" <davem@davemloft.net>,
        Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>,
        Boris Ostrovsky <boris.ostrovsky@oracle.com>,
        David Vrabel <david.vrabel@citrix.com>,
        Jay Vosburgh <jay.vosburgh@canonical.com>,
        linux-kernel@vger.kernel.org, xen-devel@lists.xenproject.org
Subject: Re: [Xen-devel] BUG in xennet_make_frags with paged skb data
References: <20141106214940.GD44162@ubuntu-hedt>	 <545C9013.1090406@linaro.org> <1415359320.13896.105.camel@edumazet-glaptop2.roam.corp.google.com> <545CB7FF.8080003@canonical.com>
In-Reply-To: <545CB7FF.8080003@canonical.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org



On 07/11/14 12:15, Stefan Bader wrote:
> On 07.11.2014 12:22, Eric Dumazet wrote:
>> On Fri, 2014-11-07 at 09:25 +0000, Zoltan Kiss wrote:
>>
>> Please do not top post.
>>
>>> Hi,
>>>
>>> AFAIK in this scenario your skb frag is wrong. The page pointer should
>>> point to the original compound page (not a member of it), and offset
>>> should be set accordingly.
>>> For example, if your compound page is 16K (4 page), then the page
>>> pointer should point to the first page, and if the data starts at the
>>> 3rd page, then offset should be >8K
>>
>> This is not accurate.
>>
>> This BUG_ON() is wrong.
>>
>> It should instead be :
>>
>> BUG_ON(len + offset > PAGE_SIZE<<compound_order(compound_head(page)));
>
> would that not have to be
>
> BUG_ON((page-compound_head(page)*PAGE_SIZE)+offset+len >
> PAGE_SIZE<<compound_order(compound_head(page)));

There should be a parentheses around "page-compound_head(page)".
>
> since offset is adjusted to start from the tail page in that case.
>>
>> splice() code can generate such cases.
>>
>>
>
>

From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S1752340AbaKGM2i (ORCPT <rfc822;w@1wt.eu>);
	Fri, 7 Nov 2014 07:28:38 -0500
Received: from youngberry.canonical.com ([91.189.89.112]:34220 "EHLO
	youngberry.canonical.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with ESMTP id S1751325AbaKGM2g (ORCPT
	<rfc822;linux-kernel@vger.kernel.org>);
	Fri, 7 Nov 2014 07:28:36 -0500
Message-ID: <545CBAF1.4020207@canonical.com>
Date: Fri, 07 Nov 2014 13:28:33 +0100
From: Stefan Bader <stefan.bader@canonical.com>
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.2.0
MIME-Version: 1.0
To: Zoltan Kiss <zoltan.kiss@linaro.org>,
        Eric Dumazet <eric.dumazet@gmail.com>
CC: netdev@vger.kernel.org, "David S. Miller" <davem@davemloft.net>,
        Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>,
        Boris Ostrovsky <boris.ostrovsky@oracle.com>,
        David Vrabel <david.vrabel@citrix.com>,
        Jay Vosburgh <jay.vosburgh@canonical.com>,
        linux-kernel@vger.kernel.org, xen-devel@lists.xenproject.org
Subject: Re: [Xen-devel] BUG in xennet_make_frags with paged skb data
References: <20141106214940.GD44162@ubuntu-hedt>	 <545C9013.1090406@linaro.org> <1415359320.13896.105.camel@edumazet-glaptop2.roam.corp.google.com> <545CB7FF.8080003@canonical.com> <545CB92E.9050106@linaro.org>
In-Reply-To: <545CB92E.9050106@linaro.org>
Content-Type: multipart/signed; micalg=pgp-sha512;
 protocol="application/pgp-signature";
 boundary="0ViuMnpe8U2G0Vr4XJacPpStuwjAKOr1S"
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

This is an OpenPGP/MIME signed message (RFC 4880 and 3156)
--0ViuMnpe8U2G0Vr4XJacPpStuwjAKOr1S
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable

On 07.11.2014 13:21, Zoltan Kiss wrote:
>=20
>=20
> On 07/11/14 12:15, Stefan Bader wrote:
>> On 07.11.2014 12:22, Eric Dumazet wrote:
>>> On Fri, 2014-11-07 at 09:25 +0000, Zoltan Kiss wrote:
>>>
>>> Please do not top post.
>>>
>>>> Hi,
>>>>
>>>> AFAIK in this scenario your skb frag is wrong. The page pointer shou=
ld
>>>> point to the original compound page (not a member of it), and offset=

>>>> should be set accordingly.
>>>> For example, if your compound page is 16K (4 page), then the page
>>>> pointer should point to the first page, and if the data starts at th=
e
>>>> 3rd page, then offset should be >8K
>>>
>>> This is not accurate.
>>>
>>> This BUG_ON() is wrong.
>>>
>>> It should instead be :
>>>
>>> BUG_ON(len + offset > PAGE_SIZE<<compound_order(compound_head(page)))=
;
>>
>> would that not have to be
>>
>> BUG_ON((page-compound_head(page)*PAGE_SIZE)+offset+len >
>> PAGE_SIZE<<compound_order(compound_head(page)));
>=20
> There should be a parentheses around "page-compound_head(page)".

*sigh* before submitting anything I'll have to make sure I run it past th=
e
compiler at least. I never manager to type the beast without some error.

But you got the drift...

>>
>> since offset is adjusted to start from the tail page in that case.
>>>
>>> splice() code can generate such cases.
>>>
>>>
>>
>>



--0ViuMnpe8U2G0Vr4XJacPpStuwjAKOr1S
Content-Type: application/pgp-signature; name="signature.asc"
Content-Description: OpenPGP digital signature
Content-Disposition: attachment; filename="signature.asc"

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1

iQIcBAEBCgAGBQJUXLrxAAoJEOhnXe7L7s6jWHkQALuMGqlxHCK1CKo30aofmUP9
hSqbVSEoqL8ahwPovgbA7aNsPfGgpoy1LM0xw1f/1nFWaGaN1YpMoS9MFqYOWK7n
SYl9sNztzG14uyQtGFyzLHhDkwCopkDqeDPYwcPbHN0knQfvZaTs47pR4Rgy6+Q+
bzvBN1aOe2wl1CC1h0Lbd8zkiXH29tGVJqgcWKnEo6Dwr/7SElujLCjXsAdm77AV
jdvdl2jEz6X2K4aXuHyUFkUtZ1DKkoHDP+RPNDxS9qyuS7UrjCMWf8n8Q0ieCWnl
0JLOA5mD79jj74md1N49XOieQMMivv/vjaJ9WrAN6u1SNpjM28ieqcXz85Yu9WXC
vQ2EkxEEWMJ7rN+Cqhk1kkFjKwL5ZEytlmtAFJhkoJbqn9Xyz+bUjG/Xsizyu+rK
JFgDYGqCd4iUl/JON4gPlk9Cclw2zeFb5yAD0rTv6tLakI3iJhnkP5z91cYPTICS
qZF0zWfhJQkgtZPKCeilTT8zWtrfpeNMK8awWUL9EAPzQ64LeJEgV9P7vo4mQNfG
XPGAbb/Rv4SKAsqTy7DYFNCnT0gEO/5Vr2LBD4/LXLtzpW9Lc1ZKzLs5IfYsYdGj
Jh6kIcAm4wWekaZqX61TKIjfwFVZ5buuZoZssmIKAtqsvxQGD+TcCgp601biRGP/
tcJMgLiZQiHGn6BCvZJl
=FOiu
-----END PGP SIGNATURE-----

--0ViuMnpe8U2G0Vr4XJacPpStuwjAKOr1S--

From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S1752664AbaKGO5C (ORCPT <rfc822;w@1wt.eu>);
	Fri, 7 Nov 2014 09:57:02 -0500
Received: from mail-wi0-f169.google.com ([209.85.212.169]:50675 "EHLO
	mail-wi0-f169.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with ESMTP id S1752558AbaKGO47 (ORCPT
	<rfc822;linux-kernel@vger.kernel.org>);
	Fri, 7 Nov 2014 09:56:59 -0500
Message-ID: <545CDC54.9050305@linaro.org>
Date: Fri, 07 Nov 2014 14:51:00 +0000
From: Zoltan Kiss <zoltan.kiss@linaro.org>
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.2.0
MIME-Version: 1.0
To: Stefan Bader <stefan.bader@canonical.com>,
        David Vrabel <david.vrabel@citrix.com>, netdev@vger.kernel.org,
        "David S. Miller" <davem@davemloft.net>,
        Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>,
        Boris Ostrovsky <boris.ostrovsky@oracle.com>,
        Jay Vosburgh <jay.vosburgh@canonical.com>,
        linux-kernel@vger.kernel.org, xen-devel@lists.xenproject.org
Subject: Re: [Xen-devel] BUG in xennet_make_frags with paged skb data
References: <20141106214940.GD44162@ubuntu-hedt> <545CA27F.4070400@citrix.com> <545CA823.2080307@canonical.com>
In-Reply-To: <545CA823.2080307@canonical.com>
Content-Type: text/plain; charset=windows-1252; format=flowed
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Hi,

On 07/11/14 11:08, Stefan Bader wrote:
> On 07.11.2014 11:44, David Vrabel wrote:
>> On 06/11/14 21:49, Seth Forshee wrote:
>>> We've had several reports of hitting the following BUG_ON in
>>> xennet_make_frags with 3.2 and 3.13 kernels (I'm currently awaiting
>>> results of testing with 3.17):
>>>
>>>          /* Grant backend access to each skb fragment page. */
>>>          for (i = 0; i < frags; i++) {
>>>                  skb_frag_t *frag = skb_shinfo(skb)->frags + i;
>>>                  struct page *page = skb_frag_page(frag);
>>>
>>>                  len = skb_frag_size(frag);
>>>                  offset = frag->page_offset;
>>>
>>>                  /* Data must not cross a page boundary. */
>>>                  BUG_ON(len + offset > PAGE_SIZE<<compound_order(page));
>>>
>>> When this happens the page in question is a "middle" page in a compound
>>> page (i.e. it's a tail page but not the last tail page), and the data is
>>> fully contained within the compound page. The data does however cross
>>> the hardware page boundary, and since compound_order evaluates to 0 for
>>> tail pages the check fails.
>>>
>>> In going over this I've been unable to determine whether the BUG_ON in
>>> xennet_make_frags is incorrect or the paged skb data is wrong. I can't
>>> find that it's documented anywhere, and the networking code itself is a
>>> bit ambiguous when it comes to compound pages. On the one hand
>>> __skb_fill_page_desc specifically handles adding tail pages as paged
>>> data, but on the other hand skb_copy_bits kmaps frag->page.p which could
>>> fail with data that extends into another page.
>>
>> netfront will safely handle this case so you can remove this BUG_ON()
>> (and the one later on).  But it would be better to find out were these
>> funny-looking skbs are coming from and (if necessary) fixing the bug there.
>
> Right, in fact it does ignore pages from the start of a compound page in case
> the offset is big enough. So there is no difference between that case and the
> one where the page pointer from the frag is adjusted the way it was observed.
>
> It really boils down to the question what the real rules for the frags are. If
> it is "only" that data may not cross the boundary of a hw or compound page this
> leaves room for interpretation. The odd skb does not violate that but a check
> for conformance would become a lot more complex. And netfront is not the only
> place that expects the frag page to be pointing to the compound head (there is
> igb for example, though it does only a warn_on upon failing).
>
> On the other hand the __skb_fill_page_desc is written in a way that seems to
> assume the frag page pointer may be pointing to a tail page. So before "blaming"
> one piece of code or the other we need an authoritative answer to what we are
> supposed to expect.

Looking at skb_copy_bits and kmap_atomic it looks confusing indeed. It 
seems kmap_atomic maps only the actual page, not the whole compound, and 
it doesn't matter whether you gave a head or a tail page to it. 
Therefore if skb_copy_bits comes over a frag where the buffer exceeds 
the page the pointer points to, it shouldn't work.
This raises a few question in me:

- I assume frags are allowed to have compound pages, where page.p points 
to the head, and the buffer can be anywhere in the compound, e.g. offset 
= 9000 and len = 4000 is allowed (if the compound is more than four 4k 
pages, of course). Is this assumption correct?
- Is it then allowed to have page.p pointed to a tail page of the 
compound? To use the above example: page.p points to the third page, and 
offset is 808. Or pointer points to the second page, and offset is 4904. 
(the answer to the above two question should be documented somewhere, or 
codified)
- how does it works with skb_copy_bits? kmap_atomic seems to map only 
the actual page, not the whole compound.

Zoli

From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S1753133AbaKJOfb (ORCPT <rfc822;w@1wt.eu>);
	Mon, 10 Nov 2014 09:35:31 -0500
Received: from mail-ob0-f169.google.com ([209.85.214.169]:53492 "EHLO
	mail-ob0-f169.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with ESMTP id S1752689AbaKJOf2 (ORCPT
	<rfc822;linux-kernel@vger.kernel.org>);
	Mon, 10 Nov 2014 09:35:28 -0500
Date: Mon, 10 Nov 2014 08:35:17 -0600
From: Seth Forshee <seth.forshee@canonical.com>
To: David Vrabel <david.vrabel@citrix.com>
Cc: netdev@vger.kernel.org, "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <eric.dumazet@gmail.com>,
        Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>,
        Boris Ostrovsky <boris.ostrovsky@oracle.com>,
        Stefan Bader <stefan.bader@canonical.com>,
        Jay Vosburgh <jay.vosburgh@canonical.com>,
        linux-kernel@vger.kernel.org, xen-devel@lists.xenproject.org,
        seth.forshee@canonical.com
Subject: Re: BUG in xennet_make_frags with paged skb data
Message-ID: <20141110143517.GA74005@ubuntu-hedt>
Mail-Followup-To: David Vrabel <david.vrabel@citrix.com>,
	netdev@vger.kernel.org, "David S. Miller" <davem@davemloft.net>,
	Eric Dumazet <eric.dumazet@gmail.com>,
	Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>,
	Boris Ostrovsky <boris.ostrovsky@oracle.com>,
	Stefan Bader <stefan.bader@canonical.com>,
	Jay Vosburgh <jay.vosburgh@canonical.com>,
	linux-kernel@vger.kernel.org, xen-devel@lists.xenproject.org
References: <20141106214940.GD44162@ubuntu-hedt>
 <545CA27F.4070400@citrix.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <545CA27F.4070400@citrix.com>
User-Agent: Mutt/1.5.21 (2010-09-15)
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Fri, Nov 07, 2014 at 10:44:15AM +0000, David Vrabel wrote:
> On 06/11/14 21:49, Seth Forshee wrote:
> > We've had several reports of hitting the following BUG_ON in
> > xennet_make_frags with 3.2 and 3.13 kernels (I'm currently awaiting
> > results of testing with 3.17):
> > 
> >         /* Grant backend access to each skb fragment page. */
> >         for (i = 0; i < frags; i++) {
> >                 skb_frag_t *frag = skb_shinfo(skb)->frags + i;
> >                 struct page *page = skb_frag_page(frag);
> > 
> >                 len = skb_frag_size(frag);
> >                 offset = frag->page_offset;
> > 
> >                 /* Data must not cross a page boundary. */
> >                 BUG_ON(len + offset > PAGE_SIZE<<compound_order(page));
> > 
> > When this happens the page in question is a "middle" page in a compound
> > page (i.e. it's a tail page but not the last tail page), and the data is
> > fully contained within the compound page. The data does however cross
> > the hardware page boundary, and since compound_order evaluates to 0 for
> > tail pages the check fails.
> > 
> > In going over this I've been unable to determine whether the BUG_ON in
> > xennet_make_frags is incorrect or the paged skb data is wrong. I can't
> > find that it's documented anywhere, and the networking code itself is a
> > bit ambiguous when it comes to compound pages. On the one hand
> > __skb_fill_page_desc specifically handles adding tail pages as paged
> > data, but on the other hand skb_copy_bits kmaps frag->page.p which could
> > fail with data that extends into another page.
> 
> netfront will safely handle this case so you can remove this BUG_ON()
> (and the one later on).  But it would be better to find out were these
> funny-looking skbs are coming from and (if necessary) fixing the bug there.

There still seems to be disagreement about whether the "funny" skb is
valid though - you imply it isn't, but Eric says it is. I've been trying
to track down where these skbs originate, and so far I've determined
that they come from a socket spliced to a pipe spliced to a socket. It
looks like the particular page/offset/len tuple originates at least as
far back as the first socket, as the tuple is simply copied from an skb
into the pipe and from the pipe into the final skb.

Anyway, it looks like at minimum it should be safe to change the first
BUG_ON to assert that the data is fully within the compound page as
Stefan suggested, then remove the second BUG_ON entirely. This is the
path I plan to pursue unless someone objects.

Thanks,
Seth

From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S1752977AbaKJOls (ORCPT <rfc822;w@1wt.eu>);
	Mon, 10 Nov 2014 09:41:48 -0500
Received: from smtp.citrix.com ([66.165.176.89]:33505 "EHLO SMTP.CITRIX.COM"
	rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
	id S1752543AbaKJOlr (ORCPT <rfc822;linux-kernel@vger.kernel.org>);
	Mon, 10 Nov 2014 09:41:47 -0500
X-IronPort-AV: E=Sophos;i="5.07,353,1413244800"; 
   d="scan'208";a="189760408"
Message-ID: <5460CEA5.3070201@citrix.com>
Date: Mon, 10 Nov 2014 14:41:41 +0000
From: David Vrabel <david.vrabel@citrix.com>
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Icedove/24.5.0
MIME-Version: 1.0
To: <netdev@vger.kernel.org>, "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <eric.dumazet@gmail.com>,
        Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>,
        Boris Ostrovsky <boris.ostrovsky@oracle.com>,
        Stefan Bader <stefan.bader@canonical.com>,
        Jay Vosburgh <jay.vosburgh@canonical.com>,
        <linux-kernel@vger.kernel.org>, <xen-devel@lists.xenproject.org>
Subject: Re: BUG in xennet_make_frags with paged skb data
References: <20141106214940.GD44162@ubuntu-hedt> <545CA27F.4070400@citrix.com> <20141110143517.GA74005@ubuntu-hedt>
In-Reply-To: <20141110143517.GA74005@ubuntu-hedt>
Content-Type: text/plain; charset="ISO-8859-1"
Content-Transfer-Encoding: 7bit
X-DLP: MIA2
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On 10/11/14 14:35, Seth Forshee wrote:
> On Fri, Nov 07, 2014 at 10:44:15AM +0000, David Vrabel wrote:
>> On 06/11/14 21:49, Seth Forshee wrote:
>>> We've had several reports of hitting the following BUG_ON in
>>> xennet_make_frags with 3.2 and 3.13 kernels (I'm currently awaiting
>>> results of testing with 3.17):
>>>
>>>         /* Grant backend access to each skb fragment page. */
>>>         for (i = 0; i < frags; i++) {
>>>                 skb_frag_t *frag = skb_shinfo(skb)->frags + i;
>>>                 struct page *page = skb_frag_page(frag);
>>>
>>>                 len = skb_frag_size(frag);
>>>                 offset = frag->page_offset;
>>>
>>>                 /* Data must not cross a page boundary. */
>>>                 BUG_ON(len + offset > PAGE_SIZE<<compound_order(page));
>>>
>>> When this happens the page in question is a "middle" page in a compound
>>> page (i.e. it's a tail page but not the last tail page), and the data is
>>> fully contained within the compound page. The data does however cross
>>> the hardware page boundary, and since compound_order evaluates to 0 for
>>> tail pages the check fails.
>>>
>>> In going over this I've been unable to determine whether the BUG_ON in
>>> xennet_make_frags is incorrect or the paged skb data is wrong. I can't
>>> find that it's documented anywhere, and the networking code itself is a
>>> bit ambiguous when it comes to compound pages. On the one hand
>>> __skb_fill_page_desc specifically handles adding tail pages as paged
>>> data, but on the other hand skb_copy_bits kmaps frag->page.p which could
>>> fail with data that extends into another page.
>>
>> netfront will safely handle this case so you can remove this BUG_ON()
>> (and the one later on).  But it would be better to find out were these
>> funny-looking skbs are coming from and (if necessary) fixing the bug there.
> 
> There still seems to be disagreement about whether the "funny" skb is
> valid though - you imply it isn't, but Eric says it is. I've been trying
> to track down where these skbs originate, and so far I've determined
> that they come from a socket spliced to a pipe spliced to a socket. It
> looks like the particular page/offset/len tuple originates at least as
> far back as the first socket, as the tuple is simply copied from an skb
> into the pipe and from the pipe into the final skb.

Apologies for the lack of clarity.  I meant either: a) fix the producer
if these skbs are invalid; or b) remove the BUG_ON()s.  Since Eric says
these are actually valid skbs, please do option (b).

i.e., remove both BUG_ON()s.

David

From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S1753562AbaKJQjm (ORCPT <rfc822;w@1wt.eu>);
	Mon, 10 Nov 2014 11:39:42 -0500
Received: from mail-wi0-f170.google.com ([209.85.212.170]:47953 "EHLO
	mail-wi0-f170.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with ESMTP id S1752272AbaKJQjl (ORCPT
	<rfc822;linux-kernel@vger.kernel.org>);
	Mon, 10 Nov 2014 11:39:41 -0500
Message-ID: <5460EA45.2080202@linaro.org>
Date: Mon, 10 Nov 2014 16:39:33 +0000
From: Zoltan Kiss <zoltan.kiss@linaro.org>
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.2.0
MIME-Version: 1.0
To: David Vrabel <david.vrabel@citrix.com>, netdev@vger.kernel.org,
        "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <eric.dumazet@gmail.com>,
        Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>,
        Boris Ostrovsky <boris.ostrovsky@oracle.com>,
        Stefan Bader <stefan.bader@canonical.com>,
        Jay Vosburgh <jay.vosburgh@canonical.com>,
        linux-kernel@vger.kernel.org, xen-devel@lists.xenproject.org
Subject: Re: [Xen-devel] BUG in xennet_make_frags with paged skb data
References: <20141106214940.GD44162@ubuntu-hedt> <545CA27F.4070400@citrix.com>	<20141110143517.GA74005@ubuntu-hedt> <5460CEA5.3070201@citrix.com>
In-Reply-To: <5460CEA5.3070201@citrix.com>
Content-Type: text/plain; charset=windows-1252; format=flowed
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org



On 10/11/14 14:41, David Vrabel wrote:
> On 10/11/14 14:35, Seth Forshee wrote:
>> On Fri, Nov 07, 2014 at 10:44:15AM +0000, David Vrabel wrote:
>>> On 06/11/14 21:49, Seth Forshee wrote:
>>>> We've had several reports of hitting the following BUG_ON in
>>>> xennet_make_frags with 3.2 and 3.13 kernels (I'm currently awaiting
>>>> results of testing with 3.17):
>>>>
>>>>          /* Grant backend access to each skb fragment page. */
>>>>          for (i = 0; i < frags; i++) {
>>>>                  skb_frag_t *frag = skb_shinfo(skb)->frags + i;
>>>>                  struct page *page = skb_frag_page(frag);
>>>>
>>>>                  len = skb_frag_size(frag);
>>>>                  offset = frag->page_offset;
>>>>
>>>>                  /* Data must not cross a page boundary. */
>>>>                  BUG_ON(len + offset > PAGE_SIZE<<compound_order(page));
>>>>
>>>> When this happens the page in question is a "middle" page in a compound
>>>> page (i.e. it's a tail page but not the last tail page), and the data is
>>>> fully contained within the compound page. The data does however cross
>>>> the hardware page boundary, and since compound_order evaluates to 0 for
>>>> tail pages the check fails.
>>>>
>>>> In going over this I've been unable to determine whether the BUG_ON in
>>>> xennet_make_frags is incorrect or the paged skb data is wrong. I can't
>>>> find that it's documented anywhere, and the networking code itself is a
>>>> bit ambiguous when it comes to compound pages. On the one hand
>>>> __skb_fill_page_desc specifically handles adding tail pages as paged
>>>> data, but on the other hand skb_copy_bits kmaps frag->page.p which could
>>>> fail with data that extends into another page.
>>>
>>> netfront will safely handle this case so you can remove this BUG_ON()
>>> (and the one later on).  But it would be better to find out were these
>>> funny-looking skbs are coming from and (if necessary) fixing the bug there.
>>
>> There still seems to be disagreement about whether the "funny" skb is
>> valid though - you imply it isn't, but Eric says it is. I've been trying
>> to track down where these skbs originate, and so far I've determined
>> that they come from a socket spliced to a pipe spliced to a socket. It
>> looks like the particular page/offset/len tuple originates at least as
>> far back as the first socket, as the tuple is simply copied from an skb
>> into the pipe and from the pipe into the final skb.
>
> Apologies for the lack of clarity.  I meant either: a) fix the producer
> if these skbs are invalid; or b) remove the BUG_ON()s.  Since Eric says
> these are actually valid skbs, please do option (b).
>
> i.e., remove both BUG_ON()s.

The BUG_ON suggested by Stefan would be still reasonable:

BUG_ON(((page-compound_head(page))*PAGE_SIZE)+offset+len >
PAGE_SIZE<<compound_order(compound_head(page)));
>
> David
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
>

From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S1753914AbaKJQmu (ORCPT <rfc822;w@1wt.eu>);
	Mon, 10 Nov 2014 11:42:50 -0500
Received: from smtp02.citrix.com ([66.165.176.63]:30622 "EHLO
	SMTP02.CITRIX.COM" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with ESMTP id S1753401AbaKJQmr (ORCPT
	<rfc822;linux-kernel@vger.kernel.org>);
	Mon, 10 Nov 2014 11:42:47 -0500
X-IronPort-AV: E=Sophos;i="5.07,353,1413244800"; 
   d="scan'208";a="191251201"
Message-ID: <5460EAF1.5020903@citrix.com>
Date: Mon, 10 Nov 2014 16:42:25 +0000
From: David Vrabel <david.vrabel@citrix.com>
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Icedove/24.5.0
MIME-Version: 1.0
To: Zoltan Kiss <zoltan.kiss@linaro.org>, <netdev@vger.kernel.org>,
        "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <eric.dumazet@gmail.com>,
        Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>,
        Boris Ostrovsky <boris.ostrovsky@oracle.com>,
        Stefan Bader <stefan.bader@canonical.com>,
        Jay Vosburgh <jay.vosburgh@canonical.com>,
        <linux-kernel@vger.kernel.org>, <xen-devel@lists.xenproject.org>
Subject: Re: [Xen-devel] BUG in xennet_make_frags with paged skb data
References: <20141106214940.GD44162@ubuntu-hedt> <545CA27F.4070400@citrix.com>	<20141110143517.GA74005@ubuntu-hedt> <5460CEA5.3070201@citrix.com> <5460EA45.2080202@linaro.org>
In-Reply-To: <5460EA45.2080202@linaro.org>
Content-Type: text/plain; charset="windows-1252"
Content-Transfer-Encoding: 7bit
X-DLP: MIA2
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On 10/11/14 16:39, Zoltan Kiss wrote:
> 
> The BUG_ON suggested by Stefan would be still reasonable:
> 
> BUG_ON(((page-compound_head(page))*PAGE_SIZE)+offset+len >
> PAGE_SIZE<<compound_order(compound_head(page)));

Well, it wouldn't trigger but I don't think it is useful any more.

David

From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S1753532AbaKJRCl (ORCPT <rfc822;w@1wt.eu>);
	Mon, 10 Nov 2014 12:02:41 -0500
Received: from mail-ie0-f172.google.com ([209.85.223.172]:39047 "EHLO
	mail-ie0-f172.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with ESMTP id S1752244AbaKJRCj (ORCPT
	<rfc822;linux-kernel@vger.kernel.org>);
	Mon, 10 Nov 2014 12:02:39 -0500
Message-ID: <1415638955.9613.4.camel@edumazet-glaptop2.roam.corp.google.com>
Subject: Re: [Xen-devel] BUG in xennet_make_frags with paged skb data
From: Eric Dumazet <eric.dumazet@gmail.com>
To: David Vrabel <david.vrabel@citrix.com>
Cc: Zoltan Kiss <zoltan.kiss@linaro.org>, netdev@vger.kernel.org,
        "David S. Miller" <davem@davemloft.net>,
        Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>,
        Boris Ostrovsky <boris.ostrovsky@oracle.com>,
        Stefan Bader <stefan.bader@canonical.com>,
        Jay Vosburgh <jay.vosburgh@canonical.com>,
        linux-kernel@vger.kernel.org, xen-devel@lists.xenproject.org
Date: Mon, 10 Nov 2014 09:02:35 -0800
In-Reply-To: <5460EAF1.5020903@citrix.com>
References: <20141106214940.GD44162@ubuntu-hedt>
	 <545CA27F.4070400@citrix.com>	<20141110143517.GA74005@ubuntu-hedt>
	 <5460CEA5.3070201@citrix.com> <5460EA45.2080202@linaro.org>
	 <5460EAF1.5020903@citrix.com>
Content-Type: text/plain; charset="UTF-8"
X-Mailer: Evolution 3.10.4-0ubuntu2 
Mime-Version: 1.0
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Mon, 2014-11-10 at 16:42 +0000, David Vrabel wrote:
> On 10/11/14 16:39, Zoltan Kiss wrote:
> > 
> > The BUG_ON suggested by Stefan would be still reasonable:
> > 
> > BUG_ON(((page-compound_head(page))*PAGE_SIZE)+offset+len >
> > PAGE_SIZE<<compound_order(compound_head(page)));
> 
> Well, it wouldn't trigger but I don't think it is useful any more.

Right.

This BUG_ON() does not make sense (its current implementation is
assuming a very precise layout anyway)

If we really wanted some debugging, we would need something more generic
and done in core networking stack, not in a particular driver.



From mboxrd@z Thu Jan  1 00:00:00 1970
From: Frediano Ziglio <freddy77@gmail.com>
Subject: Re: BUG in xennet_make_frags with paged skb data
Date: Mon, 10 Nov 2014 17:20:03 +0000
Message-ID: <CAHt6W4fFd17y0gXO9o=rrc86ErYNe_gcvvuc94j31ZrpR4huog@mail.gmail.com>
References: <20141106214940.GD44162@ubuntu-hedt> <545CA27F.4070400@citrix.com>
	<20141110143517.GA74005@ubuntu-hedt> <5460CEA5.3070201@citrix.com>
	<5460EA45.2080202@linaro.org> <5460EAF1.5020903@citrix.com>
Mime-Version: 1.0
Content-Type: multipart/mixed; boundary="===============5083918228855888436=="
Cc: Zoltan Kiss <zoltan.kiss@linaro.org>, Eric Dumazet <eric.dumazet@gmail.com>,
	netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
	Stefan Bader <stefan.bader@canonical.com>, xen-devel@lists.xenproject.org,
	Jay Vosburgh <jay.vosburgh@canonical.com>,
	Boris Ostrovsky <boris.ostrovsky@oracle.com>,
	"David S. Miller" <davem@davemloft.net>
To: David Vrabel <david.vrabel@citrix.com>
Return-path: <xen-devel-bounces@lists.xen.org>
In-Reply-To: <5460EAF1.5020903@citrix.com>
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
List-Id: netdev.vger.kernel.org

--===============5083918228855888436==
Content-Type: multipart/alternative; boundary=001a1136afd890138c0507845f7a

--001a1136afd890138c0507845f7a
Content-Type: text/plain; charset=UTF-8

2014-11-10 16:42 GMT+00:00 David Vrabel <david.vrabel@citrix.com>:

> On 10/11/14 16:39, Zoltan Kiss wrote:
> >
> > The BUG_ON suggested by Stefan would be still reasonable:
> >
> > BUG_ON(((page-compound_head(page))*PAGE_SIZE)+offset+len >
> > PAGE_SIZE<<compound_order(compound_head(page)));
>
> Well, it wouldn't trigger but I don't think it is useful any more.
>
> David
>
>
Looks like this structure was designed to just contains physical single
pages and was extended to support compound pages however there are still
some functions that assume the usage of single pages. Just for instance the
offset/size of fragments are computed from PAGE_SIZE. On x86 (4KB pages) if
your compound page is bigger than 64KB you cannot fully use for a fragment
as offset/size are 16 bits. Some functions in skbuff.c use kmap which is
limited to physical page.

Frediano

--001a1136afd890138c0507845f7a
Content-Type: text/html; charset=UTF-8
Content-Transfer-Encoding: quoted-printable

<div dir=3D"ltr"><div class=3D"gmail_extra"><div class=3D"gmail_quote">2014=
-11-10 16:42 GMT+00:00 David Vrabel <span dir=3D"ltr">&lt;<a href=3D"mailto=
:david.vrabel@citrix.com" target=3D"_blank">david.vrabel@citrix.com</a>&gt;=
</span>:<br><blockquote class=3D"gmail_quote" style=3D"margin:0 0 0 .8ex;bo=
rder-left:1px #ccc solid;padding-left:1ex"><span class=3D"">On 10/11/14 16:=
39, Zoltan Kiss wrote:<br>
&gt;<br>
&gt; The BUG_ON suggested by Stefan would be still reasonable:<br>
&gt;<br>
&gt; BUG_ON(((page-compound_head(page))*PAGE_SIZE)+offset+len &gt;<br>
&gt; PAGE_SIZE&lt;&lt;compound_order(compound_head(page)));<br>
<br>
</span>Well, it wouldn&#39;t trigger but I don&#39;t think it is useful any=
 more.<br>
<div class=3D"HOEnZb"><div class=3D"h5"><br>
David<br>
<br></div></div></blockquote><div><br></div><div>Looks like this structure =
was designed to just contains physical single pages and was extended to sup=
port compound pages however there are still some functions that assume the =
usage of single pages. Just for instance the offset/size of fragments are c=
omputed from PAGE_SIZE. On x86 (4KB pages) if your compound page is bigger =
than 64KB you cannot fully use for a fragment as offset/size are 16 bits. S=
ome functions in skbuff.c use kmap which is limited to physical page.<br></=
div><div><br>Frediano<br><br></div></div></div></div>

--001a1136afd890138c0507845f7a--


--===============5083918228855888436==
Content-Type: text/plain; charset="us-ascii"
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Content-Disposition: inline

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

--===============5083918228855888436==--

From mboxrd@z Thu Jan  1 00:00:00 1970
From: Seth Forshee <seth.forshee@canonical.com>
Subject: BUG in xennet_make_frags with paged skb data
Date: Thu, 6 Nov 2014 15:49:40 -0600
Message-ID: <20141106214940.GD44162__28578.2202300199$1415348762$gmane$org@ubuntu-hedt>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
Received: from mail6.bemta14.messagelabs.com ([193.109.254.103])
	by lists.xen.org with esmtp (Exim 4.72)
	(envelope-from <seth.forshee@canonical.com>) id 1XmUw7-00018z-Gz
	for xen-devel@lists.xenproject.org; Thu, 06 Nov 2014 21:49:47 +0000
Received: by mail-ob0-f174.google.com with SMTP id uz6so1606839obc.5
	for <xen-devel@lists.xenproject.org>;
	Thu, 06 Nov 2014 13:49:44 -0800 (PST)
Content-Disposition: inline
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: netdev@vger.kernel.org
Cc: linux-kernel@vger.kernel.org, Stefan Bader <stefan.bader@canonical.com>, seth.forshee@canonical.com, David Vrabel <david.vrabel@citrix.com>, xen-devel@lists.xenproject.org, Jay Vosburgh <jay.vosburgh@canonical.com>, Boris Ostrovsky <boris.ostrovsky@oracle.com>, "David S. Miller" <davem@davemloft.net>
List-Id: xen-devel@lists.xenproject.org

We've had several reports of hitting the following BUG_ON in
xennet_make_frags with 3.2 and 3.13 kernels (I'm currently awaiting
results of testing with 3.17):

        /* Grant backend access to each skb fragment page. */
        for (i = 0; i < frags; i++) {
                skb_frag_t *frag = skb_shinfo(skb)->frags + i;
                struct page *page = skb_frag_page(frag);

                len = skb_frag_size(frag);
                offset = frag->page_offset;

                /* Data must not cross a page boundary. */
                BUG_ON(len + offset > PAGE_SIZE<<compound_order(page));

When this happens the page in question is a "middle" page in a compound
page (i.e. it's a tail page but not the last tail page), and the data is
fully contained within the compound page. The data does however cross
the hardware page boundary, and since compound_order evaluates to 0 for
tail pages the check fails.

In going over this I've been unable to determine whether the BUG_ON in
xennet_make_frags is incorrect or the paged skb data is wrong. I can't
find that it's documented anywhere, and the networking code itself is a
bit ambiguous when it comes to compound pages. On the one hand
__skb_fill_page_desc specifically handles adding tail pages as paged
data, but on the other hand skb_copy_bits kmaps frag->page.p which could
fail with data that extends into another page.

Can anyone explain what the rules are here? My best guess based on
skb_copy_bits is that paged data should never cross the hardware page
boundary, but I'm not really sure how all of this works out when dealing
with compound pages.

Thanks,
Seth

From mboxrd@z Thu Jan  1 00:00:00 1970
From: Zoltan Kiss <zoltan.kiss@linaro.org>
Subject: Re: BUG in xennet_make_frags with paged skb data
Date: Fri, 07 Nov 2014 09:25:39 +0000
Message-ID: <545C9013.1090406__21673.49153101$1415352464$gmane$org@linaro.org>
References: <20141106214940.GD44162@ubuntu-hedt>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"; Format="flowed"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
Received: from mail6.bemta3.messagelabs.com ([195.245.230.39])
	by lists.xen.org with esmtp (Exim 4.72)
	(envelope-from <zoltan.kiss@linaro.org>) id 1Xmfnb-0005Tr-OG
	for xen-devel@lists.xenproject.org; Fri, 07 Nov 2014 09:25:43 +0000
Received: by mail-wi0-f178.google.com with SMTP id bs8so3925715wib.5
	for <xen-devel@lists.xenproject.org>;
	Fri, 07 Nov 2014 01:25:42 -0800 (PST)
In-Reply-To: <20141106214940.GD44162@ubuntu-hedt>
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: netdev@vger.kernel.org, "David S. Miller" <davem@davemloft.net>, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>, Boris Ostrovsky <boris.ostrovsky@oracle.com>, David Vrabel <david.vrabel@citrix.com>, Stefan Bader <stefan.bader@canonical.com>, Jay Vosburgh <jay.vosburgh@canonical.com>, linux-kernel@vger.kernel.org, xen-devel@lists.xenproject.org
List-Id: xen-devel@lists.xenproject.org

Hi,

AFAIK in this scenario your skb frag is wrong. The page pointer should 
point to the original compound page (not a member of it), and offset 
should be set accordingly.
For example, if your compound page is 16K (4 page), then the page 
pointer should point to the first page, and if the data starts at the 
3rd page, then offset should be >8K

Zoli

On 06/11/14 21:49, Seth Forshee wrote:
> We've had several reports of hitting the following BUG_ON in
> xennet_make_frags with 3.2 and 3.13 kernels (I'm currently awaiting
> results of testing with 3.17):
>
>          /* Grant backend access to each skb fragment page. */
>          for (i = 0; i < frags; i++) {
>                  skb_frag_t *frag = skb_shinfo(skb)->frags + i;
>                  struct page *page = skb_frag_page(frag);
>
>                  len = skb_frag_size(frag);
>                  offset = frag->page_offset;
>
>                  /* Data must not cross a page boundary. */
>                  BUG_ON(len + offset > PAGE_SIZE<<compound_order(page));
>
> When this happens the page in question is a "middle" page in a compound
> page (i.e. it's a tail page but not the last tail page), and the data is
> fully contained within the compound page. The data does however cross
> the hardware page boundary, and since compound_order evaluates to 0 for
> tail pages the check fails.
>
> In going over this I've been unable to determine whether the BUG_ON in
> xennet_make_frags is incorrect or the paged skb data is wrong. I can't
> find that it's documented anywhere, and the networking code itself is a
> bit ambiguous when it comes to compound pages. On the one hand
> __skb_fill_page_desc specifically handles adding tail pages as paged
> data, but on the other hand skb_copy_bits kmaps frag->page.p which could
> fail with data that extends into another page.
>
> Can anyone explain what the rules are here? My best guess based on
> skb_copy_bits is that paged data should never cross the hardware page
> boundary, but I'm not really sure how all of this works out when dealing
> with compound pages.
>
> Thanks,
> Seth
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
>

From mboxrd@z Thu Jan  1 00:00:00 1970
From: David Vrabel <david.vrabel@citrix.com>
Subject: Re: BUG in xennet_make_frags with paged skb data
Date: Fri, 7 Nov 2014 10:44:15 +0000
Message-ID: <545CA27F.4070400__43907.8529751443$1415357159$gmane$org@citrix.com>
References: <20141106214940.GD44162@ubuntu-hedt>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
Received: from mail6.bemta5.messagelabs.com ([195.245.231.135])
	by lists.xen.org with esmtp (Exim 4.72)
	(envelope-from <david.vrabel@citrix.com>) id 1Xmh1g-0006z0-Ey
	for xen-devel@lists.xenproject.org; Fri, 07 Nov 2014 10:44:20 +0000
In-Reply-To: <20141106214940.GD44162@ubuntu-hedt>
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: netdev@vger.kernel.org, "David S. Miller" <davem@davemloft.net>, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>, Boris Ostrovsky <boris.ostrovsky@oracle.com>, Stefan Bader <stefan.bader@canonical.com>, Jay Vosburgh <jay.vosburgh@canonical.com>, linux-kernel@vger.kernel.org, xen-devel@lists.xenproject.org
List-Id: xen-devel@lists.xenproject.org

On 06/11/14 21:49, Seth Forshee wrote:
> We've had several reports of hitting the following BUG_ON in
> xennet_make_frags with 3.2 and 3.13 kernels (I'm currently awaiting
> results of testing with 3.17):
> 
>         /* Grant backend access to each skb fragment page. */
>         for (i = 0; i < frags; i++) {
>                 skb_frag_t *frag = skb_shinfo(skb)->frags + i;
>                 struct page *page = skb_frag_page(frag);
> 
>                 len = skb_frag_size(frag);
>                 offset = frag->page_offset;
> 
>                 /* Data must not cross a page boundary. */
>                 BUG_ON(len + offset > PAGE_SIZE<<compound_order(page));
> 
> When this happens the page in question is a "middle" page in a compound
> page (i.e. it's a tail page but not the last tail page), and the data is
> fully contained within the compound page. The data does however cross
> the hardware page boundary, and since compound_order evaluates to 0 for
> tail pages the check fails.
> 
> In going over this I've been unable to determine whether the BUG_ON in
> xennet_make_frags is incorrect or the paged skb data is wrong. I can't
> find that it's documented anywhere, and the networking code itself is a
> bit ambiguous when it comes to compound pages. On the one hand
> __skb_fill_page_desc specifically handles adding tail pages as paged
> data, but on the other hand skb_copy_bits kmaps frag->page.p which could
> fail with data that extends into another page.

netfront will safely handle this case so you can remove this BUG_ON()
(and the one later on).  But it would be better to find out were these
funny-looking skbs are coming from and (if necessary) fixing the bug there.

David

From mboxrd@z Thu Jan  1 00:00:00 1970
From: Stefan Bader <stefan.bader@canonical.com>
Subject: Re: BUG in xennet_make_frags with paged skb data
Date: Fri, 07 Nov 2014 12:08:19 +0100
Message-ID: <545CA823.2080307__46490.1914973327$1415358597$gmane$org@canonical.com>
References: <20141106214940.GD44162@ubuntu-hedt> <545CA27F.4070400@citrix.com>
Mime-Version: 1.0
Content-Type: multipart/mixed; boundary="===============8721980942217411242=="
Return-path: <xen-devel-bounces@lists.xen.org>
Received: from mail6.bemta5.messagelabs.com ([195.245.231.135])
	by lists.xen.org with esmtp (Exim 4.72)
	(envelope-from <stefan.bader@canonical.com>) id 1XmhOy-0008IQ-Sr
	for xen-devel@lists.xenproject.org; Fri, 07 Nov 2014 11:08:25 +0000
In-Reply-To: <545CA27F.4070400@citrix.com>
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: David Vrabel <david.vrabel@citrix.com>, netdev@vger.kernel.org, "David S. Miller" <davem@davemloft.net>, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>, Boris Ostrovsky <boris.ostrovsky@oracle.com>, Jay Vosburgh <jay.vosburgh@canonical.com>, linux-kernel@vger.kernel.org, xen-devel@lists.xenproject.org
List-Id: xen-devel@lists.xenproject.org

This is an OpenPGP/MIME signed message (RFC 4880 and 3156)
--===============8721980942217411242==
Content-Type: multipart/signed; micalg=pgp-sha512;
 protocol="application/pgp-signature";
 boundary="bCvbWEJMulW7EirtrJbTqmj5j1SVXs2Fc"

This is an OpenPGP/MIME signed message (RFC 4880 and 3156)
--bCvbWEJMulW7EirtrJbTqmj5j1SVXs2Fc
Content-Type: text/plain; charset=windows-1252
Content-Transfer-Encoding: quoted-printable

On 07.11.2014 11:44, David Vrabel wrote:
> On 06/11/14 21:49, Seth Forshee wrote:
>> We've had several reports of hitting the following BUG_ON in
>> xennet_make_frags with 3.2 and 3.13 kernels (I'm currently awaiting
>> results of testing with 3.17):
>>
>>         /* Grant backend access to each skb fragment page. */
>>         for (i =3D 0; i < frags; i++) {
>>                 skb_frag_t *frag =3D skb_shinfo(skb)->frags + i;
>>                 struct page *page =3D skb_frag_page(frag);
>>
>>                 len =3D skb_frag_size(frag);
>>                 offset =3D frag->page_offset;
>>
>>                 /* Data must not cross a page boundary. */
>>                 BUG_ON(len + offset > PAGE_SIZE<<compound_order(page))=
;
>>
>> When this happens the page in question is a "middle" page in a compoun=
d
>> page (i.e. it's a tail page but not the last tail page), and the data =
is
>> fully contained within the compound page. The data does however cross
>> the hardware page boundary, and since compound_order evaluates to 0 fo=
r
>> tail pages the check fails.
>>
>> In going over this I've been unable to determine whether the BUG_ON in=

>> xennet_make_frags is incorrect or the paged skb data is wrong. I can't=

>> find that it's documented anywhere, and the networking code itself is =
a
>> bit ambiguous when it comes to compound pages. On the one hand
>> __skb_fill_page_desc specifically handles adding tail pages as paged
>> data, but on the other hand skb_copy_bits kmaps frag->page.p which cou=
ld
>> fail with data that extends into another page.
>=20
> netfront will safely handle this case so you can remove this BUG_ON()
> (and the one later on).  But it would be better to find out were these
> funny-looking skbs are coming from and (if necessary) fixing the bug th=
ere.

Right, in fact it does ignore pages from the start of a compound page in =
case
the offset is big enough. So there is no difference between that case and=
 the
one where the page pointer from the frag is adjusted the way it was obser=
ved.

It really boils down to the question what the real rules for the frags ar=
e. If
it is "only" that data may not cross the boundary of a hw or compound pag=
e this
leaves room for interpretation. The odd skb does not violate that but a c=
heck
for conformance would become a lot more complex. And netfront is not the =
only
place that expects the frag page to be pointing to the compound head (the=
re is
igb for example, though it does only a warn_on upon failing).

On the other hand the __skb_fill_page_desc is written in a way that seems=
 to
assume the frag page pointer may be pointing to a tail page. So before "b=
laming"
one piece of code or the other we need an authoritative answer to what we=
 are
supposed to expect.

-Stefan
>=20
> David
>=20



--bCvbWEJMulW7EirtrJbTqmj5j1SVXs2Fc
Content-Type: application/pgp-signature; name="signature.asc"
Content-Description: OpenPGP digital signature
Content-Disposition: attachment; filename="signature.asc"

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1

iQIcBAEBCgAGBQJUXKgjAAoJEOhnXe7L7s6jv00P/1h8nZmWti/L+ueiOit/Uv/u
2sBrwRi96T+9pi9dEFuDZruPCOqWEBcRjQnkdmL0Id0PwjjLwNd+pKfQuOgpGDmp
9GA1yO+yxFikHEgILlmPcvdEwAvMnRhm5BuBtthYuuJ+eWcKQfWJXswVB+769Vy+
hTYp01pqWay7XP5LMntzz7ZtShC3exOXs5nX1R/BmkxDWlm0WxlI/ALCmNCfuvpx
RkA3wfVHqC/azyzOiuyI+82MAx/0dah2kJddKbvwUr7oC3JYrCt+asOg19nB7RML
5505YPdp79SRB5vZa/bMS8QWh8x+xA5iVcTdbJxGGKix9Ms+OHk1KYC+qqKwm/yS
VdjQ5MJlp5woGDGtl04ExyOw1NXaqaMidExGktq7rl528wtn05tvn3PT6W7LdpwM
fook6Qs56Z6F+EVi1ZTY03d4aqp5NKhb5bFaKBsDJCtz+NpOSYxwa0BQZICYnKXy
ZXj8gN0s7dd9FfxGq+AVd5JKaXT18MxpIPW+pIw83ZL2QblH02L4imf9Lo5BkV7Z
0luMQMN3rsXNqINrbOa/JTrEyJwn0V2D4UwOrdJrsqN6Q1mx3Sqaa2QhVNYbBAtb
CdY8SyicB3sD8UaVlHqTB1jb2F6klst5ZTftxyDC9GT/088Zjb5AEqyeCUpItuI+
590GautyGVnF60sEHMK4
=VJCZ
-----END PGP SIGNATURE-----

--bCvbWEJMulW7EirtrJbTqmj5j1SVXs2Fc--


--===============8721980942217411242==
Content-Type: text/plain; charset="us-ascii"
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Content-Disposition: inline

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

--===============8721980942217411242==--

From mboxrd@z Thu Jan  1 00:00:00 1970
From: Eric Dumazet <eric.dumazet@gmail.com>
Subject: Re: BUG in xennet_make_frags with paged skb data
Date: Fri, 07 Nov 2014 03:22:00 -0800
Message-ID: <1415359320.13896.105.camel__40858.7746475407$1415359414$gmane$org@edumazet-glaptop2.roam.corp.google.com>
References: <20141106214940.GD44162@ubuntu-hedt> <545C9013.1090406@linaro.org>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
Received: from mail6.bemta14.messagelabs.com ([193.109.254.103])
	by lists.xen.org with esmtp (Exim 4.72)
	(envelope-from <eric.dumazet@gmail.com>) id 1XmhcD-0000Tn-7o
	for xen-devel@lists.xenproject.org; Fri, 07 Nov 2014 11:22:05 +0000
Received: by mail-ie0-f176.google.com with SMTP id rd18so4787931iec.7
	for <xen-devel@lists.xenproject.org>;
	Fri, 07 Nov 2014 03:22:02 -0800 (PST)
In-Reply-To: <545C9013.1090406@linaro.org>
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: Zoltan Kiss <zoltan.kiss@linaro.org>
Cc: netdev@vger.kernel.org, linux-kernel@vger.kernel.org, Stefan Bader <stefan.bader@canonical.com>, David Vrabel <david.vrabel@citrix.com>, xen-devel@lists.xenproject.org, Jay Vosburgh <jay.vosburgh@canonical.com>, Boris Ostrovsky <boris.ostrovsky@oracle.com>, "David S. Miller" <davem@davemloft.net>
List-Id: xen-devel@lists.xenproject.org

On Fri, 2014-11-07 at 09:25 +0000, Zoltan Kiss wrote:

Please do not top post.

> Hi,
> 
> AFAIK in this scenario your skb frag is wrong. The page pointer should 
> point to the original compound page (not a member of it), and offset 
> should be set accordingly.
> For example, if your compound page is 16K (4 page), then the page 
> pointer should point to the first page, and if the data starts at the 
> 3rd page, then offset should be >8K

This is not accurate.

This BUG_ON() is wrong.

It should instead be :

BUG_ON(len + offset > PAGE_SIZE<<compound_order(compound_head(page)));

splice() code can generate such cases.

From mboxrd@z Thu Jan  1 00:00:00 1970
From: Stefan Bader <stefan.bader@canonical.com>
Subject: Re: BUG in xennet_make_frags with paged skb data
Date: Fri, 07 Nov 2014 13:15:59 +0100
Message-ID: <545CB7FF.8080003__22645.9461068318$1415362678$gmane$org@canonical.com>
References: <20141106214940.GD44162@ubuntu-hedt>	 <545C9013.1090406@linaro.org>
	<1415359320.13896.105.camel@edumazet-glaptop2.roam.corp.google.com>
Mime-Version: 1.0
Content-Type: multipart/mixed; boundary="===============3724587234809276439=="
Return-path: <xen-devel-bounces@lists.xen.org>
Received: from mail6.bemta3.messagelabs.com ([195.245.230.39])
	by lists.xen.org with esmtp (Exim 4.72)
	(envelope-from <stefan.bader@canonical.com>) id 1XmiSS-00027Q-PQ
	for xen-devel@lists.xenproject.org; Fri, 07 Nov 2014 12:16:04 +0000
In-Reply-To: <1415359320.13896.105.camel@edumazet-glaptop2.roam.corp.google.com>
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: Eric Dumazet <eric.dumazet@gmail.com>, Zoltan Kiss <zoltan.kiss@linaro.org>
Cc: netdev@vger.kernel.org, linux-kernel@vger.kernel.org, David Vrabel <david.vrabel@citrix.com>, xen-devel@lists.xenproject.org, Jay Vosburgh <jay.vosburgh@canonical.com>, Boris Ostrovsky <boris.ostrovsky@oracle.com>, "David S. Miller" <davem@davemloft.net>
List-Id: xen-devel@lists.xenproject.org

This is an OpenPGP/MIME signed message (RFC 4880 and 3156)
--===============3724587234809276439==
Content-Type: multipart/signed; micalg=pgp-sha512;
 protocol="application/pgp-signature";
 boundary="opLDNHQ8k3gaC4DnE7Cxm0op4Sxl5uidl"

This is an OpenPGP/MIME signed message (RFC 4880 and 3156)
--opLDNHQ8k3gaC4DnE7Cxm0op4Sxl5uidl
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable

On 07.11.2014 12:22, Eric Dumazet wrote:
> On Fri, 2014-11-07 at 09:25 +0000, Zoltan Kiss wrote:
>=20
> Please do not top post.
>=20
>> Hi,
>>
>> AFAIK in this scenario your skb frag is wrong. The page pointer should=
=20
>> point to the original compound page (not a member of it), and offset=20
>> should be set accordingly.
>> For example, if your compound page is 16K (4 page), then the page=20
>> pointer should point to the first page, and if the data starts at the =

>> 3rd page, then offset should be >8K
>=20
> This is not accurate.
>=20
> This BUG_ON() is wrong.
>=20
> It should instead be :
>=20
> BUG_ON(len + offset > PAGE_SIZE<<compound_order(compound_head(page)));

would that not have to be

BUG_ON((page-compound_head(page)*PAGE_SIZE)+offset+len >
PAGE_SIZE<<compound_order(compound_head(page)));

since offset is adjusted to start from the tail page in that case.
>=20
> splice() code can generate such cases.
>=20
>=20



--opLDNHQ8k3gaC4DnE7Cxm0op4Sxl5uidl
Content-Type: application/pgp-signature; name="signature.asc"
Content-Description: OpenPGP digital signature
Content-Disposition: attachment; filename="signature.asc"

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1

iQIcBAEBCgAGBQJUXLf/AAoJEOhnXe7L7s6jwO4QANXShxZX4Btb/XdJqqqOkOyZ
5yU4K4RucnZCx3x6XZp2QCRtNLThkUeIjQJg5n8SbGrAUYIPceuECAN4HeMIPKDC
GcV/g/o4fy/rklSNaHx+ovZo4fr6vvK6ddN8kdh9S2wydD3z6oLpBIvpFAG5zj2a
YfLa/VZJl/YBGcl/NgW02z+XsNyAn0chwVVk5sQiHY/+dQpbSW9t2sF9fKmevAlN
mmY2K+nTFSE5+urKuNZWBkbBdz+hJayyociJlD6E3OWWa6211LV+ZtUhdzJAX+6C
Galm0Tx18tSg3+RDIZkodUgup6D4rpCxR385CmXSllFT3ZO+ZY2qa6bQD0qGPBR+
KFQ3W4N66auQ/onV0RwxCMCC6oj0yIldCYjq+gQPPeJG2VPiSLAyw8vdr5tQalzD
auqXimkvj/9CsRyihQLbkq7qrhv5zeqb7THLxJQlG2SkKuwwZMGTkV5P9F4rYPxO
1ZKGoXe+5lPJgO18haH2fKNztD/dsR4OkTk6QHausxzRmKIe8d2AIuwJsAwG4X+V
mts94bySYCUY9lJUd6sh8qVtTRShg3QV2GtzTyZ0sU0srSaZEJkpWjHRJ4vqKexF
ovRi8EU5Mt4svRVi3nasM3knvG8ZWsdsxQ57HHIR6fQiPiDuke+9VfCZYl4WePNF
rWCPML3R458kxlDrhr0A
=Jb9a
-----END PGP SIGNATURE-----

--opLDNHQ8k3gaC4DnE7Cxm0op4Sxl5uidl--


--===============3724587234809276439==
Content-Type: text/plain; charset="us-ascii"
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Content-Disposition: inline

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

--===============3724587234809276439==--

From mboxrd@z Thu Jan  1 00:00:00 1970
From: Zoltan Kiss <zoltan.kiss@linaro.org>
Subject: Re: BUG in xennet_make_frags with paged skb data
Date: Fri, 07 Nov 2014 12:21:02 +0000
Message-ID: <545CB92E.9050106__3203.62522402338$1415362961$gmane$org@linaro.org>
References: <20141106214940.GD44162@ubuntu-hedt>	 <545C9013.1090406@linaro.org>
	<1415359320.13896.105.camel@edumazet-glaptop2.roam.corp.google.com>
	<545CB7FF.8080003@canonical.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"; Format="flowed"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
Received: from mail6.bemta4.messagelabs.com ([85.158.143.247])
	by lists.xen.org with esmtp (Exim 4.72)
	(envelope-from <zoltan.kiss@linaro.org>) id 1XmiXJ-0002M3-UY
	for xen-devel@lists.xenproject.org; Fri, 07 Nov 2014 12:21:06 +0000
Received: by mail-wg0-f48.google.com with SMTP id m15so3558533wgh.35
	for <xen-devel@lists.xenproject.org>;
	Fri, 07 Nov 2014 04:21:04 -0800 (PST)
In-Reply-To: <545CB7FF.8080003@canonical.com>
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: Stefan Bader <stefan.bader@canonical.com>, Eric Dumazet <eric.dumazet@gmail.com>
Cc: netdev@vger.kernel.org, linux-kernel@vger.kernel.org, David Vrabel <david.vrabel@citrix.com>, xen-devel@lists.xenproject.org, Jay Vosburgh <jay.vosburgh@canonical.com>, Boris Ostrovsky <boris.ostrovsky@oracle.com>, "David S. Miller" <davem@davemloft.net>
List-Id: xen-devel@lists.xenproject.org



On 07/11/14 12:15, Stefan Bader wrote:
> On 07.11.2014 12:22, Eric Dumazet wrote:
>> On Fri, 2014-11-07 at 09:25 +0000, Zoltan Kiss wrote:
>>
>> Please do not top post.
>>
>>> Hi,
>>>
>>> AFAIK in this scenario your skb frag is wrong. The page pointer should
>>> point to the original compound page (not a member of it), and offset
>>> should be set accordingly.
>>> For example, if your compound page is 16K (4 page), then the page
>>> pointer should point to the first page, and if the data starts at the
>>> 3rd page, then offset should be >8K
>>
>> This is not accurate.
>>
>> This BUG_ON() is wrong.
>>
>> It should instead be :
>>
>> BUG_ON(len + offset > PAGE_SIZE<<compound_order(compound_head(page)));
>
> would that not have to be
>
> BUG_ON((page-compound_head(page)*PAGE_SIZE)+offset+len >
> PAGE_SIZE<<compound_order(compound_head(page)));

There should be a parentheses around "page-compound_head(page)".
>
> since offset is adjusted to start from the tail page in that case.
>>
>> splice() code can generate such cases.
>>
>>
>
>

From mboxrd@z Thu Jan  1 00:00:00 1970
From: Stefan Bader <stefan.bader@canonical.com>
Subject: Re: BUG in xennet_make_frags with paged skb data
Date: Fri, 07 Nov 2014 13:28:33 +0100
Message-ID: <545CBAF1.4020207__32597.9751338766$1415363409$gmane$org@canonical.com>
References: <20141106214940.GD44162@ubuntu-hedt>	 <545C9013.1090406@linaro.org>
	<1415359320.13896.105.camel@edumazet-glaptop2.roam.corp.google.com>
	<545CB7FF.8080003@canonical.com> <545CB92E.9050106@linaro.org>
Mime-Version: 1.0
Content-Type: multipart/mixed; boundary="===============7795218471567528611=="
Return-path: <xen-devel-bounces@lists.xen.org>
Received: from mail6.bemta4.messagelabs.com ([85.158.143.247])
	by lists.xen.org with esmtp (Exim 4.72)
	(envelope-from <stefan.bader@canonical.com>) id 1Xmieb-0002cm-5Q
	for xen-devel@lists.xenproject.org; Fri, 07 Nov 2014 12:28:37 +0000
In-Reply-To: <545CB92E.9050106@linaro.org>
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: Zoltan Kiss <zoltan.kiss@linaro.org>, Eric Dumazet <eric.dumazet@gmail.com>
Cc: netdev@vger.kernel.org, linux-kernel@vger.kernel.org, David Vrabel <david.vrabel@citrix.com>, xen-devel@lists.xenproject.org, Jay Vosburgh <jay.vosburgh@canonical.com>, Boris Ostrovsky <boris.ostrovsky@oracle.com>, "David S. Miller" <davem@davemloft.net>
List-Id: xen-devel@lists.xenproject.org

This is an OpenPGP/MIME signed message (RFC 4880 and 3156)
--===============7795218471567528611==
Content-Type: multipart/signed; micalg=pgp-sha512;
 protocol="application/pgp-signature";
 boundary="0ViuMnpe8U2G0Vr4XJacPpStuwjAKOr1S"

This is an OpenPGP/MIME signed message (RFC 4880 and 3156)
--0ViuMnpe8U2G0Vr4XJacPpStuwjAKOr1S
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable

On 07.11.2014 13:21, Zoltan Kiss wrote:
>=20
>=20
> On 07/11/14 12:15, Stefan Bader wrote:
>> On 07.11.2014 12:22, Eric Dumazet wrote:
>>> On Fri, 2014-11-07 at 09:25 +0000, Zoltan Kiss wrote:
>>>
>>> Please do not top post.
>>>
>>>> Hi,
>>>>
>>>> AFAIK in this scenario your skb frag is wrong. The page pointer shou=
ld
>>>> point to the original compound page (not a member of it), and offset=

>>>> should be set accordingly.
>>>> For example, if your compound page is 16K (4 page), then the page
>>>> pointer should point to the first page, and if the data starts at th=
e
>>>> 3rd page, then offset should be >8K
>>>
>>> This is not accurate.
>>>
>>> This BUG_ON() is wrong.
>>>
>>> It should instead be :
>>>
>>> BUG_ON(len + offset > PAGE_SIZE<<compound_order(compound_head(page)))=
;
>>
>> would that not have to be
>>
>> BUG_ON((page-compound_head(page)*PAGE_SIZE)+offset+len >
>> PAGE_SIZE<<compound_order(compound_head(page)));
>=20
> There should be a parentheses around "page-compound_head(page)".

*sigh* before submitting anything I'll have to make sure I run it past th=
e
compiler at least. I never manager to type the beast without some error.

But you got the drift...

>>
>> since offset is adjusted to start from the tail page in that case.
>>>
>>> splice() code can generate such cases.
>>>
>>>
>>
>>



--0ViuMnpe8U2G0Vr4XJacPpStuwjAKOr1S
Content-Type: application/pgp-signature; name="signature.asc"
Content-Description: OpenPGP digital signature
Content-Disposition: attachment; filename="signature.asc"

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1

iQIcBAEBCgAGBQJUXLrxAAoJEOhnXe7L7s6jWHkQALuMGqlxHCK1CKo30aofmUP9
hSqbVSEoqL8ahwPovgbA7aNsPfGgpoy1LM0xw1f/1nFWaGaN1YpMoS9MFqYOWK7n
SYl9sNztzG14uyQtGFyzLHhDkwCopkDqeDPYwcPbHN0knQfvZaTs47pR4Rgy6+Q+
bzvBN1aOe2wl1CC1h0Lbd8zkiXH29tGVJqgcWKnEo6Dwr/7SElujLCjXsAdm77AV
jdvdl2jEz6X2K4aXuHyUFkUtZ1DKkoHDP+RPNDxS9qyuS7UrjCMWf8n8Q0ieCWnl
0JLOA5mD79jj74md1N49XOieQMMivv/vjaJ9WrAN6u1SNpjM28ieqcXz85Yu9WXC
vQ2EkxEEWMJ7rN+Cqhk1kkFjKwL5ZEytlmtAFJhkoJbqn9Xyz+bUjG/Xsizyu+rK
JFgDYGqCd4iUl/JON4gPlk9Cclw2zeFb5yAD0rTv6tLakI3iJhnkP5z91cYPTICS
qZF0zWfhJQkgtZPKCeilTT8zWtrfpeNMK8awWUL9EAPzQ64LeJEgV9P7vo4mQNfG
XPGAbb/Rv4SKAsqTy7DYFNCnT0gEO/5Vr2LBD4/LXLtzpW9Lc1ZKzLs5IfYsYdGj
Jh6kIcAm4wWekaZqX61TKIjfwFVZ5buuZoZssmIKAtqsvxQGD+TcCgp601biRGP/
tcJMgLiZQiHGn6BCvZJl
=FOiu
-----END PGP SIGNATURE-----

--0ViuMnpe8U2G0Vr4XJacPpStuwjAKOr1S--


--===============7795218471567528611==
Content-Type: text/plain; charset="us-ascii"
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Content-Disposition: inline

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

--===============7795218471567528611==--

From mboxrd@z Thu Jan  1 00:00:00 1970
From: Zoltan Kiss <zoltan.kiss@linaro.org>
Subject: Re: BUG in xennet_make_frags with paged skb data
Date: Fri, 07 Nov 2014 14:51:00 +0000
Message-ID: <545CDC54.9050305__33555.5039848716$1415371980$gmane$org@linaro.org>
References: <20141106214940.GD44162@ubuntu-hedt> <545CA27F.4070400@citrix.com>
	<545CA823.2080307@canonical.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"; Format="flowed"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
Received: from mail6.bemta4.messagelabs.com ([85.158.143.247])
	by lists.xen.org with esmtp (Exim 4.72)
	(envelope-from <zoltan.kiss@linaro.org>) id 1XmksR-00073B-9K
	for xen-devel@lists.xenproject.org; Fri, 07 Nov 2014 14:51:03 +0000
Received: by mail-wg0-f41.google.com with SMTP id k14so3883591wgh.28
	for <xen-devel@lists.xenproject.org>;
	Fri, 07 Nov 2014 06:51:02 -0800 (PST)
In-Reply-To: <545CA823.2080307@canonical.com>
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: Stefan Bader <stefan.bader@canonical.com>, David Vrabel <david.vrabel@citrix.com>, netdev@vger.kernel.org, "David S. Miller" <davem@davemloft.net>, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>, Boris Ostrovsky <boris.ostrovsky@oracle.com>, Jay Vosburgh <jay.vosburgh@canonical.com>, linux-kernel@vger.kernel.org, xen-devel@lists.xenproject.org
List-Id: xen-devel@lists.xenproject.org

Hi,

On 07/11/14 11:08, Stefan Bader wrote:
> On 07.11.2014 11:44, David Vrabel wrote:
>> On 06/11/14 21:49, Seth Forshee wrote:
>>> We've had several reports of hitting the following BUG_ON in
>>> xennet_make_frags with 3.2 and 3.13 kernels (I'm currently awaiting
>>> results of testing with 3.17):
>>>
>>>          /* Grant backend access to each skb fragment page. */
>>>          for (i = 0; i < frags; i++) {
>>>                  skb_frag_t *frag = skb_shinfo(skb)->frags + i;
>>>                  struct page *page = skb_frag_page(frag);
>>>
>>>                  len = skb_frag_size(frag);
>>>                  offset = frag->page_offset;
>>>
>>>                  /* Data must not cross a page boundary. */
>>>                  BUG_ON(len + offset > PAGE_SIZE<<compound_order(page));
>>>
>>> When this happens the page in question is a "middle" page in a compound
>>> page (i.e. it's a tail page but not the last tail page), and the data is
>>> fully contained within the compound page. The data does however cross
>>> the hardware page boundary, and since compound_order evaluates to 0 for
>>> tail pages the check fails.
>>>
>>> In going over this I've been unable to determine whether the BUG_ON in
>>> xennet_make_frags is incorrect or the paged skb data is wrong. I can't
>>> find that it's documented anywhere, and the networking code itself is a
>>> bit ambiguous when it comes to compound pages. On the one hand
>>> __skb_fill_page_desc specifically handles adding tail pages as paged
>>> data, but on the other hand skb_copy_bits kmaps frag->page.p which could
>>> fail with data that extends into another page.
>>
>> netfront will safely handle this case so you can remove this BUG_ON()
>> (and the one later on).  But it would be better to find out were these
>> funny-looking skbs are coming from and (if necessary) fixing the bug there.
>
> Right, in fact it does ignore pages from the start of a compound page in case
> the offset is big enough. So there is no difference between that case and the
> one where the page pointer from the frag is adjusted the way it was observed.
>
> It really boils down to the question what the real rules for the frags are. If
> it is "only" that data may not cross the boundary of a hw or compound page this
> leaves room for interpretation. The odd skb does not violate that but a check
> for conformance would become a lot more complex. And netfront is not the only
> place that expects the frag page to be pointing to the compound head (there is
> igb for example, though it does only a warn_on upon failing).
>
> On the other hand the __skb_fill_page_desc is written in a way that seems to
> assume the frag page pointer may be pointing to a tail page. So before "blaming"
> one piece of code or the other we need an authoritative answer to what we are
> supposed to expect.

Looking at skb_copy_bits and kmap_atomic it looks confusing indeed. It 
seems kmap_atomic maps only the actual page, not the whole compound, and 
it doesn't matter whether you gave a head or a tail page to it. 
Therefore if skb_copy_bits comes over a frag where the buffer exceeds 
the page the pointer points to, it shouldn't work.
This raises a few question in me:

- I assume frags are allowed to have compound pages, where page.p points 
to the head, and the buffer can be anywhere in the compound, e.g. offset 
= 9000 and len = 4000 is allowed (if the compound is more than four 4k 
pages, of course). Is this assumption correct?
- Is it then allowed to have page.p pointed to a tail page of the 
compound? To use the above example: page.p points to the third page, and 
offset is 808. Or pointer points to the second page, and offset is 4904. 
(the answer to the above two question should be documented somewhere, or 
codified)
- how does it works with skb_copy_bits? kmap_atomic seems to map only 
the actual page, not the whole compound.

Zoli

From mboxrd@z Thu Jan  1 00:00:00 1970
From: Seth Forshee <seth.forshee@canonical.com>
Subject: Re: BUG in xennet_make_frags with paged skb data
Date: Mon, 10 Nov 2014 08:35:17 -0600
Message-ID: <20141110143517.GA74005__48780.5828139701$1415630240$gmane$org@ubuntu-hedt>
References: <20141106214940.GD44162@ubuntu-hedt> <545CA27F.4070400@citrix.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
Received: from mail6.bemta4.messagelabs.com ([85.158.143.247])
	by lists.xen.org with esmtp (Exim 4.72)
	(envelope-from <seth.forshee@canonical.com>) id 1Xnq42-0007ZK-Iq
	for xen-devel@lists.xenproject.org; Mon, 10 Nov 2014 14:35:30 +0000
Received: by mail-oi0-f47.google.com with SMTP id a3so5477299oib.34
	for <xen-devel@lists.xenproject.org>;
	Mon, 10 Nov 2014 06:35:28 -0800 (PST)
Content-Disposition: inline
In-Reply-To: <545CA27F.4070400@citrix.com>
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: David Vrabel <david.vrabel@citrix.com>
Cc: Eric Dumazet <eric.dumazet@gmail.com>, netdev@vger.kernel.org, linux-kernel@vger.kernel.org, Stefan Bader <stefan.bader@canonical.com>, seth.forshee@canonical.com, xen-devel@lists.xenproject.org, Jay Vosburgh <jay.vosburgh@canonical.com>, Boris Ostrovsky <boris.ostrovsky@oracle.com>, "David S. Miller" <davem@davemloft.net>
List-Id: xen-devel@lists.xenproject.org

On Fri, Nov 07, 2014 at 10:44:15AM +0000, David Vrabel wrote:
> On 06/11/14 21:49, Seth Forshee wrote:
> > We've had several reports of hitting the following BUG_ON in
> > xennet_make_frags with 3.2 and 3.13 kernels (I'm currently awaiting
> > results of testing with 3.17):
> > 
> >         /* Grant backend access to each skb fragment page. */
> >         for (i = 0; i < frags; i++) {
> >                 skb_frag_t *frag = skb_shinfo(skb)->frags + i;
> >                 struct page *page = skb_frag_page(frag);
> > 
> >                 len = skb_frag_size(frag);
> >                 offset = frag->page_offset;
> > 
> >                 /* Data must not cross a page boundary. */
> >                 BUG_ON(len + offset > PAGE_SIZE<<compound_order(page));
> > 
> > When this happens the page in question is a "middle" page in a compound
> > page (i.e. it's a tail page but not the last tail page), and the data is
> > fully contained within the compound page. The data does however cross
> > the hardware page boundary, and since compound_order evaluates to 0 for
> > tail pages the check fails.
> > 
> > In going over this I've been unable to determine whether the BUG_ON in
> > xennet_make_frags is incorrect or the paged skb data is wrong. I can't
> > find that it's documented anywhere, and the networking code itself is a
> > bit ambiguous when it comes to compound pages. On the one hand
> > __skb_fill_page_desc specifically handles adding tail pages as paged
> > data, but on the other hand skb_copy_bits kmaps frag->page.p which could
> > fail with data that extends into another page.
> 
> netfront will safely handle this case so you can remove this BUG_ON()
> (and the one later on).  But it would be better to find out were these
> funny-looking skbs are coming from and (if necessary) fixing the bug there.

There still seems to be disagreement about whether the "funny" skb is
valid though - you imply it isn't, but Eric says it is. I've been trying
to track down where these skbs originate, and so far I've determined
that they come from a socket spliced to a pipe spliced to a socket. It
looks like the particular page/offset/len tuple originates at least as
far back as the first socket, as the tuple is simply copied from an skb
into the pipe and from the pipe into the final skb.

Anyway, it looks like at minimum it should be safe to change the first
BUG_ON to assert that the data is fully within the compound page as
Stefan suggested, then remove the second BUG_ON entirely. This is the
path I plan to pursue unless someone objects.

Thanks,
Seth

From mboxrd@z Thu Jan  1 00:00:00 1970
From: David Vrabel <david.vrabel@citrix.com>
Subject: Re: BUG in xennet_make_frags with paged skb data
Date: Mon, 10 Nov 2014 14:41:41 +0000
Message-ID: <5460CEA5.3070201__47387.3338116848$1415630618$gmane$org@citrix.com>
References: <20141106214940.GD44162@ubuntu-hedt> <545CA27F.4070400@citrix.com>
	<20141110143517.GA74005@ubuntu-hedt>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
Received: from mail6.bemta5.messagelabs.com ([195.245.231.135])
	by lists.xen.org with esmtp (Exim 4.72)
	(envelope-from <david.vrabel@citrix.com>) id 1XnqAB-0007rn-Ir
	for xen-devel@lists.xenproject.org; Mon, 10 Nov 2014 14:41:51 +0000
In-Reply-To: <20141110143517.GA74005@ubuntu-hedt>
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: netdev@vger.kernel.org, "David S. Miller" <davem@davemloft.net>, Eric Dumazet <eric.dumazet@gmail.com>, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>, Boris Ostrovsky <boris.ostrovsky@oracle.com>, Stefan Bader <stefan.bader@canonical.com>, Jay Vosburgh <jay.vosburgh@canonical.com>, linux-kernel@vger.kernel.org, xen-devel@lists.xenproject.org
List-Id: xen-devel@lists.xenproject.org

On 10/11/14 14:35, Seth Forshee wrote:
> On Fri, Nov 07, 2014 at 10:44:15AM +0000, David Vrabel wrote:
>> On 06/11/14 21:49, Seth Forshee wrote:
>>> We've had several reports of hitting the following BUG_ON in
>>> xennet_make_frags with 3.2 and 3.13 kernels (I'm currently awaiting
>>> results of testing with 3.17):
>>>
>>>         /* Grant backend access to each skb fragment page. */
>>>         for (i = 0; i < frags; i++) {
>>>                 skb_frag_t *frag = skb_shinfo(skb)->frags + i;
>>>                 struct page *page = skb_frag_page(frag);
>>>
>>>                 len = skb_frag_size(frag);
>>>                 offset = frag->page_offset;
>>>
>>>                 /* Data must not cross a page boundary. */
>>>                 BUG_ON(len + offset > PAGE_SIZE<<compound_order(page));
>>>
>>> When this happens the page in question is a "middle" page in a compound
>>> page (i.e. it's a tail page but not the last tail page), and the data is
>>> fully contained within the compound page. The data does however cross
>>> the hardware page boundary, and since compound_order evaluates to 0 for
>>> tail pages the check fails.
>>>
>>> In going over this I've been unable to determine whether the BUG_ON in
>>> xennet_make_frags is incorrect or the paged skb data is wrong. I can't
>>> find that it's documented anywhere, and the networking code itself is a
>>> bit ambiguous when it comes to compound pages. On the one hand
>>> __skb_fill_page_desc specifically handles adding tail pages as paged
>>> data, but on the other hand skb_copy_bits kmaps frag->page.p which could
>>> fail with data that extends into another page.
>>
>> netfront will safely handle this case so you can remove this BUG_ON()
>> (and the one later on).  But it would be better to find out were these
>> funny-looking skbs are coming from and (if necessary) fixing the bug there.
> 
> There still seems to be disagreement about whether the "funny" skb is
> valid though - you imply it isn't, but Eric says it is. I've been trying
> to track down where these skbs originate, and so far I've determined
> that they come from a socket spliced to a pipe spliced to a socket. It
> looks like the particular page/offset/len tuple originates at least as
> far back as the first socket, as the tuple is simply copied from an skb
> into the pipe and from the pipe into the final skb.

Apologies for the lack of clarity.  I meant either: a) fix the producer
if these skbs are invalid; or b) remove the BUG_ON()s.  Since Eric says
these are actually valid skbs, please do option (b).

i.e., remove both BUG_ON()s.

David

From mboxrd@z Thu Jan  1 00:00:00 1970
From: David Vrabel <david.vrabel@citrix.com>
Subject: Re: BUG in xennet_make_frags with paged skb data
Date: Mon, 10 Nov 2014 16:42:25 +0000
Message-ID: <5460EAF1.5020903__28760.7083948844$1415637885$gmane$org@citrix.com>
References: <20141106214940.GD44162@ubuntu-hedt>
	<545CA27F.4070400@citrix.com>	<20141110143517.GA74005@ubuntu-hedt>
	<5460CEA5.3070201@citrix.com> <5460EA45.2080202@linaro.org>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
Received: from mail6.bemta5.messagelabs.com ([195.245.231.135])
	by lists.xen.org with esmtp (Exim 4.72)
	(envelope-from <david.vrabel@citrix.com>) id 1Xns3F-0005ZI-Dm
	for xen-devel@lists.xenproject.org; Mon, 10 Nov 2014 16:42:49 +0000
In-Reply-To: <5460EA45.2080202@linaro.org>
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: Zoltan Kiss <zoltan.kiss@linaro.org>, netdev@vger.kernel.org, "David S.
	Miller" <davem@davemloft.net>, Eric Dumazet <eric.dumazet@gmail.com>, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>, Boris Ostrovsky <boris.ostrovsky@oracle.com>, Stefan Bader <stefan.bader@canonical.com>, Jay Vosburgh <jay.vosburgh@canonical.com>, linux-kernel@vger.kernel.org, xen-devel@lists.xenproject.org
List-Id: xen-devel@lists.xenproject.org

On 10/11/14 16:39, Zoltan Kiss wrote:
> 
> The BUG_ON suggested by Stefan would be still reasonable:
> 
> BUG_ON(((page-compound_head(page))*PAGE_SIZE)+offset+len >
> PAGE_SIZE<<compound_order(compound_head(page)));

Well, it wouldn't trigger but I don't think it is useful any more.

David

From mboxrd@z Thu Jan  1 00:00:00 1970
From: Zoltan Kiss <zoltan.kiss@linaro.org>
Subject: Re: BUG in xennet_make_frags with paged skb data
Date: Mon, 10 Nov 2014 16:39:33 +0000
Message-ID: <5460EA45.2080202__12317.9615279243$1415637972$gmane$org@linaro.org>
References: <20141106214940.GD44162@ubuntu-hedt>
	<545CA27F.4070400@citrix.com>	<20141110143517.GA74005@ubuntu-hedt>
	<5460CEA5.3070201@citrix.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"; Format="flowed"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
Received: from mail6.bemta3.messagelabs.com ([195.245.230.39])
	by lists.xen.org with esmtp (Exim 4.72)
	(envelope-from <zoltan.kiss@linaro.org>) id 1Xns55-0005ff-2o
	for xen-devel@lists.xenproject.org; Mon, 10 Nov 2014 16:44:43 +0000
Received: by mail-wi0-f181.google.com with SMTP id n3so11036034wiv.8
	for <xen-devel@lists.xenproject.org>;
	Mon, 10 Nov 2014 08:44:41 -0800 (PST)
In-Reply-To: <5460CEA5.3070201@citrix.com>
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: David Vrabel <david.vrabel@citrix.com>, netdev@vger.kernel.org, "David S. Miller" <davem@davemloft.net>, Eric Dumazet <eric.dumazet@gmail.com>, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>, Boris Ostrovsky <boris.ostrovsky@oracle.com>, Stefan Bader <stefan.bader@canonical.com>, Jay Vosburgh <jay.vosburgh@canonical.com>, linux-kernel@vger.kernel.org, xen-devel@lists.xenproject.org
List-Id: xen-devel@lists.xenproject.org



On 10/11/14 14:41, David Vrabel wrote:
> On 10/11/14 14:35, Seth Forshee wrote:
>> On Fri, Nov 07, 2014 at 10:44:15AM +0000, David Vrabel wrote:
>>> On 06/11/14 21:49, Seth Forshee wrote:
>>>> We've had several reports of hitting the following BUG_ON in
>>>> xennet_make_frags with 3.2 and 3.13 kernels (I'm currently awaiting
>>>> results of testing with 3.17):
>>>>
>>>>          /* Grant backend access to each skb fragment page. */
>>>>          for (i = 0; i < frags; i++) {
>>>>                  skb_frag_t *frag = skb_shinfo(skb)->frags + i;
>>>>                  struct page *page = skb_frag_page(frag);
>>>>
>>>>                  len = skb_frag_size(frag);
>>>>                  offset = frag->page_offset;
>>>>
>>>>                  /* Data must not cross a page boundary. */
>>>>                  BUG_ON(len + offset > PAGE_SIZE<<compound_order(page));
>>>>
>>>> When this happens the page in question is a "middle" page in a compound
>>>> page (i.e. it's a tail page but not the last tail page), and the data is
>>>> fully contained within the compound page. The data does however cross
>>>> the hardware page boundary, and since compound_order evaluates to 0 for
>>>> tail pages the check fails.
>>>>
>>>> In going over this I've been unable to determine whether the BUG_ON in
>>>> xennet_make_frags is incorrect or the paged skb data is wrong. I can't
>>>> find that it's documented anywhere, and the networking code itself is a
>>>> bit ambiguous when it comes to compound pages. On the one hand
>>>> __skb_fill_page_desc specifically handles adding tail pages as paged
>>>> data, but on the other hand skb_copy_bits kmaps frag->page.p which could
>>>> fail with data that extends into another page.
>>>
>>> netfront will safely handle this case so you can remove this BUG_ON()
>>> (and the one later on).  But it would be better to find out were these
>>> funny-looking skbs are coming from and (if necessary) fixing the bug there.
>>
>> There still seems to be disagreement about whether the "funny" skb is
>> valid though - you imply it isn't, but Eric says it is. I've been trying
>> to track down where these skbs originate, and so far I've determined
>> that they come from a socket spliced to a pipe spliced to a socket. It
>> looks like the particular page/offset/len tuple originates at least as
>> far back as the first socket, as the tuple is simply copied from an skb
>> into the pipe and from the pipe into the final skb.
>
> Apologies for the lack of clarity.  I meant either: a) fix the producer
> if these skbs are invalid; or b) remove the BUG_ON()s.  Since Eric says
> these are actually valid skbs, please do option (b).
>
> i.e., remove both BUG_ON()s.

The BUG_ON suggested by Stefan would be still reasonable:

BUG_ON(((page-compound_head(page))*PAGE_SIZE)+offset+len >
PAGE_SIZE<<compound_order(compound_head(page)));
>
> David
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
>

From mboxrd@z Thu Jan  1 00:00:00 1970
From: Eric Dumazet <eric.dumazet@gmail.com>
Subject: Re: BUG in xennet_make_frags with paged skb data
Date: Mon, 10 Nov 2014 09:02:35 -0800
Message-ID: <1415638955.9613.4.camel__26819.6974335753$1415639097$gmane$org@edumazet-glaptop2.roam.corp.google.com>
References: <20141106214940.GD44162@ubuntu-hedt>
	<545CA27F.4070400@citrix.com>	<20141110143517.GA74005@ubuntu-hedt>
	<5460CEA5.3070201@citrix.com> <5460EA45.2080202@linaro.org>
	<5460EAF1.5020903@citrix.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
Received: from mail6.bemta3.messagelabs.com ([195.245.230.39])
	by lists.xen.org with esmtp (Exim 4.72)
	(envelope-from <eric.dumazet@gmail.com>) id 1XnsMT-0006Q3-2H
	for xen-devel@lists.xenproject.org; Mon, 10 Nov 2014 17:02:41 +0000
Received: by mail-ie0-f177.google.com with SMTP id tp5so9582141ieb.22
	for <xen-devel@lists.xenproject.org>;
	Mon, 10 Nov 2014 09:02:38 -0800 (PST)
In-Reply-To: <5460EAF1.5020903@citrix.com>
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: David Vrabel <david.vrabel@citrix.com>
Cc: Zoltan Kiss <zoltan.kiss@linaro.org>, netdev@vger.kernel.org, linux-kernel@vger.kernel.org, Stefan Bader <stefan.bader@canonical.com>, xen-devel@lists.xenproject.org, Jay Vosburgh <jay.vosburgh@canonical.com>, Boris Ostrovsky <boris.ostrovsky@oracle.com>, "David S.
	Miller" <davem@davemloft.net>
List-Id: xen-devel@lists.xenproject.org

On Mon, 2014-11-10 at 16:42 +0000, David Vrabel wrote:
> On 10/11/14 16:39, Zoltan Kiss wrote:
> > 
> > The BUG_ON suggested by Stefan would be still reasonable:
> > 
> > BUG_ON(((page-compound_head(page))*PAGE_SIZE)+offset+len >
> > PAGE_SIZE<<compound_order(compound_head(page)));
> 
> Well, it wouldn't trigger but I don't think it is useful any more.

Right.

This BUG_ON() does not make sense (its current implementation is
assuming a very precise layout anyway)

If we really wanted some debugging, we would need something more generic
and done in core networking stack, not in a particular driver.

