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:

quoted

On Wed, 2013-05-01 at 11:53 +0100, Wei Liu wrote:

quoted

On Wed, May 01, 2013 at 11:32:41AM +0100, Ian Campbell wrote:

quoted

On Tue, 2013-04-30 at 17:50 +0100, Wei Liu wrote:

quoted

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

quoted

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.

quoted

quoted

quoted

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.