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
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
> [email protected]
> http://lists.xen.org/xen-devel
>
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
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.
-Stefan
>
> David
>
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.
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)));
since offset is adjusted to start from the tail page in that case.
>
> splice() code can generate such cases.
>
>
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.
>>
>>
>
>
On 07.11.2014 13:21, Zoltan Kiss wrote:
>
>
> 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)".
*sigh* before submitting anything I'll have to make sure I run it past the
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.
>>>
>>>
>>
>>
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
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
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
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
> [email protected]
> http://lists.xen.org/xen-devel
>
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
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.