Re: [PATCH net-next v2 1/9] xen-netback: Introduce TX grant map definitions

[Zoltan Kiss <zoltan.kiss@citrix.com>]

On 13/12/13 19:14, Wei Liu wrote:
>>>> +	spin_lock_irqsave(&vif->dealloc_lock, flags);
>>>> > >>+	do {
>>>> > >>+		pending_idx = ubuf->desc;
>>>> > >>+		ubuf = (struct ubuf_info *) ubuf->ctx;
>>>> > >>+		index = pending_index(vif->dealloc_prod);
>>>> > >>+		vif->dealloc_ring[index] = pending_idx;
>>>> > >>+		/* Sync with xenvif_tx_action_dealloc:
>>>> > >>+		 * insert idx then incr producer.
>>>> > >>+		 */
>>>> > >>+		smp_wmb();
>>>> > >>+		vif->dealloc_prod++;
>>>> > >>+	} while (ubuf);
>>>> > >>+	wake_up(&vif->dealloc_wq);
>>>> > >>+	spin_unlock_irqrestore(&vif->dealloc_lock, flags);
> [...]
>>> > >
>>>> > >>+		smp_rmb();
>>>> > >>+
>>>> > >>+		while (dc != dp) {
>>>> > >>+			pending_idx =
>>>> > >>+				vif->dealloc_ring[pending_index(dc++)];
>>>> > >>+
>>>> > >>+			/* Already unmapped? */
>>>> > >>+			if (vif->grant_tx_handle[pending_idx] ==
>>>> > >>+				NETBACK_INVALID_HANDLE) {
>>>> > >>+				netdev_err(vif->dev,
>>>> > >>+					"Trying to unmap invalid handle! "
>>>> > >>+					"pending_idx: %x\n", pending_idx);
>>>> > >>+				continue;
>>>> > >>+			}
>>> > >
>>> > >Should this be BUG_ON? AIUI this kthread should be the only one doing
>>> > >unmap, right?
>> >The NAPI instance can do it as well if it is a small packet fits
>> >into PKT_PROT_LEN. But still this scenario shouldn't really happen,
>> >I was just not sure we have to crash immediately. Maybe handle it as
>> >a fatal error and destroy the vif?
>> >
> It depends. If this is within the trust boundary, i.e. everything at the
> stage should have been sanitized then we should BUG_ON because there's
> clearly a bug somewhere in the sanitization process, or in the
> interaction of various backend routines.

My understanding is that crashing should be avoided if we can bail out 
somehow. At this point there is clearly a bug in netback somewhere, 
something unmapped that page before it should have happened, or at least 
that array get corrupted somehow. However there is a chance that 
xenvif_fatal_tx_err() can contain the issue, and the rest of the system 
can go unaffected.

Zoli