Patchwork [6/7] xen-netback: coalesce slots and fix regressions

login
register
mail settings
Submitter Wei Liu
Date April 9, 2013, 11:07 a.m.
Message ID <1365505655-8021-7-git-send-email-wei.liu2@citrix.com>
Download mbox | patch
Permalink /patch/235042/
State Changes Requested
Delegated to: David Miller
Headers show

Comments

Wei Liu - April 9, 2013, 11:07 a.m.
This patch tries to coalesce tx requests when constructing grant copy
structures. It enables netback to deal with situation when frontend's
MAX_SKB_FRAGS is larger than backend's MAX_SKB_FRAGS.

With the help of coalescing, this patch tries to address two regressions and
avoid reopening the security hole in XSA-39.

Regression 1. The reduction of the number of supported ring entries (slots)
per packet (from 18 to 17). This regression has been around for some time but
remains unnoticed until XSA-39 security fix. This is fixed by coalescing
slots.

Regression 2. The XSA-39 security fix turning "too many frags" errors from
just dropping the packet to a fatal error and disabling the VIF. This is fixed
by coalescing slots (handling 18 slots when backend's MAX_SKB_FRAGS is 17)
which rules out false positive (using 18 slots is legit) and dropping packets
using 19 to `max_skb_slots` slots.

To avoid reopening security hole in XSA-39, frontend sending packet using more
than max_skb_slots is considered malicious.

The behavior of netback for packet is thus:

    1-18            slots: valid
   19-max_skb_slots slots: drop and respond with an error
   max_skb_slots+   slots: fatal error

max_skb_slots is configurable by admin, default value is 20.

Also change variable name from "frags" to "slots" in netbk_count_requests.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
 drivers/net/xen-netback/netback.c |  270 ++++++++++++++++++++++++++++++-------
 include/xen/interface/io/netif.h  |   18 +++
 2 files changed, 238 insertions(+), 50 deletions(-)
Jan Beulich - April 9, 2013, 12:13 p.m.
>>> On 09.04.13 at 13:07, Wei Liu <wei.liu2@citrix.com> wrote:
> --- a/drivers/net/xen-netback/netback.c
> +++ b/drivers/net/xen-netback/netback.c
> @@ -47,11 +47,47 @@
>  #include <asm/xen/hypercall.h>
>  #include <asm/xen/page.h>
>  
> +/*
> + * This is the maximum slots a skb can have. If a guest sends a skb
> + * which exceeds this limit it is considered malicious.
> + */
> +#define MAX_SKB_SLOTS_DEFAULT 20
> +static unsigned int max_skb_slots = MAX_SKB_SLOTS_DEFAULT;
> +
> +static int max_skb_slots_set(const char *val, const struct kernel_param *kp)
> +{
> +	int ret;
> +	unsigned long param = 0;
> +
> +	ret = kstrtoul(val, 10, &param);
> +
> +	if (ret < 0 || param < XEN_NETIF_NR_SLOTS_MIN)
> +		return -EINVAL;
> +
> +	max_skb_slots = param;
> +
> +	return 0;
> +}
> +
> +static struct kernel_param_ops max_skb_slots_param_ops = {

__moduleparam_const

> +	.set = max_skb_slots_set,
> +	.get = param_get_ulong,

param_get_uint

> @@ -251,7 +291,7 @@ static int max_required_rx_slots(struct xenvif *vif)
>  	int max = DIV_ROUND_UP(vif->dev->mtu, PAGE_SIZE);
>  
>  	if (vif->can_sg || vif->gso || vif->gso_prefix)
> -		max += MAX_SKB_FRAGS + 1; /* extra_info + frags */
> +		max += XEN_NETIF_NR_SLOTS_MIN + 1; /* extra_info + frags */
>  
>  	return max;
>  }
> @@ -657,7 +697,7 @@ static void xen_netbk_rx_action(struct xen_netbk *netbk)
>  		__skb_queue_tail(&rxq, skb);
>  
>  		/* Filled the batch queue? */
> -		if (count + MAX_SKB_FRAGS >= XEN_NETIF_RX_RING_SIZE)
> +		if (count + XEN_NETIF_NR_SLOTS_MIN >= XEN_NETIF_RX_RING_SIZE)
>  			break;
>  	}
>  

Are the two changes above really correct? You're having an skb as
input here, and hence you want to use all the frags, and nothing
beyond. Another question is whether the frontend can handle
those, but that aspect isn't affected by the code being modified
here.

> @@ -904,38 +944,56 @@ static void netbk_fatal_tx_err(struct xenvif *vif)
>  
>  static int netbk_count_requests(struct xenvif *vif,
>  				struct xen_netif_tx_request *first,
> +				RING_IDX first_idx,
>  				struct xen_netif_tx_request *txp,
>  				int work_to_do)
>  {
>  	RING_IDX cons = vif->tx.req_cons;
> -	int frags = 0;
> +	int slots = 0;
> +	int drop_err = 0;
>  
>  	if (!(first->flags & XEN_NETTXF_more_data))
>  		return 0;
>  
>  	do {
> -		if (frags >= work_to_do) {
> -			netdev_err(vif->dev, "Need more frags\n");
> +		if (slots >= work_to_do) {
> +			netdev_err(vif->dev, "Need more slots\n");
>  			netbk_fatal_tx_err(vif);
>  			return -ENODATA;
>  		}
>  
> -		if (unlikely(frags >= MAX_SKB_FRAGS)) {
> -			netdev_err(vif->dev, "Too many frags\n");
> +		/* Xen network protocol had implicit dependency on
> +		 * MAX_SKB_FRAGS. XEN_NETIF_NR_SLOTS_MIN is set to the
> +		 * historical MAX_SKB_FRAGS value 18 to honor the same
> +		 * behavior as before. Any packet using more than 18
> +		 * slots but less than max_skb_slots slots is dropped
> +		 */
> +		if (!drop_err && slots >= XEN_NETIF_NR_SLOTS_MIN) {
> +			if (net_ratelimit())
> +				netdev_dbg(vif->dev, "Too many slots\n");
> +			drop_err = -E2BIG;
> +		}
> +
> +		/* This guest is really using too many slots and
> +		 * considered malicious.
> +		 */
> +		if (unlikely(slots >= max_skb_slots)) {
> +			netdev_err(vif->dev,
> +				   "Malicious frontend using too many slots\n");

Wouldn't you better swap this and the previous if?

>  			netbk_fatal_tx_err(vif);
>  			return -E2BIG;
>  		}
>  
> -		memcpy(txp, RING_GET_REQUEST(&vif->tx, cons + frags),
> +		memcpy(txp, RING_GET_REQUEST(&vif->tx, cons + slots),
>  		       sizeof(*txp));
>  		if (txp->size > first->size) {
> -			netdev_err(vif->dev, "Frag is bigger than frame.\n");
> +			netdev_err(vif->dev, "Packet is bigger than frame.\n");

I don't think "packet" is the right term here.

Jan

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Wei Liu - April 9, 2013, 12:48 p.m.
On Tue, Apr 09, 2013 at 01:13:39PM +0100, Jan Beulich wrote:
> >>> On 09.04.13 at 13:07, Wei Liu <wei.liu2@citrix.com> wrote:
[...]
> > +
> > +static struct kernel_param_ops max_skb_slots_param_ops = {
> 
> __moduleparam_const

TBH I don't see any driver makes use of this. Probably a simple "const"
can do?

> 
> > +	.set = max_skb_slots_set,
> > +	.get = param_get_ulong,
> 
> param_get_uint
> 

Done.

> > @@ -251,7 +291,7 @@ static int max_required_rx_slots(struct xenvif *vif)
> >  	int max = DIV_ROUND_UP(vif->dev->mtu, PAGE_SIZE);
> >  
> >  	if (vif->can_sg || vif->gso || vif->gso_prefix)
> > -		max += MAX_SKB_FRAGS + 1; /* extra_info + frags */
> > +		max += XEN_NETIF_NR_SLOTS_MIN + 1; /* extra_info + frags */
> >  
> >  	return max;
> >  }
> > @@ -657,7 +697,7 @@ static void xen_netbk_rx_action(struct xen_netbk *netbk)
> >  		__skb_queue_tail(&rxq, skb);
> >  
> >  		/* Filled the batch queue? */
> > -		if (count + MAX_SKB_FRAGS >= XEN_NETIF_RX_RING_SIZE)
> > +		if (count + XEN_NETIF_NR_SLOTS_MIN >= XEN_NETIF_RX_RING_SIZE)
> >  			break;
> >  	}
> >  
> 
> Are the two changes above really correct? You're having an skb as
> input here, and hence you want to use all the frags, and nothing
> beyond. Another question is whether the frontend can handle
> those, but that aspect isn't affected by the code being modified
> here.
> 

This patch tries to remove dependency on MAX_SKB_FRAGS. Writing the
protocol-defined value here is OK IMHO.

[...]
> >  			return -E2BIG;
> >  		}
> >  
> > -		memcpy(txp, RING_GET_REQUEST(&vif->tx, cons + frags),
> > +		memcpy(txp, RING_GET_REQUEST(&vif->tx, cons + slots),
> >  		       sizeof(*txp));
> >  		if (txp->size > first->size) {
> > -			netdev_err(vif->dev, "Frag is bigger than frame.\n");
> > +			netdev_err(vif->dev, "Packet is bigger than frame.\n");
> 
> I don't think "packet" is the right term here.
> 

Maybe just "Invalid tx request" and dump all information?


Wei.

> Jan
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jan Beulich - April 9, 2013, 1:13 p.m.
>>> On 09.04.13 at 14:48, Wei Liu <wei.liu2@citrix.com> wrote:
> On Tue, Apr 09, 2013 at 01:13:39PM +0100, Jan Beulich wrote:
>> >>> On 09.04.13 at 13:07, Wei Liu <wei.liu2@citrix.com> wrote:
> [...]
>> > +
>> > +static struct kernel_param_ops max_skb_slots_param_ops = {
>> 
>> __moduleparam_const
> 
> TBH I don't see any driver makes use of this.

Sure, because generally you use the simple module_param() or
module_param_named() macros.

> Probably a simple "const" can do?

The purpose of __moduleparam_const is to abstract away the
need to not have the const for a very limited set of architectures.
Even if Xen currently doesn't support any of those, I would still
not want to see architecture incompatibilities introduced if
avoidable.

>> > @@ -251,7 +291,7 @@ static int max_required_rx_slots(struct xenvif *vif)
>> >  	int max = DIV_ROUND_UP(vif->dev->mtu, PAGE_SIZE);
>> >  
>> >  	if (vif->can_sg || vif->gso || vif->gso_prefix)
>> > -		max += MAX_SKB_FRAGS + 1; /* extra_info + frags */
>> > +		max += XEN_NETIF_NR_SLOTS_MIN + 1; /* extra_info + frags */
>> >  
>> >  	return max;
>> >  }
>> > @@ -657,7 +697,7 @@ static void xen_netbk_rx_action(struct xen_netbk *netbk)
>> >  		__skb_queue_tail(&rxq, skb);
>> >  
>> >  		/* Filled the batch queue? */
>> > -		if (count + MAX_SKB_FRAGS >= XEN_NETIF_RX_RING_SIZE)
>> > +		if (count + XEN_NETIF_NR_SLOTS_MIN >= XEN_NETIF_RX_RING_SIZE)
>> >  			break;
>> >  	}
>> >  
>> 
>> Are the two changes above really correct? You're having an skb as
>> input here, and hence you want to use all the frags, and nothing
>> beyond. Another question is whether the frontend can handle
>> those, but that aspect isn't affected by the code being modified
>> here.
>> 
> 
> This patch tries to remove dependency on MAX_SKB_FRAGS. Writing the
> protocol-defined value here is OK IMHO.

I understand the intentions of the patch, but you shouldn't go
further with this than you need to. Just think through carefully
the cases of MAX_SKB_FRAGS being smaller/bigger than
XEN_NETIF_NR_SLOTS_MIN: In the first instance, you needlessly
return too big a value when the latter is the bigger one, and in
the second instance you bail from the loop early in the same case.

What's worse, in the opposite case I'm having the impression that
you would continue the loop when you shouldn't (because there's
not enough room left), and I'd suspect problems for the caller of
max_required_rx_slots() in that case too.

>> >  			return -E2BIG;
>> >  		}
>> >  
>> > -		memcpy(txp, RING_GET_REQUEST(&vif->tx, cons + frags),
>> > +		memcpy(txp, RING_GET_REQUEST(&vif->tx, cons + slots),
>> >  		       sizeof(*txp));
>> >  		if (txp->size > first->size) {
>> > -			netdev_err(vif->dev, "Frag is bigger than frame.\n");
>> > +			netdev_err(vif->dev, "Packet is bigger than frame.\n");
>> 
>> I don't think "packet" is the right term here.
>> 
> 
> Maybe just "Invalid tx request" and dump all information?

Possibly. But this may become a little too verbose then...

Jan

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Wei Liu - April 9, 2013, 1:48 p.m.
On Tue, Apr 09, 2013 at 02:13:29PM +0100, Jan Beulich wrote:
> >>> On 09.04.13 at 14:48, Wei Liu <wei.liu2@citrix.com> wrote:
> > On Tue, Apr 09, 2013 at 01:13:39PM +0100, Jan Beulich wrote:
> >> >>> On 09.04.13 at 13:07, Wei Liu <wei.liu2@citrix.com> wrote:
> > [...]
> >> > +
> >> > +static struct kernel_param_ops max_skb_slots_param_ops = {
> >> 
> >> __moduleparam_const
> > 
> > TBH I don't see any driver makes use of this.
> 
> Sure, because generally you use the simple module_param() or
> module_param_named() macros.
> 

That means other modules using this need to be fixed too. :-)

> > Probably a simple "const" can do?
> 
> The purpose of __moduleparam_const is to abstract away the
> need to not have the const for a very limited set of architectures.
> Even if Xen currently doesn't support any of those, I would still
> not want to see architecture incompatibilities introduced if
> avoidable.
> 

Sure.

> >> > @@ -251,7 +291,7 @@ static int max_required_rx_slots(struct xenvif *vif)
> >> >  	int max = DIV_ROUND_UP(vif->dev->mtu, PAGE_SIZE);
> >> >  
> >> >  	if (vif->can_sg || vif->gso || vif->gso_prefix)
> >> > -		max += MAX_SKB_FRAGS + 1; /* extra_info + frags */
> >> > +		max += XEN_NETIF_NR_SLOTS_MIN + 1; /* extra_info + frags */
> >> >  
> >> >  	return max;
> >> >  }
> >> > @@ -657,7 +697,7 @@ static void xen_netbk_rx_action(struct xen_netbk *netbk)
> >> >  		__skb_queue_tail(&rxq, skb);
> >> >  
> >> >  		/* Filled the batch queue? */
> >> > -		if (count + MAX_SKB_FRAGS >= XEN_NETIF_RX_RING_SIZE)
> >> > +		if (count + XEN_NETIF_NR_SLOTS_MIN >= XEN_NETIF_RX_RING_SIZE)
> >> >  			break;
> >> >  	}
> >> >  
> >> 
> >> Are the two changes above really correct? You're having an skb as
> >> input here, and hence you want to use all the frags, and nothing
> >> beyond. Another question is whether the frontend can handle
> >> those, but that aspect isn't affected by the code being modified
> >> here.
> >> 
> > 
> > This patch tries to remove dependency on MAX_SKB_FRAGS. Writing the
> > protocol-defined value here is OK IMHO.
> 
> I understand the intentions of the patch, but you shouldn't go
> further with this than you need to. Just think through carefully
> the cases of MAX_SKB_FRAGS being smaller/bigger than
> XEN_NETIF_NR_SLOTS_MIN: In the first instance, you needlessly
> return too big a value when the latter is the bigger one, and in
> the second instance you bail from the loop early in the same case.
> 
> What's worse, in the opposite case I'm having the impression that
> you would continue the loop when you shouldn't (because there's
> not enough room left), and I'd suspect problems for the caller of
> max_required_rx_slots() in that case too.
> 

The frontend and backend work at the moment is because MAX_SKB_FRAGS only
went down once. If it goes like 18 -> 17 -> 19 then we are screwed...

For the MAX_SKB_FRAGS < XEN_NETIF_NR_SLOTS_MIN case it is fine, we are
just reserving more room in the ring.

For the MAX_SKB_FRAGS > XEN_NETIF_NR_SLOTS_MIN case, my thought is that
is not likely to happen in the near future, we could possibly upstream
mechinasim to negotiate number of slots before MAX_SKB_FRAGS >
XEN_NETIF_NR_SLOTS_MIN ever happens.

But yes, let's leave RX path along at the moment, need to investigate
more on this.


Wei.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/net/xen-netback/netback.c b/drivers/net/xen-netback/netback.c
index 6e8e51a..3490b2c 100644
--- a/drivers/net/xen-netback/netback.c
+++ b/drivers/net/xen-netback/netback.c
@@ -47,11 +47,47 @@ 
 #include <asm/xen/hypercall.h>
 #include <asm/xen/page.h>
 
+/*
+ * This is the maximum slots a skb can have. If a guest sends a skb
+ * which exceeds this limit it is considered malicious.
+ */
+#define MAX_SKB_SLOTS_DEFAULT 20
+static unsigned int max_skb_slots = MAX_SKB_SLOTS_DEFAULT;
+
+static int max_skb_slots_set(const char *val, const struct kernel_param *kp)
+{
+	int ret;
+	unsigned long param = 0;
+
+	ret = kstrtoul(val, 10, &param);
+
+	if (ret < 0 || param < XEN_NETIF_NR_SLOTS_MIN)
+		return -EINVAL;
+
+	max_skb_slots = param;
+
+	return 0;
+}
+
+static struct kernel_param_ops max_skb_slots_param_ops = {
+	.set = max_skb_slots_set,
+	.get = param_get_ulong,
+};
+
+module_param_cb(max_skb_slots, &max_skb_slots_param_ops,
+		&max_skb_slots, 0444);
+
+typedef unsigned int pending_ring_idx_t;
+#define INVALID_PENDING_RING_IDX (~0U)
+
 struct pending_tx_info {
-	struct xen_netif_tx_request req;
+	struct xen_netif_tx_request req; /* coalesced tx request  */
 	struct xenvif *vif;
+	pending_ring_idx_t head; /* head != INVALID_PENDING_RING_IDX
+				  * if it is head of one or more tx
+				  * reqs
+				  */
 };
-typedef unsigned int pending_ring_idx_t;
 
 struct netbk_rx_meta {
 	int id;
@@ -102,7 +138,11 @@  struct xen_netbk {
 	atomic_t netfront_count;
 
 	struct pending_tx_info pending_tx_info[MAX_PENDING_REQS];
-	struct gnttab_copy tx_copy_ops[MAX_PENDING_REQS];
+	/* Coalescing tx requests before copying makes number of grant
+	 * copy ops greater of equal to number of slots required. In
+	 * worst case a tx request consumes 2 gnttab_copy.
+	 */
+	struct gnttab_copy tx_copy_ops[2*MAX_PENDING_REQS];
 
 	u16 pending_ring[MAX_PENDING_REQS];
 
@@ -251,7 +291,7 @@  static int max_required_rx_slots(struct xenvif *vif)
 	int max = DIV_ROUND_UP(vif->dev->mtu, PAGE_SIZE);
 
 	if (vif->can_sg || vif->gso || vif->gso_prefix)
-		max += MAX_SKB_FRAGS + 1; /* extra_info + frags */
+		max += XEN_NETIF_NR_SLOTS_MIN + 1; /* extra_info + frags */
 
 	return max;
 }
@@ -657,7 +697,7 @@  static void xen_netbk_rx_action(struct xen_netbk *netbk)
 		__skb_queue_tail(&rxq, skb);
 
 		/* Filled the batch queue? */
-		if (count + MAX_SKB_FRAGS >= XEN_NETIF_RX_RING_SIZE)
+		if (count + XEN_NETIF_NR_SLOTS_MIN >= XEN_NETIF_RX_RING_SIZE)
 			break;
 	}
 
@@ -904,38 +944,56 @@  static void netbk_fatal_tx_err(struct xenvif *vif)
 
 static int netbk_count_requests(struct xenvif *vif,
 				struct xen_netif_tx_request *first,
+				RING_IDX first_idx,
 				struct xen_netif_tx_request *txp,
 				int work_to_do)
 {
 	RING_IDX cons = vif->tx.req_cons;
-	int frags = 0;
+	int slots = 0;
+	int drop_err = 0;
 
 	if (!(first->flags & XEN_NETTXF_more_data))
 		return 0;
 
 	do {
-		if (frags >= work_to_do) {
-			netdev_err(vif->dev, "Need more frags\n");
+		if (slots >= work_to_do) {
+			netdev_err(vif->dev, "Need more slots\n");
 			netbk_fatal_tx_err(vif);
 			return -ENODATA;
 		}
 
-		if (unlikely(frags >= MAX_SKB_FRAGS)) {
-			netdev_err(vif->dev, "Too many frags\n");
+		/* Xen network protocol had implicit dependency on
+		 * MAX_SKB_FRAGS. XEN_NETIF_NR_SLOTS_MIN is set to the
+		 * historical MAX_SKB_FRAGS value 18 to honor the same
+		 * behavior as before. Any packet using more than 18
+		 * slots but less than max_skb_slots slots is dropped
+		 */
+		if (!drop_err && slots >= XEN_NETIF_NR_SLOTS_MIN) {
+			if (net_ratelimit())
+				netdev_dbg(vif->dev, "Too many slots\n");
+			drop_err = -E2BIG;
+		}
+
+		/* This guest is really using too many slots and
+		 * considered malicious.
+		 */
+		if (unlikely(slots >= max_skb_slots)) {
+			netdev_err(vif->dev,
+				   "Malicious frontend using too many slots\n");
 			netbk_fatal_tx_err(vif);
 			return -E2BIG;
 		}
 
-		memcpy(txp, RING_GET_REQUEST(&vif->tx, cons + frags),
+		memcpy(txp, RING_GET_REQUEST(&vif->tx, cons + slots),
 		       sizeof(*txp));
 		if (txp->size > first->size) {
-			netdev_err(vif->dev, "Frag is bigger than frame.\n");
+			netdev_err(vif->dev, "Packet is bigger than frame.\n");
 			netbk_fatal_tx_err(vif);
 			return -EIO;
 		}
 
 		first->size -= txp->size;
-		frags++;
+		slots++;
 
 		if (unlikely((txp->offset + txp->size) > PAGE_SIZE)) {
 			netdev_err(vif->dev, "txp->offset: %x, size: %u\n",
@@ -944,7 +1002,13 @@  static int netbk_count_requests(struct xenvif *vif,
 			return -EINVAL;
 		}
 	} while ((txp++)->flags & XEN_NETTXF_more_data);
-	return frags;
+
+	if (drop_err) {
+		netbk_tx_err(vif, first, first_idx + slots);
+		return drop_err;
+	}
+
+	return slots;
 }
 
 static struct page *xen_netbk_alloc_page(struct xen_netbk *netbk,
@@ -968,48 +1032,114 @@  static struct gnttab_copy *xen_netbk_get_requests(struct xen_netbk *netbk,
 	struct skb_shared_info *shinfo = skb_shinfo(skb);
 	skb_frag_t *frags = shinfo->frags;
 	u16 pending_idx = *((u16 *)skb->data);
-	int i, start;
+	u16 head_idx = 0;
+	int slot, start;
+	struct page *page;
+	pending_ring_idx_t index, start_idx = 0;
+	uint16_t dst_offset;
+	unsigned int nr_slots;
+	struct pending_tx_info *first = NULL;
+
+	/* At this point shinfo->nr_frags is in fact the number of
+	 * slots, which can be as large as XEN_NETIF_NR_SLOTS_MIN.
+	 */
+	nr_slots = shinfo->nr_frags;
 
 	/* Skip first skb fragment if it is on same page as header fragment. */
 	start = (frag_get_pending_idx(&shinfo->frags[0]) == pending_idx);
 
-	for (i = start; i < shinfo->nr_frags; i++, txp++) {
-		struct page *page;
-		pending_ring_idx_t index;
+	/* Coalesce tx requests, at this point the packet passed in
+	 * should be <= 64K. Any packets larger than 64K have been
+	 * handled in netbk_count_requests().
+	 */
+	for (shinfo->nr_frags = slot = start; slot < nr_slots;
+	     shinfo->nr_frags++) {
 		struct pending_tx_info *pending_tx_info =
 			netbk->pending_tx_info;
 
-		index = pending_index(netbk->pending_cons++);
-		pending_idx = netbk->pending_ring[index];
-		page = xen_netbk_alloc_page(netbk, pending_idx);
+		page = alloc_page(GFP_KERNEL|__GFP_COLD);
 		if (!page)
 			goto err;
 
-		gop->source.u.ref = txp->gref;
-		gop->source.domid = vif->domid;
-		gop->source.offset = txp->offset;
-
-		gop->dest.u.gmfn = virt_to_mfn(page_address(page));
-		gop->dest.domid = DOMID_SELF;
-		gop->dest.offset = txp->offset;
-
-		gop->len = txp->size;
-		gop->flags = GNTCOPY_source_gref;
+		dst_offset = 0;
+		first = NULL;
+		while (dst_offset < PAGE_SIZE && slot < nr_slots) {
+			gop->flags = GNTCOPY_source_gref;
+
+			gop->source.u.ref = txp->gref;
+			gop->source.domid = vif->domid;
+			gop->source.offset = txp->offset;
+
+			gop->dest.domid = DOMID_SELF;
+
+			gop->dest.offset = dst_offset;
+			gop->dest.u.gmfn = virt_to_mfn(page_address(page));
+
+			if (dst_offset + txp->size > PAGE_SIZE) {
+				/* This page can only merge a portion
+				 * of tx request. Do not increment any
+				 * pointer / counter here. The txp
+				 * will be dealt with in future
+				 * rounds, eventually hitting the
+				 * `else` branch.
+				 */
+				gop->len = PAGE_SIZE - dst_offset;
+				txp->offset += gop->len;
+				txp->size -= gop->len;
+				dst_offset += gop->len; /* quit loop */
+			} else {
+				/* This tx request can be merged in the page */
+				gop->len = txp->size;
+				dst_offset += gop->len;
+
+				index = pending_index(netbk->pending_cons++);
+
+				pending_idx = netbk->pending_ring[index];
+
+				memcpy(&pending_tx_info[pending_idx].req, txp,
+				       sizeof(*txp));
+				xenvif_get(vif);
+
+				pending_tx_info[pending_idx].vif = vif;
+
+				/* Poison these fields, corresponding
+				 * fields for head tx req will be set
+				 * to correct values after the loop.
+				 */
+				netbk->mmap_pages[pending_idx] = (void *)(~0UL);
+				pending_tx_info[pending_idx].head =
+					INVALID_PENDING_RING_IDX;
+
+				if (!first) {
+					first = &pending_tx_info[pending_idx];
+					start_idx = index;
+					head_idx = pending_idx;
+				}
+
+				txp++;
+				slot++;
+			}
 
-		gop++;
+			gop++;
+		}
 
-		memcpy(&pending_tx_info[pending_idx].req, txp, sizeof(*txp));
-		xenvif_get(vif);
-		pending_tx_info[pending_idx].vif = vif;
-		frag_set_pending_idx(&frags[i], pending_idx);
+		first->req.offset = 0;
+		first->req.size = dst_offset;
+		first->head = start_idx;
+		set_page_ext(page, netbk, head_idx);
+		netbk->mmap_pages[head_idx] = page;
+		frag_set_pending_idx(&frags[shinfo->nr_frags], head_idx);
 	}
 
+	BUG_ON(shinfo->nr_frags > MAX_SKB_FRAGS);
+
 	return gop;
 err:
 	/* Unwind, freeing all pages and sending error responses. */
-	while (i-- > start) {
-		xen_netbk_idx_release(netbk, frag_get_pending_idx(&frags[i]),
-				      XEN_NETIF_RSP_ERROR);
+	while (shinfo->nr_frags-- > start) {
+		xen_netbk_idx_release(netbk,
+				frag_get_pending_idx(&frags[shinfo->nr_frags]),
+				XEN_NETIF_RSP_ERROR);
 	}
 	/* The head too, if necessary. */
 	if (start)
@@ -1025,8 +1155,10 @@  static int xen_netbk_tx_check_gop(struct xen_netbk *netbk,
 	struct gnttab_copy *gop = *gopp;
 	u16 pending_idx = *((u16 *)skb->data);
 	struct skb_shared_info *shinfo = skb_shinfo(skb);
+	struct pending_tx_info *tx_info;
 	int nr_frags = shinfo->nr_frags;
 	int i, err, start;
+	u16 peek; /* peek into next tx request */
 
 	/* Check status of header. */
 	err = gop->status;
@@ -1038,11 +1170,21 @@  static int xen_netbk_tx_check_gop(struct xen_netbk *netbk,
 
 	for (i = start; i < nr_frags; i++) {
 		int j, newerr;
+		pending_ring_idx_t head;
 
 		pending_idx = frag_get_pending_idx(&shinfo->frags[i]);
+		tx_info = &netbk->pending_tx_info[pending_idx];
+		head = tx_info->head;
 
 		/* Check error status: if okay then remember grant handle. */
-		newerr = (++gop)->status;
+		do {
+			newerr = (++gop)->status;
+			if (newerr)
+				break;
+			peek = netbk->pending_ring[pending_index(++head)];
+		} while (netbk->pending_tx_info[peek].head
+			 == INVALID_PENDING_RING_IDX);
+
 		if (likely(!newerr)) {
 			/* Had a previous error? Invalidate this fragment. */
 			if (unlikely(err))
@@ -1267,11 +1409,12 @@  static unsigned xen_netbk_tx_build_gops(struct xen_netbk *netbk)
 	struct sk_buff *skb;
 	int ret;
 
-	while (((nr_pending_reqs(netbk) + MAX_SKB_FRAGS) < MAX_PENDING_REQS) &&
+	while ((nr_pending_reqs(netbk) + XEN_NETIF_NR_SLOTS_MIN
+		 < MAX_PENDING_REQS) &&
 		!list_empty(&netbk->net_schedule_list)) {
 		struct xenvif *vif;
 		struct xen_netif_tx_request txreq;
-		struct xen_netif_tx_request txfrags[MAX_SKB_FRAGS];
+		struct xen_netif_tx_request txfrags[max_skb_slots];
 		struct page *page;
 		struct xen_netif_extra_info extras[XEN_NETIF_EXTRA_TYPE_MAX-1];
 		u16 pending_idx;
@@ -1332,7 +1475,8 @@  static unsigned xen_netbk_tx_build_gops(struct xen_netbk *netbk)
 				continue;
 		}
 
-		ret = netbk_count_requests(vif, &txreq, txfrags, work_to_do);
+		ret = netbk_count_requests(vif, &txreq, idx,
+					   txfrags, work_to_do);
 		if (unlikely(ret < 0))
 			continue;
 
@@ -1359,7 +1503,7 @@  static unsigned xen_netbk_tx_build_gops(struct xen_netbk *netbk)
 		pending_idx = netbk->pending_ring[index];
 
 		data_len = (txreq.size > PKT_PROT_LEN &&
-			    ret < MAX_SKB_FRAGS) ?
+			    ret < XEN_NETIF_NR_SLOTS_MIN) ?
 			PKT_PROT_LEN : txreq.size;
 
 		skb = alloc_skb(data_len + NET_SKB_PAD + NET_IP_ALIGN,
@@ -1409,6 +1553,7 @@  static unsigned xen_netbk_tx_build_gops(struct xen_netbk *netbk)
 		memcpy(&netbk->pending_tx_info[pending_idx].req,
 		       &txreq, sizeof(txreq));
 		netbk->pending_tx_info[pending_idx].vif = vif;
+		netbk->pending_tx_info[pending_idx].head = index;
 		*((u16 *)skb->data) = pending_idx;
 
 		__skb_put(skb, data_len);
@@ -1539,7 +1684,10 @@  static void xen_netbk_idx_release(struct xen_netbk *netbk, u16 pending_idx,
 {
 	struct xenvif *vif;
 	struct pending_tx_info *pending_tx_info;
-	pending_ring_idx_t index;
+	pending_ring_idx_t head;
+	u16 peek; /* peek into next tx request */
+
+	BUG_ON(netbk->mmap_pages[pending_idx] == (void *)(~0UL));
 
 	/* Already complete? */
 	if (netbk->mmap_pages[pending_idx] == NULL)
@@ -1548,13 +1696,34 @@  static void xen_netbk_idx_release(struct xen_netbk *netbk, u16 pending_idx,
 	pending_tx_info = &netbk->pending_tx_info[pending_idx];
 
 	vif = pending_tx_info->vif;
+	head = pending_tx_info->head;
 
-	make_tx_response(vif, &pending_tx_info->req, status);
+	BUG_ON(head == INVALID_PENDING_RING_IDX);
+	BUG_ON(netbk->pending_ring[pending_index(head)] != pending_idx);
 
-	index = pending_index(netbk->pending_prod++);
-	netbk->pending_ring[index] = pending_idx;
+	do {
+		pending_ring_idx_t index;
+		pending_ring_idx_t idx = pending_index(head);
+		u16 info_idx = netbk->pending_ring[idx];
 
-	xenvif_put(vif);
+		pending_tx_info = &netbk->pending_tx_info[info_idx];
+		make_tx_response(vif, &pending_tx_info->req, status);
+
+		/* Setting any number other than
+		 * INVALID_PENDING_RING_IDX indicates this slot is
+		 * starting a new packet / ending a previous packet.
+		 */
+		pending_tx_info->head = 0;
+
+		index = pending_index(netbk->pending_prod++);
+		netbk->pending_ring[index] = netbk->pending_ring[info_idx];
+
+		xenvif_put(vif);
+
+		peek = netbk->pending_ring[pending_index(++head)];
+
+	} while (netbk->pending_tx_info[peek].head
+		 == INVALID_PENDING_RING_IDX);
 
 	netbk->mmap_pages[pending_idx]->mapping = 0;
 	put_page(netbk->mmap_pages[pending_idx]);
@@ -1613,7 +1782,8 @@  static inline int rx_work_todo(struct xen_netbk *netbk)
 static inline int tx_work_todo(struct xen_netbk *netbk)
 {
 
-	if (((nr_pending_reqs(netbk) + MAX_SKB_FRAGS) < MAX_PENDING_REQS) &&
+	if (((nr_pending_reqs(netbk) + XEN_NETIF_NR_SLOTS_MIN)
+	     < MAX_PENDING_REQS) &&
 			!list_empty(&netbk->net_schedule_list))
 		return 1;
 
diff --git a/include/xen/interface/io/netif.h b/include/xen/interface/io/netif.h
index 9dfc120..e829a09 100644
--- a/include/xen/interface/io/netif.h
+++ b/include/xen/interface/io/netif.h
@@ -13,6 +13,24 @@ 
 #include <xen/interface/grant_table.h>
 
 /*
+ * Older implementation of Xen network frontend / backend has an
+ * implicit dependency on the MAX_SKB_FRAGS as the maximum number of
+ * ring slots a skb can use. Netfront / netback may not work as
+ * expected when frontend and backend have different MAX_SKB_FRAGS.
+ *
+ * A better approach is to add mechanism for netfront / netback to
+ * negotiate this value. However we cannot fix all possible
+ * frontends, so we need to define a value which states the minimum
+ * slots backend must support.
+ *
+ * The minimum value derives from older Linux kernel's MAX_SKB_FRAGS
+ * (18), which is proved to work with most frontends. Any new backend
+ * which doesn't negotiate with frontend should expect frontend to
+ * send a valid packet using slots up to this value.
+ */
+#define XEN_NETIF_NR_SLOTS_MIN 18
+
+/*
  * Notifications after enqueuing any type of message should be conditional on
  * the appropriate req_event or rsp_event field in the shared ring.
  * If the client sends notification for rx requests then it should specify