| Message ID | 1367340644-19690-3-git-send-email-wei.liu2@citrix.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
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; -- 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
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. -- 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
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. -- 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
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. -- 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
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. -- 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
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. -- 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
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;
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(-)