netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH net-next 0/2] xen-netback: misc fixes
@ 2013-04-30 16:50 Wei Liu
  2013-04-30 16:50 ` [PATCH net-next 1/2] xen-netback: remove redundent parameter in netbk_count_requests Wei Liu
  2013-04-30 16:50 ` [PATCH net-next 2/2] xen-netback: avoid allocating variable size array on stack Wei Liu
  0 siblings, 2 replies; 11+ messages in thread
From: Wei Liu @ 2013-04-30 16:50 UTC (permalink / raw)
  To: xen-devel, netdev; +Cc: ian.campbell, jbeulich

These two patches fix glitches spotted by Jan. Please see commit logs for
details.


Thanks
Wei.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH net-next 1/2] xen-netback: remove redundent parameter in netbk_count_requests
  2013-04-30 16:50 [PATCH net-next 0/2] xen-netback: misc fixes Wei Liu
@ 2013-04-30 16:50 ` Wei Liu
  2013-05-01 10:10   ` Ian Campbell
  2013-05-02  9:31   ` Jan Beulich
  2013-04-30 16:50 ` [PATCH net-next 2/2] xen-netback: avoid allocating variable size array on stack Wei Liu
  1 sibling, 2 replies; 11+ messages in thread
From: Wei Liu @ 2013-04-30 16:50 UTC (permalink / raw)
  To: xen-devel, netdev; +Cc: ian.campbell, jbeulich, Wei Liu

Tracking down from the caller, first_idx is always equal to vif->tx.req_cons.
Remove it to avoid confusion.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
 drivers/net/xen-netback/netback.c |    6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/net/xen-netback/netback.c b/drivers/net/xen-netback/netback.c
index a2865f1..c44772d 100644
--- a/drivers/net/xen-netback/netback.c
+++ b/drivers/net/xen-netback/netback.c
@@ -928,7 +928,6 @@ 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)
 {
@@ -1005,7 +1004,7 @@ static int netbk_count_requests(struct xenvif *vif,
 	} while ((txp++)->flags & XEN_NETTXF_more_data);
 
 	if (drop_err) {
-		netbk_tx_err(vif, first, first_idx + slots);
+		netbk_tx_err(vif, first, cons + slots);
 		return drop_err;
 	}
 
@@ -1470,8 +1469,7 @@ static unsigned xen_netbk_tx_build_gops(struct xen_netbk *netbk)
 				continue;
 		}
 
-		ret = netbk_count_requests(vif, &txreq, idx,
-					   txfrags, work_to_do);
+		ret = netbk_count_requests(vif, &txreq, txfrags, work_to_do);
 		if (unlikely(ret < 0))
 			continue;
 
-- 
1.7.10.4

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH net-next 2/2] xen-netback: avoid allocating variable size array on stack
  2013-04-30 16:50 [PATCH net-next 0/2] xen-netback: misc fixes Wei Liu
  2013-04-30 16:50 ` [PATCH net-next 1/2] xen-netback: remove redundent parameter in netbk_count_requests Wei Liu
@ 2013-04-30 16:50 ` Wei Liu
  2013-05-01 10:32   ` Ian Campbell
  1 sibling, 1 reply; 11+ messages in thread
From: Wei Liu @ 2013-04-30 16:50 UTC (permalink / raw)
  To: xen-devel, netdev; +Cc: ian.campbell, jbeulich, Wei Liu

Tune xen_netbk_count_requests to not touch working array beyond limit, so that
we can make working array size constant.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
 drivers/net/xen-netback/netback.c |   26 +++++++++++++++++++++-----
 1 file changed, 21 insertions(+), 5 deletions(-)

diff --git a/drivers/net/xen-netback/netback.c b/drivers/net/xen-netback/netback.c
index c44772d..c6dc084 100644
--- a/drivers/net/xen-netback/netback.c
+++ b/drivers/net/xen-netback/netback.c
@@ -934,11 +934,15 @@ static int netbk_count_requests(struct xenvif *vif,
 	RING_IDX cons = vif->tx.req_cons;
 	int slots = 0;
 	int drop_err = 0;
+	int keep_looping;
 
 	if (!(first->flags & XEN_NETTXF_more_data))
 		return 0;
 
 	do {
+		struct xen_netif_tx_request dropped_tx = { 0 };
+		int cross_page = 0;
+
 		if (slots >= work_to_do) {
 			netdev_err(vif->dev,
 				   "Asked for %d slots but exceeds this limit\n",
@@ -972,8 +976,12 @@ static int netbk_count_requests(struct xenvif *vif,
 			drop_err = -E2BIG;
 		}
 
-		memcpy(txp, RING_GET_REQUEST(&vif->tx, cons + slots),
-		       sizeof(*txp));
+		if (!drop_err)
+			memcpy(txp, RING_GET_REQUEST(&vif->tx, cons + slots),
+			       sizeof(*txp));
+		else
+			memcpy(&dropped_tx, RING_GET_REQUEST(&vif->tx, cons + slots),
+			       sizeof(dropped_tx));
 
 		/* If the guest submitted a frame >= 64 KiB then
 		 * first->size overflowed and following slots will
@@ -995,13 +1003,21 @@ static int netbk_count_requests(struct xenvif *vif,
 		first->size -= txp->size;
 		slots++;
 
-		if (unlikely((txp->offset + txp->size) > PAGE_SIZE)) {
+		if (!drop_err)
+			cross_page = (txp->offset + txp->size) > PAGE_SIZE;
+		else
+			cross_page = (dropped_tx.offset + dropped_tx.size) > PAGE_SIZE;
+
+		if (unlikely(cross_page)) {
 			netdev_err(vif->dev, "Cross page boundary, txp->offset: %x, size: %u\n",
 				 txp->offset, txp->size);
 			netbk_fatal_tx_err(vif);
 			return -EINVAL;
 		}
-	} while ((txp++)->flags & XEN_NETTXF_more_data);
+
+		keep_looping = (!drop_err && (txp++)->flags & XEN_NETTXF_more_data) ||
+			(dropped_tx.flags & XEN_NETTXF_more_data);
+	} while (keep_looping);
 
 	if (drop_err) {
 		netbk_tx_err(vif, first, cons + slots);
@@ -1408,7 +1424,7 @@ static unsigned xen_netbk_tx_build_gops(struct xen_netbk *netbk)
 		!list_empty(&netbk->net_schedule_list)) {
 		struct xenvif *vif;
 		struct xen_netif_tx_request txreq;
-		struct xen_netif_tx_request txfrags[max_skb_slots];
+		struct xen_netif_tx_request txfrags[XEN_NETIF_NR_SLOTS_MIN];
 		struct page *page;
 		struct xen_netif_extra_info extras[XEN_NETIF_EXTRA_TYPE_MAX-1];
 		u16 pending_idx;
-- 
1.7.10.4

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH net-next 1/2] xen-netback: remove redundent parameter in netbk_count_requests
  2013-04-30 16:50 ` [PATCH net-next 1/2] xen-netback: remove redundent parameter in netbk_count_requests Wei Liu
@ 2013-05-01 10:10   ` Ian Campbell
  2013-05-02  9:31   ` Jan Beulich
  1 sibling, 0 replies; 11+ messages in thread
From: Ian Campbell @ 2013-05-01 10:10 UTC (permalink / raw)
  To: Wei Liu; +Cc: xen-devel, netdev, jbeulich

On Tue, 2013-04-30 at 17:50 +0100, Wei Liu wrote:
> Tracking down from the caller, first_idx is always equal to vif->tx.req_cons.
> Remove it to avoid confusion.
> 
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

> ---
>  drivers/net/xen-netback/netback.c |    6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/xen-netback/netback.c b/drivers/net/xen-netback/netback.c
> index a2865f1..c44772d 100644
> --- a/drivers/net/xen-netback/netback.c
> +++ b/drivers/net/xen-netback/netback.c
> @@ -928,7 +928,6 @@ 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)
>  {
> @@ -1005,7 +1004,7 @@ static int netbk_count_requests(struct xenvif *vif,
>  	} while ((txp++)->flags & XEN_NETTXF_more_data);
>  
>  	if (drop_err) {
> -		netbk_tx_err(vif, first, first_idx + slots);
> +		netbk_tx_err(vif, first, cons + slots);
>  		return drop_err;
>  	}
>  
> @@ -1470,8 +1469,7 @@ static unsigned xen_netbk_tx_build_gops(struct xen_netbk *netbk)
>  				continue;
>  		}
>  
> -		ret = netbk_count_requests(vif, &txreq, idx,
> -					   txfrags, work_to_do);
> +		ret = netbk_count_requests(vif, &txreq, txfrags, work_to_do);
>  		if (unlikely(ret < 0))
>  			continue;
>  

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH net-next 2/2] xen-netback: avoid allocating variable size array on stack
  2013-04-30 16:50 ` [PATCH net-next 2/2] xen-netback: avoid allocating variable size array on stack Wei Liu
@ 2013-05-01 10:32   ` Ian Campbell
  2013-05-01 10:53     ` Wei Liu
  0 siblings, 1 reply; 11+ messages in thread
From: Ian Campbell @ 2013-05-01 10:32 UTC (permalink / raw)
  To: Wei Liu; +Cc: xen-devel, netdev, jbeulich

On Tue, 2013-04-30 at 17:50 +0100, Wei Liu wrote:
> Tune xen_netbk_count_requests to not touch working array beyond limit, so that
> we can make working array size constant.

Is this really correct when max_skb_slots > XEN_NETIF_NR_SLOTS_MIN?
Seems like we would either overrun the array or drop frames which
max_skb_slots suggests we should accept?

If anything the array would need to be size by XEN_NETIF_NR_SLOTS_MAX
which a) doesn't exist and b) would be worse than using max_skb_slots. I
wouldn't be particularly averse to enforcing some sensible maximum on
max_skb_slots.

Other options:

Handle batches of work in <max_skb_slots sized bundles, but that gets
complex when you consider the case of an skb which crosses multiple such
bundles.

xen_netbk_get_requests() copes the tx req again into the pending_tx_info
-- any way we can arrange for this to just happen right in the first
place?

Or perhaps it is time for each vif to allocate a page of its own to
shadow the shared ring, and remove that field from pending_tx_info?
(which isn't really a net increase in memory usage, but might simplify
some things?)

One comment on the existing implementation below...

> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> ---
>  drivers/net/xen-netback/netback.c |   26 +++++++++++++++++++++-----
>  1 file changed, 21 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/xen-netback/netback.c b/drivers/net/xen-netback/netback.c
> index c44772d..c6dc084 100644
> --- a/drivers/net/xen-netback/netback.c
> +++ b/drivers/net/xen-netback/netback.c
> @@ -934,11 +934,15 @@ static int netbk_count_requests(struct xenvif *vif,
>  	RING_IDX cons = vif->tx.req_cons;
>  	int slots = 0;
>  	int drop_err = 0;
> +	int keep_looping;
>  
>  	if (!(first->flags & XEN_NETTXF_more_data))
>  		return 0;
>  
>  	do {
> +		struct xen_netif_tx_request dropped_tx = { 0 };
> +		int cross_page = 0;
> +
>  		if (slots >= work_to_do) {
>  			netdev_err(vif->dev,
>  				   "Asked for %d slots but exceeds this limit\n",
> @@ -972,8 +976,12 @@ static int netbk_count_requests(struct xenvif *vif,
>  			drop_err = -E2BIG;
>  		}
>  
> -		memcpy(txp, RING_GET_REQUEST(&vif->tx, cons + slots),
> -		       sizeof(*txp));
> +		if (!drop_err)
> +			memcpy(txp, RING_GET_REQUEST(&vif->tx, cons + slots),
> +			       sizeof(*txp));
> +		else
> +			memcpy(&dropped_tx, RING_GET_REQUEST(&vif->tx, cons + slots),
> +			       sizeof(dropped_tx));

Can we avoid needing to replicate if (!drop_err) txp else &dropped_tx
with a macro or some other trickery? e.g txp = &dropped_tx and then the
check is only on the txp++?

>  
>  		/* If the guest submitted a frame >= 64 KiB then
>  		 * first->size overflowed and following slots will
> @@ -995,13 +1003,21 @@ static int netbk_count_requests(struct xenvif *vif,
>  		first->size -= txp->size;
>  		slots++;
>  
> -		if (unlikely((txp->offset + txp->size) > PAGE_SIZE)) {
> +		if (!drop_err)
> +			cross_page = (txp->offset + txp->size) > PAGE_SIZE;
> +		else
> +			cross_page = (dropped_tx.offset + dropped_tx.size) > PAGE_SIZE;
> +
> +		if (unlikely(cross_page)) {
>  			netdev_err(vif->dev, "Cross page boundary, txp->offset: %x, size: %u\n",
>  				 txp->offset, txp->size);
>  			netbk_fatal_tx_err(vif);
>  			return -EINVAL;
>  		}
> -	} while ((txp++)->flags & XEN_NETTXF_more_data);
> +
> +		keep_looping = (!drop_err && (txp++)->flags & XEN_NETTXF_more_data) ||
> +			(dropped_tx.flags & XEN_NETTXF_more_data);
> +	} while (keep_looping);
>  
>  	if (drop_err) {
>  		netbk_tx_err(vif, first, cons + slots);
> @@ -1408,7 +1424,7 @@ static unsigned xen_netbk_tx_build_gops(struct xen_netbk *netbk)
>  		!list_empty(&netbk->net_schedule_list)) {
>  		struct xenvif *vif;
>  		struct xen_netif_tx_request txreq;
> -		struct xen_netif_tx_request txfrags[max_skb_slots];
> +		struct xen_netif_tx_request txfrags[XEN_NETIF_NR_SLOTS_MIN];
>  		struct page *page;
>  		struct xen_netif_extra_info extras[XEN_NETIF_EXTRA_TYPE_MAX-1];
>  		u16 pending_idx;

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH net-next 2/2] xen-netback: avoid allocating variable size array on stack
  2013-05-01 10:32   ` Ian Campbell
@ 2013-05-01 10:53     ` Wei Liu
  2013-05-01 11:21       ` Ian Campbell
  0 siblings, 1 reply; 11+ messages in thread
From: Wei Liu @ 2013-05-01 10:53 UTC (permalink / raw)
  To: Ian Campbell; +Cc: Wei Liu, xen-devel, netdev, jbeulich

On Wed, May 01, 2013 at 11:32:41AM +0100, Ian Campbell wrote:
> On Tue, 2013-04-30 at 17:50 +0100, Wei Liu wrote:
> > Tune xen_netbk_count_requests to not touch working array beyond limit, so that
> > we can make working array size constant.
> 
> Is this really correct when max_skb_slots > XEN_NETIF_NR_SLOTS_MIN?
> Seems like we would either overrun the array or drop frames which
> max_skb_slots suggests we should accept?
> 

So the max_skb_slots for now is the standard to determine whether a
guest is malicious, not the maximum slots we can process.

> If anything the array would need to be size by XEN_NETIF_NR_SLOTS_MAX
> which a) doesn't exist and b) would be worse than using max_skb_slots. I
> wouldn't be particularly averse to enforcing some sensible maximum on
> max_skb_slots.
> 

A sensible one is tricky, but presumably we would need it sooner or
later.

> Other options:
> 
> Handle batches of work in <max_skb_slots sized bundles, but that gets
> complex when you consider the case of an skb which crosses multiple such
> bundles.
> 
> xen_netbk_get_requests() copes the tx req again into the pending_tx_info
> -- any way we can arrange for this to just happen right in the first
> place?
> 

Isn't the point of having xen_netbk_count_requests to drop malformed
packets before wasting any effort processing them?

In the current design pending_tx_info only have valid tx request.

> Or perhaps it is time for each vif to allocate a page of its own to
> shadow the shared ring, and remove that field from pending_tx_info?
> (which isn't really a net increase in memory usage, but might simplify
> some things?)
> 

Not sure about this, will need to look into it.


Wei.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH net-next 2/2] xen-netback: avoid allocating variable size array on stack
  2013-05-01 10:53     ` Wei Liu
@ 2013-05-01 11:21       ` Ian Campbell
  2013-05-01 11:40         ` Wei Liu
  0 siblings, 1 reply; 11+ messages in thread
From: Ian Campbell @ 2013-05-01 11:21 UTC (permalink / raw)
  To: Wei Liu; +Cc: xen-devel, netdev, jbeulich

On Wed, 2013-05-01 at 11:53 +0100, Wei Liu wrote:
> On Wed, May 01, 2013 at 11:32:41AM +0100, Ian Campbell wrote:
> > On Tue, 2013-04-30 at 17:50 +0100, Wei Liu wrote:
> > > Tune xen_netbk_count_requests to not touch working array beyond limit, so that
> > > we can make working array size constant.
> > 
> > Is this really correct when max_skb_slots > XEN_NETIF_NR_SLOTS_MIN?
> > Seems like we would either overrun the array or drop frames which
> > max_skb_slots suggests we should accept?
> > 
> 
> So the max_skb_slots for now is the standard to determine whether a
> guest is malicious, not the maximum slots we can process.

Perhaps I've have misunderstood this patch then but it looks to me like
it will cause us to drop skbs which use slots > XEN_NETIF_NR_SLOTS_MIN
and < max_skb_slots, i.e. ones which are considered non-malicious by the
above definition. Or it will cause us to access indexes into
xen_netbk_tx_build_gops.txfrags which are > XEN_NETIF_NR_SLOTS_MIN.

If XEN_NETIF_NR_SLOTS_MIN==18 and max_skb_slots == 22 what will this
patch cause to happen to an SKB which uses 20 slots? Will it be dropped
or will it access index 20 into an array with size 18?

> > Other options:
> > 
> > Handle batches of work in <max_skb_slots sized bundles, but that gets
> > complex when you consider the case of an skb which crosses multiple such
> > bundles.
> > 
> > xen_netbk_get_requests() copes the tx req again into the pending_tx_info
> > -- any way we can arrange for this to just happen right in the first
> > place?
> > 
> 
> Isn't the point of having xen_netbk_count_requests to drop malformed
> packets before wasting any effort processing them?

Yes, but it seems to me like you are dropping non-malformed packets.

Also remember that the tx requests accumulated by
xen_netbk_count_requests into the txfrags array are subsequently used by
xen_netbk_get_requests to do the actual processing.

Ian.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH net-next 2/2] xen-netback: avoid allocating variable size array on stack
  2013-05-01 11:21       ` Ian Campbell
@ 2013-05-01 11:40         ` Wei Liu
  2013-05-01 11:47           ` Ian Campbell
  0 siblings, 1 reply; 11+ messages in thread
From: Wei Liu @ 2013-05-01 11:40 UTC (permalink / raw)
  To: Ian Campbell; +Cc: Wei Liu, xen-devel, netdev, jbeulich

On Wed, May 01, 2013 at 12:21:43PM +0100, Ian Campbell wrote:
> On Wed, 2013-05-01 at 11:53 +0100, Wei Liu wrote:
> > On Wed, May 01, 2013 at 11:32:41AM +0100, Ian Campbell wrote:
> > > On Tue, 2013-04-30 at 17:50 +0100, Wei Liu wrote:
> > > > Tune xen_netbk_count_requests to not touch working array beyond limit, so that
> > > > we can make working array size constant.
> > > 
> > > Is this really correct when max_skb_slots > XEN_NETIF_NR_SLOTS_MIN?
> > > Seems like we would either overrun the array or drop frames which
> > > max_skb_slots suggests we should accept?
> > > 
> > 
> > So the max_skb_slots for now is the standard to determine whether a
> > guest is malicious, not the maximum slots we can process.
> 
> Perhaps I've have misunderstood this patch then but it looks to me like
> it will cause us to drop skbs which use slots > XEN_NETIF_NR_SLOTS_MIN
> and < max_skb_slots, i.e. ones which are considered non-malicious by the
> above definition. Or it will cause us to access indexes into
> xen_netbk_tx_build_gops.txfrags which are > XEN_NETIF_NR_SLOTS_MIN.
> 

Any packet using more than XEN_NETIF_NR_SLOTS_MIN are considered
malformed at this point. The behavior is documented in previous commit
log. 2810e5b9a "xen-netback: coalesce slots in TX path and fix
regressions".

"""
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
"""

> If XEN_NETIF_NR_SLOTS_MIN==18 and max_skb_slots == 22 what will this
> patch cause to happen to an SKB which uses 20 slots? Will it be dropped
> or will it access index 20 into an array with size 18?
> 

That packet will be dropped.

> > > Other options:
> > > 
> > > Handle batches of work in <max_skb_slots sized bundles, but that gets
> > > complex when you consider the case of an skb which crosses multiple such
> > > bundles.
> > > 
> > > xen_netbk_get_requests() copes the tx req again into the pending_tx_info
> > > -- any way we can arrange for this to just happen right in the first
> > > place?
> > > 
> > 
> > Isn't the point of having xen_netbk_count_requests to drop malformed
> > packets before wasting any effort processing them?
> 
> Yes, but it seems to me like you are dropping non-malformed packets.
> 
> Also remember that the tx requests accumulated by
> xen_netbk_count_requests into the txfrags array are subsequently used by
> xen_netbk_get_requests to do the actual processing.
> 

Yes. But the coalesce code add a layer of complexity. It would require
rewriting that function and embbed error handling logic in it.

Now that we guarantee when we come to xen_netbk_get_requests the packet
must be valid, at which point we already construct a SKB for it.
Rewriting the whole process requires lots of code changes.


Wei.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH net-next 2/2] xen-netback: avoid allocating variable size array on stack
  2013-05-01 11:40         ` Wei Liu
@ 2013-05-01 11:47           ` Ian Campbell
  2013-05-01 13:01             ` Wei Liu
  0 siblings, 1 reply; 11+ messages in thread
From: Ian Campbell @ 2013-05-01 11:47 UTC (permalink / raw)
  To: Wei Liu; +Cc: xen-devel, netdev, jbeulich

On Wed, 2013-05-01 at 12:40 +0100, Wei Liu wrote:
> On Wed, May 01, 2013 at 12:21:43PM +0100, Ian Campbell wrote:
> > On Wed, 2013-05-01 at 11:53 +0100, Wei Liu wrote:
> > > On Wed, May 01, 2013 at 11:32:41AM +0100, Ian Campbell wrote:
> > > > On Tue, 2013-04-30 at 17:50 +0100, Wei Liu wrote:
> > > > > Tune xen_netbk_count_requests to not touch working array beyond limit, so that
> > > > > we can make working array size constant.
> > > > 
> > > > Is this really correct when max_skb_slots > XEN_NETIF_NR_SLOTS_MIN?
> > > > Seems like we would either overrun the array or drop frames which
> > > > max_skb_slots suggests we should accept?
> > > > 
> > > 
> > > So the max_skb_slots for now is the standard to determine whether a
> > > guest is malicious, not the maximum slots we can process.
> > 
> > Perhaps I've have misunderstood this patch then but it looks to me like
> > it will cause us to drop skbs which use slots > XEN_NETIF_NR_SLOTS_MIN
> > and < max_skb_slots, i.e. ones which are considered non-malicious by the
> > above definition. Or it will cause us to access indexes into
> > xen_netbk_tx_build_gops.txfrags which are > XEN_NETIF_NR_SLOTS_MIN.
> > 
> 
> Any packet using more than XEN_NETIF_NR_SLOTS_MIN are considered
> malformed at this point. The behavior is documented in previous commit
> log. 2810e5b9a "xen-netback: coalesce slots in TX path and fix
> regressions".
> 
> """
> 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
> """

OK, so my understanding was wrong and this patch is doing the right
thing.

However it does seem rather like NR_SLOTS_MIN and max_skb_slots are a
bit misnamed. They are actually NR_SLOTS_MAX and fatal_skb_slots? The
NR_SLOTS{MIN/MAX} disparity is particularly confusing in the context of
this code (I understand its the minimum that a backend must support, but
its still confusing in the context of these functions).

> > If XEN_NETIF_NR_SLOTS_MIN==18 and max_skb_slots == 22 what will this
> > patch cause to happen to an SKB which uses 20 slots? Will it be dropped
> > or will it access index 20 into an array with size 18?
> > 
> 
> That packet will be dropped.
> 
> > > > Other options:
> > > > 
> > > > Handle batches of work in <max_skb_slots sized bundles, but that gets
> > > > complex when you consider the case of an skb which crosses multiple such
> > > > bundles.
> > > > 
> > > > xen_netbk_get_requests() copes the tx req again into the pending_tx_info
> > > > -- any way we can arrange for this to just happen right in the first
> > > > place?
> > > > 
> > > 
> > > Isn't the point of having xen_netbk_count_requests to drop malformed
> > > packets before wasting any effort processing them?
> > 
> > Yes, but it seems to me like you are dropping non-malformed packets.
> > 
> > Also remember that the tx requests accumulated by
> > xen_netbk_count_requests into the txfrags array are subsequently used by
> > xen_netbk_get_requests to do the actual processing.
> > 
> 
> Yes. But the coalesce code add a layer of complexity. It would require
> rewriting that function and embbed error handling logic in it.
> 
> Now that we guarantee when we come to xen_netbk_get_requests the packet
> must be valid, at which point we already construct a SKB for it.
> Rewriting the whole process requires lots of code changes.

My point here was that if you aren't accumulating the last frags of a
valid frag into txfrags then things will break, but as you've explained
this is not the case.

Ian.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH net-next 2/2] xen-netback: avoid allocating variable size array on stack
  2013-05-01 11:47           ` Ian Campbell
@ 2013-05-01 13:01             ` Wei Liu
  0 siblings, 0 replies; 11+ messages in thread
From: Wei Liu @ 2013-05-01 13:01 UTC (permalink / raw)
  To: Ian Campbell; +Cc: Wei Liu, xen-devel, netdev, jbeulich

On Wed, May 01, 2013 at 12:47:06PM +0100, Ian Campbell wrote:
> On Wed, 2013-05-01 at 12:40 +0100, Wei Liu wrote:
> > On Wed, May 01, 2013 at 12:21:43PM +0100, Ian Campbell wrote:
> > > On Wed, 2013-05-01 at 11:53 +0100, Wei Liu wrote:
> > > > On Wed, May 01, 2013 at 11:32:41AM +0100, Ian Campbell wrote:
> > > > > On Tue, 2013-04-30 at 17:50 +0100, Wei Liu wrote:
> > > > > > Tune xen_netbk_count_requests to not touch working array beyond limit, so that
> > > > > > we can make working array size constant.
> > > > > 
> > > > > Is this really correct when max_skb_slots > XEN_NETIF_NR_SLOTS_MIN?
> > > > > Seems like we would either overrun the array or drop frames which
> > > > > max_skb_slots suggests we should accept?
> > > > > 
> > > > 
> > > > So the max_skb_slots for now is the standard to determine whether a
> > > > guest is malicious, not the maximum slots we can process.
> > > 
> > > Perhaps I've have misunderstood this patch then but it looks to me like
> > > it will cause us to drop skbs which use slots > XEN_NETIF_NR_SLOTS_MIN
> > > and < max_skb_slots, i.e. ones which are considered non-malicious by the
> > > above definition. Or it will cause us to access indexes into
> > > xen_netbk_tx_build_gops.txfrags which are > XEN_NETIF_NR_SLOTS_MIN.
> > > 
> > 
> > Any packet using more than XEN_NETIF_NR_SLOTS_MIN are considered
> > malformed at this point. The behavior is documented in previous commit
> > log. 2810e5b9a "xen-netback: coalesce slots in TX path and fix
> > regressions".
> > 
> > """
> > 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
> > """
> 
> OK, so my understanding was wrong and this patch is doing the right
> thing.
> 
> However it does seem rather like NR_SLOTS_MIN and max_skb_slots are a
> bit misnamed. They are actually NR_SLOTS_MAX and fatal_skb_slots? The
> NR_SLOTS{MIN/MAX} disparity is particularly confusing in the context of
> this code (I understand its the minimum that a backend must support, but
> its still confusing in the context of these functions).
> 

Yes probably the naming is weird.

Probably we can do

  #define XEN_NETBK_SLOTS_MAX XEN_NETIF_NR_SLOTS_MIN
  max_skb_slots -> fatal_skb_slots

if it makes things clearer.


Wei.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH net-next 1/2] xen-netback: remove redundent parameter in netbk_count_requests
  2013-04-30 16:50 ` [PATCH net-next 1/2] xen-netback: remove redundent parameter in netbk_count_requests Wei Liu
  2013-05-01 10:10   ` Ian Campbell
@ 2013-05-02  9:31   ` Jan Beulich
  1 sibling, 0 replies; 11+ messages in thread
From: Jan Beulich @ 2013-05-02  9:31 UTC (permalink / raw)
  To: Wei Liu; +Cc: ian.campbell, xen-devel, netdev

>>> On 30.04.13 at 18:50, Wei Liu <wei.liu2@citrix.com> wrote:
> Tracking down from the caller, first_idx is always equal to vif->tx.req_cons.
> Remove it to avoid confusion.
> 
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>

Suggested-by: Jan Beulich <jbeulich@suse.com>

> ---
>  drivers/net/xen-netback/netback.c |    6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/xen-netback/netback.c 
> b/drivers/net/xen-netback/netback.c
> index a2865f1..c44772d 100644
> --- a/drivers/net/xen-netback/netback.c
> +++ b/drivers/net/xen-netback/netback.c
> @@ -928,7 +928,6 @@ 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)
>  {
> @@ -1005,7 +1004,7 @@ static int netbk_count_requests(struct xenvif *vif,
>  	} while ((txp++)->flags & XEN_NETTXF_more_data);
>  
>  	if (drop_err) {
> -		netbk_tx_err(vif, first, first_idx + slots);
> +		netbk_tx_err(vif, first, cons + slots);
>  		return drop_err;
>  	}
>  
> @@ -1470,8 +1469,7 @@ static unsigned xen_netbk_tx_build_gops(struct 
> xen_netbk *netbk)
>  				continue;
>  		}
>  
> -		ret = netbk_count_requests(vif, &txreq, idx,
> -					   txfrags, work_to_do);
> +		ret = netbk_count_requests(vif, &txreq, txfrags, work_to_do);
>  		if (unlikely(ret < 0))
>  			continue;
>  
> -- 
> 1.7.10.4

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2013-05-02  9:31 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-04-30 16:50 [PATCH net-next 0/2] xen-netback: misc fixes Wei Liu
2013-04-30 16:50 ` [PATCH net-next 1/2] xen-netback: remove redundent parameter in netbk_count_requests Wei Liu
2013-05-01 10:10   ` Ian Campbell
2013-05-02  9:31   ` Jan Beulich
2013-04-30 16:50 ` [PATCH net-next 2/2] xen-netback: avoid allocating variable size array on stack Wei Liu
2013-05-01 10:32   ` Ian Campbell
2013-05-01 10:53     ` Wei Liu
2013-05-01 11:21       ` Ian Campbell
2013-05-01 11:40         ` Wei Liu
2013-05-01 11:47           ` Ian Campbell
2013-05-01 13:01             ` Wei Liu

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).