| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <1353420865.13542.44.camel@zakaz.uk.xensource.com>
Date: Tue, 20 Nov 2012 14:14:25 +0000
From: Ian Campbell <Ian.Campbell@...rix.com>
To: Jan Beulich <JBeulich@...e.com>
CC: Stefan Bader <stefan.bader@...onical.com>,
Sander Eikelenboom <linux@...elenboom.it>,
Eric Dumazet <edumazet@...gle.com>,
"KonradRzeszutek Wilk" <konrad@...nel.org>,
"xen-devel@...ts.xen.org" <xen-devel@...ts.xen.org>,
ANNIE LI <annie.li@...cle.com>,
"netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: [Xen-devel] [PATCH] xen/netfront: handle compound page
fragments on transmit
On Tue, 2012-11-20 at 13:51 +0000, Jan Beulich wrote:
> >>> On 20.11.12 at 14:35, Ian Campbell <Ian.Campbell@...rix.com> wrote:
> > On Tue, 2012-11-20 at 12:28 +0000, Jan Beulich wrote:
> >> >>> On 20.11.12 at 12:40, Ian Campbell <ian.campbell@...rix.com> wrote:
> >> > An SKB paged fragment can consist of a compound page with order > 0.
> >> > However the netchannel protocol deals only in PAGE_SIZE frames.
> >> >
> >> > Handle this in xennet_make_frags by iterating over the frames which
> >> > make up the page.
> >> >
> >> > This is the netfront equivalent to 6a8ed462f16b for netback.
> >>
> >> Wouldn't you need to be at least a little more conservative here
> >> with respect to resource use: I realize that get_id_from_freelist()
> >> return values were never checked, and failure of
> >> gnttab_claim_grant_reference() was always dealt with via
> >> BUG_ON(), but considering that netfront_tx_slot_available()
> >> doesn't account for compound page fragments, I think this (lack
> >> of) error handling needs improvement in the course of the
> >> change here (regardless of - I think - someone having said that
> >> usually the sum of all pages referenced from an skb's fragments
> >> would not exceed MAX_SKB_FRAGS - "usually" just isn't enough
> >> imo).
> >
> > I think it is more than "usually", it is derived from the number of
> > pages needed to contain 64K of data which is the maximum size of the
> > data associated with an skb (AIUI).
> >
> > Unwinding from failure in xennet_make_frags looks pretty tricky,
>
> Yes, I agree.
>
> > but how about this incremental patch:
>
> Looks good, but can probably be simplified quite a bit:
>
> > --- a/drivers/net/xen-netfront.c
> > +++ b/drivers/net/xen-netfront.c
> > @@ -505,6 +505,46 @@ static void xennet_make_frags(struct sk_buff *skb, struct net_device *dev,
> > np->tx.req_prod_pvt = prod;
> > }
> >
> > +/*
> > + * Count how many ring slots are required to send the frags of this
> > + * skb. Each frag might be a compound page.
> > + */
> > +static int xennet_count_skb_frag_pages(struct sk_buff *skb)
> > +{
> > + int i, frags = skb_shinfo(skb)->nr_frags;
> > + int pages = 0;
> > +
> > + for (i = 0; i < frags; i++) {
> > + skb_frag_t *frag = skb_shinfo(skb)->frags + i;
> > + unsigned long size = skb_frag_size(frag);
> > + unsigned long offset = frag->page_offset;
> > +
> > + /* Skip unused frames from start of page */
> > + offset &= ~PAGE_MASK;
> > +
> > + while (size > 0) {
> > + unsigned long bytes;
> > +
> > + BUG_ON(offset >= PAGE_SIZE);
> > +
> > + bytes = PAGE_SIZE - offset;
> > + if (bytes > size)
> > + bytes = size;
> > +
> > + offset += bytes;
> > + size -= bytes;
> > +
> > + /* Next frame */
> > + if (offset == PAGE_SIZE && size) {
> > + pages++;
> > + offset = 0;
> > + }
> > + }
>
> Isn't the whole loop equivalent to
>
> pages = PFN_UP(offset + size);
>
> (at least as long as size is not zero)?
Er, yes. Wood for the trees etc...
I think using PFN_UP overcounts a bit since the data needed start in the
first frame of a compound frame, but if you keep the
/* Skip unused frames from start of page */
offset &= ~PAGE_MASK;
I think that does the right thing
> > @@ -517,12 +557,13 @@ static int xennet_start_xmit(struct sk_buff *skb, struct net_device *dev)
> > grant_ref_t ref;
> > unsigned long mfn;
> > int notify;
> > - int frags = skb_shinfo(skb)->nr_frags;
> > + int frags;
> > unsigned int offset = offset_in_page(data);
> > unsigned int len = skb_headlen(skb);
> > unsigned long flags;
> >
> > - frags += DIV_ROUND_UP(offset + len, PAGE_SIZE);
> > + frags = xennet_count_skb_frag_pages(skb) +
> > + DIV_ROUND_UP(offset + len, PAGE_SIZE);
> > if (unlikely(frags > MAX_SKB_FRAGS + 1)) {
>
> This condition would now need adjustment, though (because
> "frags" is no longer what its name says).
I think it already wasn't what the name says, since it included the skb
head too. Perhaps "slots" would be a better name?
diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c
index a12b99a..b744875 100644
--- a/drivers/net/xen-netfront.c
+++ b/drivers/net/xen-netfront.c
@@ -505,6 +505,29 @@ static void xennet_make_frags(struct sk_buff *skb, struct net_device *dev,
np->tx.req_prod_pvt = prod;
}
+/*
+ * Count how many ring slots are required to send the frags of this
+ * skb. Each frag might be a compound page.
+ */
+static int xennet_count_skb_frag_slots(struct sk_buff *skb)
+{
+ int i, frags = skb_shinfo(skb)->nr_frags;
+ int pages = 0;
+
+ for (i = 0; i < frags; i++) {
+ skb_frag_t *frag = skb_shinfo(skb)->frags + i;
+ unsigned long size = skb_frag_size(frag);
+ unsigned long offset = frag->page_offset;
+
+ /* Skip unused frames from start of page */
+ offset &= ~PAGE_MASK;
+
+ pages += PFN_UP(offset + size);
+ }
+
+ return pages;
+}
+
static int xennet_start_xmit(struct sk_buff *skb, struct net_device *dev)
{
unsigned short id;
@@ -517,15 +540,16 @@ static int xennet_start_xmit(struct sk_buff *skb, struct net_device *dev)
grant_ref_t ref;
unsigned long mfn;
int notify;
- int frags = skb_shinfo(skb)->nr_frags;
+ int slots;
unsigned int offset = offset_in_page(data);
unsigned int len = skb_headlen(skb);
unsigned long flags;
- frags += DIV_ROUND_UP(offset + len, PAGE_SIZE);
- if (unlikely(frags > MAX_SKB_FRAGS + 1)) {
- printk(KERN_ALERT "xennet: skb rides the rocket: %d frags\n",
- frags);
+ slots = DIV_ROUND_UP(offset + len, PAGE_SIZE) +
+ xennet_count_skb_frag_slots(skb);
+ if (unlikely(slots > MAX_SKB_FRAGS + 1)) {
+ printk(KERN_ALERT "xennet: skb rides the rocket: %d slots\n",
+ slots);
dump_stack();
goto drop;
}
@@ -533,7 +557,7 @@ static int xennet_start_xmit(struct sk_buff *skb, struct net_device *dev)
spin_lock_irqsave(&np->tx_lock, flags);
if (unlikely(!netif_carrier_ok(dev) ||
- (frags > 1 && !xennet_can_sg(dev)) ||
+ (slots > 1 && !xennet_can_sg(dev)) ||
netif_needs_gso(skb, netif_skb_features(skb)))) {
spin_unlock_irqrestore(&np->tx_lock, flags);
goto drop;
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists