[PATCH net-next 0/2] xen-netback: misc fixes

[PATCH net-next 0/2] xen-netback: misc fixes

[Wei Liu]

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


Thanks
Wei.

[PATCH net-next 1/2] xen-netback: remove redundent parameter in netbk_count_requests

[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

Re: [PATCH net-next 1/2] xen-netback: remove redundent parameter in netbk_count_requests

[Ian Campbell]

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

Re: [PATCH net-next 1/2] xen-netback: remove redundent parameter in netbk_count_requests

[Jan Beulich]

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

[PATCH net-next 2/2] xen-netback: avoid allocating variable size array on stack

[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

Re: [PATCH net-next 2/2] xen-netback: avoid allocating variable size array on stack

[Ian Campbell]

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;

Re: [PATCH net-next 2/2] xen-netback: avoid allocating variable size array on stack

[Wei Liu]

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.

Re: [PATCH net-next 2/2] xen-netback: avoid allocating variable size array on stack

[Ian Campbell]

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.

Re: [PATCH net-next 2/2] xen-netback: avoid allocating variable size array on stack

[Wei Liu]

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.

Re: [PATCH net-next 2/2] xen-netback: avoid allocating variable size array on stack

[Ian Campbell]

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.

Re: [PATCH net-next 2/2] xen-netback: avoid allocating variable size array on stack

[Wei Liu]

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.