linux-kernel - RE: [PATCH] xen/netback: calculate correctly the SKB slots.

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 22 May 2012 15:01:55 -0400
From:	Simon Graham <simon.graham@...rix.com>
To:	Konrad Rzeszutek Wilk <konrad.wilk@...cle.com>,
	Ian Campbell <Ian.Campbell@...rix.com>
CC:	Ben Hutchings <bhutchings@...arflare.com>,
	"xen-devel@...ts.xensource.com" <xen-devel@...ts.xensource.com>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"davem@...emloft.net" <davem@...emloft.net>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Adnan Misherfi <adnan.misherfi@...cle.com>
Subject: RE: [PATCH] xen/netback: calculate correctly the SKB slots.

> >
> > > >  	int i, copy_off;
> > > >
> > > >  	count = DIV_ROUND_UP(
> > > > -			offset_in_page(skb->data)+skb_headlen(skb),
> PAGE_SIZE);
> > > > +			offset_in_page(skb->data + skb_headlen(skb)),
> PAGE_SIZE);
> > >
> > > The new version would be equivalent to:
> > > 	count = offset_in_page(skb->data + skb_headlen(skb)) != 0;
> > > which is not right, as netbk_gop_skb() will use one slot per page.
> >
> > Just outside the context of this patch we separately count the frag
> > pages.
> >
> > However I think you are right if skb->data covers > 1 page, since the
> > new version can only ever return 0 or 1. I expect this patch papers
> over
> > the underlying issue by not stopping often enough, rather than
> actually
> > fixing the underlying issue.
> 
> Ah, any thoughts? Have you guys seen this behavior as well?

We ran into this same problem and the fix we've been running with for a while now (been meaning to submit it!) is:

diff --git a/drivers/net/xen-netback/netback.c b/drivers/net/xen-netback/netback.c
index c2669b8..7925bd3 100644
--- a/drivers/net/xen-netback/netback.c
+++ b/drivers/net/xen-netback/netback.c
@@ -312,8 +312,7 @@ unsigned int xen_netbk_count_skb_slots(struct xenvif *vif, struct sk_buff *skb)
        unsigned int count;
        int i, copy_off;

-       count = DIV_ROUND_UP(
-                       offset_in_page(skb->data)+skb_headlen(skb), PAGE_SIZE);
+       count = DIV_ROUND_UP(skb_headlen(skb), PAGE_SIZE);

        copy_off = skb_headlen(skb) % PAGE_SIZE;

The rationale for this is that if the header spanned a page boundary, you would calculate that it needs 2 slots for the header BUT netback_gop_skb copies the header into the start of the page so only needs one slot (and only decrements the count of inuse entries by 1).

We found this running with a VIF bridged to a USB 3G Modem where skb->data started near the end of a page so the header would always span the page boundary.

It was very easy to get the VIF to stop processing frames with the old code and we have not seen any problems since applying this patch.

Simon

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/